Re: CVS commit: src/sys/uvm

[Masao Uebayashi <uebayasi%tombi.co.jp@localhost>]

I take silence as "no objection".

On Thu, Dec 23, 2010 at 12:48:04PM +0900, Masao Uebayashi wrote:
> On Wed, Dec 22, 2010 at 05:37:57AM +0000, YAMAMOTO Takashi wrote:
> > hi,
> > 
> > > Could you ack this discussion?
> > 
> > sorry for dropping a ball.
> > 
> > > 
> > > On Tue, Dec 07, 2010 at 01:19:46AM +0900, Masao Uebayashi wrote:
> > >> On Thu, Nov 25, 2010 at 11:32:39PM +0000, YAMAMOTO Takashi wrote:
> > >> > [ adding cc: tech-kern@ ]
> > >> > 
> > >> > hi,
> > >> > 
> > >> > > On Wed, Nov 24, 2010 at 11:26:39PM -0800, Matt Thomas wrote:
> > >> > >> 
> > >> > >> On Nov 24, 2010, at 10:47 PM, Masao Uebayashi wrote:
> > >> > >> 
> > >> > >> > On Thu, Nov 25, 2010 at 05:44:21AM +0000, YAMAMOTO Takashi wrote:
> > >> > >> >> hi,
> > >> > >> >> 
> > >> > >> >>> On Thu, Nov 25, 2010 at 04:18:25AM +0000, YAMAMOTO Takashi 
> > >> > >> >>> wrote:
> > >> > >> >>>> hi,
> > >> > >> >>>> 
> > >> > >> >>>>> Hi, thanks for review.
> > >> > >> >>>>> 
> > >> > >> >>>>> On Thu, Nov 25, 2010 at 01:58:04AM +0000, YAMAMOTO Takashi 
> > >> > >> >>>>> wrote:
> > >> > >> >>>>>> hi,
> > >> > >> >>>>>> 
> > >> > >> >>>>>> - what's VM_PHYSSEG_OP_PG?
> > >> > >> >>>>> 
> > >> > >> >>>>> It's to lookup vm_physseg by "struct vm_page *", relying on 
> > >> > >> >>>>> that
> > >> > >> >>>>> "struct vm_page *[]" is allocated linearly.  It'll be used to 
> > >> > >> >>>>> remove
> > >> > >> >>>>> vm_page::phys_addr as we talked some time ago.
> > >> > >> >>>> 
> > >> > >> >>>> i'm not sure if commiting this unused uncommented code now 
> > >> > >> >>>> helps it.
> > >> > >> >>>> some try-and-benchmark cycles might be necessary given that
> > >> > >> >>>> vm_page <-> paddr conversion could be performace critical.
> > >> > >> >>> 
> > >> > >> >>> If you really care performance, we can directly pass "struct 
> > >> > >> >>> vm_page
> > >> > >> >>> *" to pmap_enter().
> > >> > >> >>> 
> > >> > >> >>> We're doing "struct vm_page *" -> "paddr_t" just before 
> > >> > >> >>> pmap_enter(),
> > >> > >> >>> then doing "paddr_t" -> "vm_physseg" reverse lookup again in
> > >> > >> >>> pmap_enter() to check if a given PA is managed.  What is really
> > >> > >> >>> needed here is, to lookup "struct vm_page *" -> "vm_physseg" 
> > >> > >> >>> once
> > >> > >> >>> and you'll know both paddr_t and managed or not.
> > >> > >> >> 
> > >> > >> >> i agree that the current code is not ideal in that respect.
> > >> > >> >> otoh, i'm not sure if passing vm_physseg around is a good idea.
> > >> > >> > 
> > >> > >> > It's great you share the interest.
> > >> > >> > 
> > >> > >> > I chose vm_physseg, because it was there.  I'm open to 
> > >> > >> > alternatives,
> > >> > >> > but I don't think you have many options...
> > >> > >> 
> > >> > >> Passing vm_page * doesn't work if the page isn't managed since there
> > >> > >> won't be a vm_page for the paddr_t.
> > >> > >> 
> > >> > >> Now passing both paddr_t and vm_page * would work and if the pointer
> > >> > >> to the vm_page, it would be an unmanaged mapping.  This also gives 
> > >> > >> the
> > >> > >> access to mdpg without another lookup.
> > >> > > 
> > >> > > What if XIP'ed md(4), where physical pages are in .data (or .rodata)?
> > >> > > 
> > >> > > And don't forget that you're the one who first pointed out that
> > >> > > allocating vm_pages for XIP is a pure waste of memory. ;)
> > >> > 
> > >> > i guess matt meant "if the pointer to the vm_page is NULL,".
> > >> > 
> > >> > > 
> > >> > > I'm allocating vm_pages, only because of phys_addr and loan_count.
> > >> > > I believe vm_pages is unnecessary for read-only XIP segments.
> > >> > > Because they're read-only, and stateless.
> > >> > > 
> > >> > > I've already concluded that the current "managed or not" model
> > >> > > doesn't work for XIP.  I'm pretty sure that my vm_physseg + off_t
> > >> > > model can explain everything.  I'm rather waiting for a counter
> > >> > > example how vm_physseg doesn't work...
> > >> > 
> > >> > i guess your suggestion is too vague.
> > >> > where do you want to use vm_physseg * + off_t instead of vm_page * ?
> > >> > getpages, pmap_enter, and?  how their function prototypes would be?
> > >> 
> > >> The basic idea is straightforward; always allocate vm_physseg for
> > >> memories/devices.  If a vm_physseg is used as general purpose
> > >> memory, you allocate vm_page[] (as vm_physseg::pgs).  If it's
> > >> potentially mapped as cached, you allocate pvh (as vm_physseg:pvh).
> > 
> > can you provide function prototypes?
> 
> I have no real code for this big picture at this moment.  Making
> vm_physseg available as reference is the first step.  This only
> changes uvm_page_physload() to return a pointer:
> 
>       -void uvm_page_physload();
>       +void *uvm_page_physload();
> 
> But this makes XIP pager MUCH cleaner.  The reason has been explained
> many times.
> 
> Making fault handlers and pagers to use vm_physseg * + off_t is
> the next step, and I don't intend to work on it now.  I just want
> to explain the big picture.
> 
> > 
> > >> 
> > >> Keep vm_physseg * + off_t array on stack.  If UVM objects uses
> > >> vm_page (e.g. vnode), its pager looks up vm_page -> vm_physseg *
> > >> + off_t *once* and cache it on stack.
> > 
> > do you mean something like this?
> >     struct {
> >             vm_physseg *hoge;
> >             off_t fuga;
> >     } foo [16];
> 
> Yes.
> 
> Or cache vm_page * with it, like:
> 
>       struct vm_id {
>               vm_physseg *seg;
>               off_t off;
>               vm_page *pg;
>       };
> 
>       uvm_fault()
>       {
>               vm_id pgs[];
>               :
>       }
> 
> Vnode pager (genfs_getpages) takes vm_page's by looking up
> vnode::v_uobj's list, or uvn_findpages().
> 
> When it returns back to fault handler, we have to lookup vm_physseg
> for each page.  Then fill the "seg" slot above (assuming we'll
> remove vm_page::phys_addr soon).
> 
> Fault handler calls per-vm_page operations iff vm_page slot is filled.
> XIP pages are not pageq'ed.  XIP pages don't need vm_page, but
> cached because it's vnode.
> 
> (Just in case, have you read my paper?)

-- 
Masao Uebayashi / Tombi Inc. / Tel: +81-90-9141-4635