Re: linux-next: manual merge of the akpm-current tree with the tip tree

[NeilBrown]

On Wed, Apr 12 2017, Vlastimil Babka wrote:

> On 12.4.2017 8:46, Stephen Rothwell wrote:
>> Hi Andrew,
>> 
>> Today's linux-next merge of the akpm-current tree got conflicts in:
>> 
>>   drivers/block/nbd.c
>>   drivers/scsi/iscsi_tcp.c
>>   net/core/dev.c
>>   net/core/sock.c
>> 
>> between commit:
>> 
>>   717a94b5fc70 ("sched/core: Remove 'task' parameter and rename tsk_restore_flags() to current_restore_flags()")
>> 
>> from the tip tree and commit:
>> 
>>   61d5ad5b2e8a ("treewide: convert PF_MEMALLOC manipulations to new helpers")
>> 
>> from the akpm-current tree.
>
> Yeah, the first patch from Neil renames a function (as its subject says) and the
> second patch from me converts most of its users to new helpers specific to the
> PF_MEMALLOC flags.
>
>> I fixed it up (the latter is just a superset of the former, so I used
>
> It's not a complete superset though, more on that below.
>
>> that) and can carry the fix as necessary. This is now fixed as far as
>> linux-next is concerned, but any non trivial conflicts should be mentioned
>> to your upstream maintainer when your tree is submitted for merging.
>> You may also want to consider cooperating with the maintainer of the
>> conflicting tree to minimise any particularly complex conflicts.
>
> Hmm I could redo my patch on top of Neil's patch, but then Andrew would have to
> carry Neil's patch as well just to have a working mmotm? And then make sure to
> send my patch (but not Neil's) only after the tip tree is pulled? Would that
> work for the maintainers involved?
>
>> It looks like there may be more instances that the latter patch should
>> update.
>
> I see two remaining instances of current_restore_flags(). One in __do_softirq()
> is even for PF_MEMALLOC, but there the flag is cleared first and then set back,
> which is opposite of the common case that my helpers provide. The other in nfsd
> is for PF_LESS_THROTTLE which is not common enough to earn own helpers yet. IIRC
> Neil originally wanted to add a new one?

[Sorry - I thought I had sent this last week, but just noticed that I didn't]

In general, I'm not a fan of overly-specific helpers.

As a general rule, tsk_restore_flags() is probably better than
current_restore_flags() as it is more general.
However in this specific case, using any task other than 'current' would
almost certainly be incorrect code as locking is impossible.  So I
prefer the 'current' to be implicit, but the actual flag to be explicit.

If you are going to add helpers for setting/clearing PF flags, I would
much rather that you take

#define current_test_flags(f)   (current->flags & (f))
#define current_set_flags_nested(sp, f)         \
                (*(sp) = current->flags, current->flags |= (f))
#define current_clear_flags_nested(sp, f)       \
                (*(sp) = current->flags, current->flags &= ~(f))
#define current_restore_flags_nested(sp, f)     \
                (current->flags = ((current->flags & ~(f)) | (*(sp) & (f))))

out of fs/xfs/xfs_linux.h and use them globally.

Your
  noreclaim_flag = memalloc_reclaim_save()
becomes
  current_set_flags_nested&noreclaim_flag, PF_MEMALLOC)
which is more typing, but arguably easier to read.

If you then changed all uses of tsk_restore_flags() to use
current_restore_flags_nested(), my patch could be discarded as
irrelevant.

Thanks,
NeilBrown

Attachment: signature.asc
Description: PGP signature