On 6/13/24 18:26, Keith Busch wrote:
On Thu, Jun 13, 2024 at 10:26:11AM +0530, Nilay Shroff wrote:
On 6/12/24 21:21, Keith Busch wrote:
+static inline void list_cut(struct list_head *list,
struct list_head *head, struct list_head *entry)+{
- list->next = entry;
- list->prev = head->prev;
- head->prev = entry->prev;
- entry->prev->next = head;
- entry->prev = list;
- list->prev->next = list;
+}
I am wondering whether we really need the _rcu version of list_cut here? I think that @head could point to an _rcu protected list and that's true for this patch. So there might be concurrent readers accessing @head using _rcu list-traversal primitives, such as list_for_each_entry_rcu().
An _rcu version of list_cut():
static inline void list_cut_rcu(struct list_head *list, struct list_head *head, struct list_head *entry) { list->next = entry; list->prev = head->prev; head->prev = entry->prev; rcu_assign_pointer(list_next_rcu(entry->prev), head); entry->prev = list; list->prev->next = list; }
I was initially thinking similiar, but this is really just doing a "list_del", and the rcu version calls the same generic __list_del() helper. To make this more clear, we could change
head->prev = entry->prev; entry->prev->next = head;
To just this:
__list_del(entry->prev, head);
And that also gets the "WRITE_ONCE" usage right.
Yeah this sounds reasonable.
But that's not the problem for the rcu case. It's the last line that's the problem:
list->prev->next = list;
We can't change forward pointers for any element being detached from @head because a reader iterating the list may see that new pointer value and end up in the wrong list, breaking iteration. A synchronize rcu needs to happen before forward pointers can be mucked with, so it still needs to be done in two steps. Oh bother...
Agree and probably we may break it down using this API: static inline void list_cut_rcu(struct list_head *list, struct list_head *head, struct list_head *entry, void (*sync)(void)) { list->next = entry; list->prev = head->prev; __list_del(entry->prev, head); sync(); entry->prev = list; list->prev->next = list; }
Thanks, --Nilay