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->attr.notify_next.next != 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
(kernfs_notify):
spin_lock_irqsave(&kernfs_notify_lock, flags);
if (kn->attr.notify_next.next != NULL) {
kernfs_get(kn);
llist_add(&kn->attr.notify_next, &kernfs_notify_list);
schedule_work(&kernfs_notify_work);
}
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->attr.notify_next.next != 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.
Thanks
-- Imran
