On Tue, Aug 17, 2021 at 9:43 AM Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote:
>
> On Mon, Aug 16, 2021 at 05:59:52PM -0700, Andrew Morton wrote:
> > On Sun, 15 Aug 2021 14:10:34 +0800 yanghui <yanghui.def@xxxxxxxxxxxxx> wrote:
> >
> > > Servers happened below panic:
> > > Kernel version:5.4.56
> > > BUG: unable to handle page fault for address: 0000000000002c48
> > > RIP: 0010:__next_zones_zonelist+0x1d/0x40
> > > [264003.977696] RAX: 0000000000002c40 RBX: 0000000000100dca RCX: 0000000000000014
> > > [264003.977872] Call Trace:
> > > [264003.977888] __alloc_pages_nodemask+0x277/0x310
> > > [264003.977908] alloc_page_interleave+0x13/0x70
> > > [264003.977926] handle_mm_fault+0xf99/0x1390
> > > [264003.977951] __do_page_fault+0x288/0x500
> > > [264003.977979] ? schedule+0x39/0xa0
> > > [264003.977994] do_page_fault+0x30/0x110
> > > [264003.978010] page_fault+0x3e/0x50
> > >
> > > The reason of panic is that MAX_NUMNODES is passd in the third parameter
> > > in function __alloc_pages_nodemask(preferred_nid). So if to access
> > > zonelist->zoneref->zone_idx in __next_zones_zonelist the panic will happen.
> > >
> > > In offset_il_node(), first_node() return nid from pol->v.nodes, after
> > > this other threads may changed pol->v.nodes before next_node().
> > > This race condition will let next_node return MAX_NUMNODES.So put
> > > pol->nodes in a local variable.
> > >
> > > The race condition is between offset_il_node and cpuset_change_task_nodemask:
> > > CPU0: CPU1:
> > > alloc_pages_vma()
> > > interleave_nid(pol,)
> > > offset_il_node(pol,)
> > > first_node(pol->v.nodes) cpuset_change_task_nodemask
> > > //nodes==0xc mpol_rebind_task
> > > mpol_rebind_policy
> > > mpol_rebind_nodemask(pol,nodes)
> > > //nodes==0x3
> > > next_node(nid, pol->v.nodes)//return MAX_NUMNODES
> > >
> > >
> > > ...
> > >
> > > --- a/mm/mempolicy.c
> > > +++ b/mm/mempolicy.c
> > > @@ -1965,17 +1965,26 @@ unsigned int mempolicy_slab_node(void)
> > > */
> > > static unsigned offset_il_node(struct mempolicy *pol, unsigned long n)
> > > {
> > > - unsigned nnodes = nodes_weight(pol->nodes);
> > > - unsigned target;
> > > + nodemask_t nodemask = pol->nodes;
> >
> > Ouch. nodemask_t can be large - up to 128 bytes I think. This looks
> > like an expensive thing to be adding to fast paths (alloc_pages_vma()).
>
> Copying a fixed-size 128 bytes to the stack isn't going to be _that_
> expensive.
>
> > Plus it consumes a lot of stack.
>
> alloc_pages_vma() tends to be a leaf function, so not that bad.
>
> > > + unsigned int target, nnodes;
> > > int i;
> > > int nid;
> > > + /*
> > > + * The barrier will stabilize the nodemask in a register or on
> > > + * the stack so that it will stop changing under the code.
> > > + *
> > > + * Between first_node() and next_node(), pol->nodes could be changed
> > > + * by other threads. So we put pol->nodes in a local stack.
> > > + */
> > > + barrier();
>
> I think this could be an smp_rmb()?
Hi Matthew,
I have a question. Why is barrier() not enough?
Thanks.
>
> > > + nnodes = nodes_weight(nodemask);
> > > if (!nnodes)
> > > return numa_node_id();
> > > target = (unsigned int)n % nnodes;
> > > - nid = first_node(pol->nodes);
> > > + nid = first_node(nodemask);
> > > for (i = 0; i < target; i++)
> > > - nid = next_node(nid, pol->nodes);
> > > + nid = next_node(nid, nodemask);
> > > return nid;
> > > }
> >
> > The whole idea seems a bit hacky and fragile to be. We're dealing with
> > a potentially stale copy of the nodemask, yes?
>
> Correct. Also potentially a nodemask in the middle of being changed,
> so it may be some unholy amalgam of previous and next.
>
> > Ordinarily this is troublesome because there could be other problems
> > caused by working off stale data and a better fix would be to simply
> > avoid using stale data!
> >
> > But I guess that if the worst case is that once in a billion times,
> > interleaving hands out a page which isn't on the intended node then we
> > can live with that.
> >
> > And if this guess is correct and it is indeed the case that this is the
> > worst case, can we please spell all this out in the changelog.
>
> I think that taking a lock here is worse than copying to the stack.
> But that seems like the kind of thing that could be measured?
>
> I don't think that working off stale / amalgam data is a bad thing,
> we only need consistency. This is, after all, interleaved allocation.
> The user has asked for us, more or less, to choose a node at random to
> allocate from.
>
> What ruffles my feathers more is that we call next_node() up to n-2 times,
> and on average (n-1)/2 times (where n is the number of permitted nodes).
> I can't help but feel that we could do better to randomly distribute
> pages between nodes. Even having a special case for all-bits-set or
> n-contiguous-bits-set-and-all-other-bits-clear would go a long way to
> speed this up.
>
> I don't know if anyone has a real complaint about how long this takes
> to choose a node, though. I'm loathe to optimise this without data.
