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
