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()?
> > + 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.
