On Mon, Oct 02, 2023 at 03:07:46PM +0100, Jonathan Cameron wrote: > On Tue, 19 Sep 2023 19:09:07 -0400 > Gregory Price <gourry.memverge@xxxxxxxxx> wrote: > > > @@ -2181,6 +2290,10 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, > > int start, i; > > int err = 0, err1; > > > > + /* This should never occur in regular operation */ > > + if (!mm && nodes_weight(task_nodes) > 0) > > You document below that task_nodes isn't used if !mm but it is used > to indicate this condition... > good point, and in fact the code below does ignore the contents of nodes_weight if mm is null, so i can just drop this check. > > +set_status: > > + *status = err; > > Given you update it unconditionally this seems to wipe out earlier > potential errors with the nid. > > > + > > + pages++; > > + status++; > > + } > > +} > status is a list, the status++ increments the list pointer. > > + /* > > + * When the mm argument to do_pages_move is null, the task_nodes > > + * argument is ignored, so pass in an empty nodemask as a dummy. > > If ignored, why bother clearing it? Looks like you are using it as > as signal rather than ignoring it. So not sure this comment is correct. > > > + */ > > + nodes_clear(dummy_nodes); I'm a bit of a stickler around uninitialized memory, seemed better to ensure the nodelist was empty to force errors associated with an empty nodelist should future updates break do_pages_move to remove the ignoring mechanism. That said, I updated the function such that it does actually ignore it, but I'll leave the here since there's no harm in clearing the nodemask and leads to more predictable behavior. Unless there is particularly strong feelings about this, then i'm fine to remove it.
