[lkml]   [2022]   [Jul]   [5]   [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.

On Sun, Jul 03, 2022 at 09:09:05PM +1000, Imran Khan wrote:
> Can we use kernfs_notify_lock like below snippet to serialize producers
> (kernfs_notify):
> spin_lock_irqsave(&kernfs_notify_lock, flags);
> if (kn-> != 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);

But then what's the point of using llist?

> 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.

It supports multiple producers in the sense that multiple producers can try
to add their own llist_nodes concurrently. It doesn't support multiple
producers trying to add the same llist_node whether that depends on NULL
check or not.

> 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.

The original conversion is broken and the right thing to do, for now, is
reverting it.



 \ /
  Last update: 2022-07-05 20:34    [W:0.080 / U:0.644 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site