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.
> 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>