Messages in this thread | | | From | JeeHeng Sia <> | Subject | RE: [PATCH v4 4/4] RISC-V: Add arch functions to support hibernation/suspend-to-disk | Date | Tue, 28 Feb 2023 07:34:26 +0000 |
| |
> -----Original Message----- > From: Andrew Jones <ajones@ventanamicro.com> > Sent: Tuesday, 28 February, 2023 3:30 PM > To: JeeHeng Sia <jeeheng.sia@starfivetech.com> > Cc: paul.walmsley@sifive.com; palmer@dabbelt.com; aou@eecs.berkeley.edu; linux-riscv@lists.infradead.org; linux- > kernel@vger.kernel.org; Leyfoon Tan <leyfoon.tan@starfivetech.com>; Mason Huo <mason.huo@starfivetech.com> > Subject: Re: [PATCH v4 4/4] RISC-V: Add arch functions to support hibernation/suspend-to-disk > > On Tue, Feb 28, 2023 at 06:33:59AM +0000, JeeHeng Sia wrote: > > > > > > > -----Original Message----- > > > From: Andrew Jones <ajones@ventanamicro.com> > > > Sent: Tuesday, 28 February, 2023 1:05 PM > > > To: JeeHeng Sia <jeeheng.sia@starfivetech.com> > > > Cc: paul.walmsley@sifive.com; palmer@dabbelt.com; aou@eecs.berkeley.edu; linux-riscv@lists.infradead.org; linux- > > > kernel@vger.kernel.org; Leyfoon Tan <leyfoon.tan@starfivetech.com>; Mason Huo <mason.huo@starfivetech.com> > > > Subject: Re: [PATCH v4 4/4] RISC-V: Add arch functions to support hibernation/suspend-to-disk > > > > > > On Tue, Feb 28, 2023 at 01:32:53AM +0000, JeeHeng Sia wrote: > > > > > > > > load image; > > > > > > > > loop: Create pbe chain, return error if failed; > > > > > > > > > > > > > > This loop pseudocode is incomplete. It's > > > > > > > > > > > > > > loop: > > > > > > > if (swsusp_page_is_forbidden(page) && swsusp_page_is_free(page)) > > > > > > > return page_address(page); > > > > > > > Create pbe chain, return error if failed; > > > > > > > ... > > > > > > > > > > > > > > which I pointed out explicitly in my last reply. Also, as I asked in my > > > > > > > last reply (and have been asking four times now, albeit less explicitly > > > > > > > the first two times), how do we know at least one PBE will be linked? > > > > > > 1 PBE correspond to 1 page, you shouldn't expect only 1 page is saved. > > > > > > > > > > I know PBEs correspond to pages. *Why* should I not expect only one page > > > > > is saved? Or, more importantly, why should I expect more than zero pages > > > > > are saved? > > > > > > > > > > Convincing answers might be because we *always* put the restore code in > > > > > pages which get added to the PBE list or that the original page tables > > > > > *always* get put in pages which get added to the PBE list. It's not very > > > > > convincing to simply *assume* that at least one random page will always > > > > > meet the PBE list criteria. > > > > > > > > > > > Hibernation core will do the calculation. If the PBEs (restore_pblist) linked successfully, the hibernated image will be restore > else > > > > > normal boot will take place. > > > > > > > Or, even more specifically this time, where is the proof that for each > > > > > > > hibernation resume, there exists some page such that > > > > > > > !swsusp_page_is_forbidden(page) or !swsusp_page_is_free(page) is true? > > > > > > forbidden_pages and free_pages are not contributed to the restore_pblist (as you already aware from the code). Infact, the > > > > > forbidden_pages and free_pages are not save into the disk. > > > > > > > > > > Exactly, so those pages are *not* going to contribute to the greater than > > > > > zero pages. What I've been asking for, from the beginning, is to know > > > > > which page(s) are known to *always* contribute to the list. Or, IOW, how > > > > > do you know the PBE list isn't empty, a.k.a restore_pblist isn't NULL? > > > > Well, this is keep going around in a circle, thought the answer is in the hibernation code. restore_pblist get the pointer from the > PBE, > > > and the PBE already checked for validity. > > > > > > It keeps going around in circles because you keep avoiding my question by > > > pointing out trivial linked list code. I'm not worried about the linked > > > list code being correct. My concern is that you're using a linked list > > > with an assumption that it is not empty. My question has been all along, > > > how do you know it's not empty? > > > > > > I'll change the way I ask this time. Please take a look at your PBE list > > > and let me know if there are PBEs on it that must be there on each > > > hibernation resume, e.g. the resume code page is there or whatever. > > Just to add on, it is not "my" PBE list but the list is from the hibernation core. As already draw out the scenarios for you, checking > should be done at the initialization phase. > > Your PBE list is your instance of the PBE list when you resume your > hibernation test. I'm simply asking you to dump the PBE list while > you resume a hibernation, and then tell me what's there. > > Please stop thinking about the trivial details of the code, like which > file a variable is in, and start thinking about how the code is being > used. A PBE list is a concept, your PBE list is an instance of that > concept, the code, which is the least interesting part, is just an > implementation of that concept. First, I want to understand the concept, > then we can worry about the code. > Dear Andrew, perhaps a conference call is better? otherwise it is going to waste the time in typing...Let me know how to join the call with you....thank you. > drew > > > > > > > > Can I suggest you to submit a patch to the hibernation core? > > > > > > Why? What's wrong with it? > > > > > > Thanks, > > > drew
| |