[lkml]   [2022]   [Jul]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [RESEND PATCH] kernfs: Avoid re-adding kernfs_node into kernfs_notify_list.
Hello Tejun,
Thanks for your feedback.

On 2/7/22 6:11 am, Tejun Heo wrote:
> Hello,
> On Sat, Jul 02, 2022 at 01:46:04AM +1000, Imran Khan wrote:
>> @@ -992,9 +993,11 @@ void kernfs_notify(struct kernfs_node *kn)
>> rcu_read_unlock();
>> /* schedule work to kick fsnotify */
>> - kernfs_get(kn);
>> - llist_add(&kn->attr.notify_next, &kernfs_notify_list);
>> - schedule_work(&kernfs_notify_work);
>> + if (kn-> != NULL) {
>> + kernfs_get(kn);
>> + llist_add(&kn->attr.notify_next, &kernfs_notify_list);
>> + schedule_work(&kernfs_notify_work);
>> + }
> Aren't you just narrowing the race window here? What prevents two
> threads simultaneously testing for non NULL and then entering the
> addition block together?
Indeed that is possible.
> Looked at the llist code and it doesn't support multiple producers
> trying to add the same node, unfortunately, so I'm not sure llist is
> gonna work here. For now, the right thing to do prolly is reverting
> it.

Can we use kernfs_notify_lock like below snippet to serialize producers

spin_lock_irqsave(&kernfs_notify_lock, flags);
if (kn-> != NULL) {
llist_add(&kn->attr.notify_next, &kernfs_notify_list);
spin_unlock_irqsave(&kernfs_notify_lock, flags);

As per following comments at the beginning of llist.h

* Cases where locking is not needed:
* If there are multiple producers and multiple consumers, llist_add can be
* used in producers and llist_del_all can be used in consumers simultaneously
* without locking. Also a single consumer can use llist_del_first while
* multiple producers simultaneously use llist_add, without any locking.

Multiple producers and single consumer can work in parallel but as in our case
addition is dependent on kn-> != NULL, we may keep the
checking and list addition under kernfs_notify_lock and for consumer just lock
free->next = NULL under kernfs_notify_lock.

Having said this, I am okay with reverting the llist change as well, because
anyways it is not helping in the contentions that we are chasing here, but I
thought of sharing the above idea to see if it is reliable and better than
revert option.

-- Imran

 \ /
  Last update: 2022-07-03 13:11    [W:0.078 / U:0.104 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site