On Tue, Aug 25, 2015 at 09:33:54PM +0300, Kirill A. Shutemov wrote:
> On Tue, Aug 25, 2015 at 01:44:13PM +0200, Vlastimil Babka wrote:
> > On 08/21/2015 02:10 PM, Kirill A. Shutemov wrote:
> > >On Thu, Aug 20, 2015 at 04:36:43PM -0700, Andrew Morton wrote:
> > >>On Wed, 19 Aug 2015 12:21:45 +0300 "Kirill A. Shutemov" <kirill.shutemov@xxxxxxxxxxxxxxx> wrote:
> > >>
> > >>>The patch introduces page->compound_head into third double word block in
> > >>>front of compound_dtor and compound_order. That means it shares storage
> > >>>space with:
> > >>>
> > >>> - page->lru.next;
> > >>> - page->next;
> > >>> - page->rcu_head.next;
> > >>> - page->pmd_huge_pte;
> > >>>
> >
> > We should probably ask Paul about the chances that rcu_head.next would like
> > to use the bit too one day?
>
> +Paul.
The call_rcu() function does stomp that bit, but if you stop using that
bit before you invoke call_rcu(), no problem.
Thanx, Paul
> > For pgtable_t I can't think of anything better than a warning in the generic
> > definition in include/asm-generic/page.h and hope that anyone reimplementing
> > it for a new arch will look there first.
>
> I will move it to other word, just in case.
>
> > The lru part is probably the hardest to prevent danger. It can be used for
> > any private purposes. Hopefully everyone currently uses only standard list
> > operations here, and the list poison values don't set bit 0. But I see there
> > can be some arbitrary CONFIG_ILLEGAL_POINTER_VALUE added to the poisons, so
> > maybe that's worth some build error check? Anyway we would be imposing
> > restrictions on types that are not ours, so there might be some
> > resistance...
>
> I will add BUILD_BUG_ON((unsigned long)LIST_POISON1 & 1);
>
> > >>Anyway, this is quite subtle and there's a risk that people will
> > >>accidentally break it later on. I don't think the patch puts
> > >>sufficient documentation in place to prevent this.
> > >
> > >I would appreciate for suggestion on place and form of documentation.
> > >
> > >>And even documentation might not be enough to prevent accidents.
> > >
> > >The only think I can propose is VM_BUG_ON() in PageTail() and
> > >compound_head() which would ensure that page->compound_page points to
> > >place within MAX_ORDER_NR_PAGES before the current page if bit 0 is set.
> >
> > That should probably catch some bad stuff, but probably only moments before
> > it would crash anyway if the pointer was bogus. But I also don't see better
> > way, because we can't proactively put checks in those who would "misbehave",
> > as we don't know who they are. Putting more debug checks in e.g. page
> > freeing might help, but probably not much.
>
> So, do you think it worth it or not after all?
> >
> > >Do you consider this helpful?
> > >
> > >>>
> > >>>...
> > >>>
> > >>>--- a/include/linux/mm_types.h
> > >>>+++ b/include/linux/mm_types.h
> > >>>@@ -120,7 +120,12 @@ struct page {
> > >>> };
> > >>> };
> > >>>
> > >>>- /* Third double word block */
> > >>>+ /*
> > >>>+ * Third double word block
> > >>>+ *
> > >>>+ * WARNING: bit 0 of the first word encode PageTail and *must* be 0
> > >>>+ * for non-tail pages.
> > >>>+ */
> > >>> union {
> > >>> struct list_head lru; /* Pageout list, eg. active_list
> > >>> * protected by zone->lru_lock !
> > >>>@@ -143,6 +148,7 @@ struct page {
> > >>> */
> > >>> /* First tail page of compound page */
> >
> > Note that compound_head is not just in the *first* tail page. Only the rest
> > is.
>
> Right.
>
> --
> Kirill A. Shutemov
>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>