Re: [PATCH 7/9] mm: split page table lock

[Andrew Morton]

Hugh Dickins <hugh@xxxxxxxxxxx> wrote:
>
> On Sun, 23 Oct 2005, Andrew Morton wrote:
> > Andrew Morton <akpm@xxxxxxxx> wrote:
> > >  Hugh Dickins <hugh@xxxxxxxxxxx> wrote:
> > >  >
> > >  > In this implementation, the spinlock is tucked inside the struct page of
> > >  >  the page table page: with a BUILD_BUG_ON in case it overflows - which it
> > >  >  would in the case of 32-bit PA-RISC with spinlock debugging enabled.
> > > 
> > >  eh?   It's going to overflow an unsigned long on x86 too:
> > 
> > Ah, I think I see what you've done: assume that .index, .lru and .virtual
> > are unused on pagetable pages, so we can just overwrite them.
> 
> That's right (so I'm ignoring some of your earlier stabs).
> Sorry, it's looking like my comment block was inadequate.
> 
> I'm also assuming that it'd be very quickly noticed, by bad_page
> or otherwise, if anyone changes these fields, so that what it's
> overwriting becomes significant.

Well it won't necesarily be noticed quickly - detecting an overflow depends
upon the right settings of CONFIG_PREEMPT, CONFIG_SMP, CONFIG_NR_CPUS,
WANT_PAGE_VIRTUAL, CONFIG_PAGE_OWNER and appropriate selection of
architecture and the absence of additional spinlock debugging patches and
the absence of reworked struct page layout!

I'm rather surprised that no architectures are already using page.mapping,
.index, .lru or .virtual in pte pages.

> Would it be better if pte_lock_init(page) explicitly initialize each
> field from _page->index onwards, so that any search for uses of that
> page field shows up pte_lock_init?  With the BUILD_BUG_ON adjusted
> somehow so _page->virtual is excluded (I tend to erase that one from
> my mind, but we most certainly don't want a spinlock overwriting it).
> 
> > ick.  I think I prefer the union, although it'll make struct page bigger
> > for CONFIG_PREEMPT+CONFIG_SMP+NR_CPUS>=4.    hmm.
> 
> Hmm indeed.  Definitely not the tradeoff I chose or would choose.

It's not that bad, really.  I do think that this approach is just too
dirty, sorry.  We can avoid it by moving something else into the union. 
lru, perhaps?
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/