On 06/01, Jason Wang wrote: > > On Wed, May 31, 2023 at 5:14 PM Oleg Nesterov <oleg@xxxxxxxxxx> wrote: > > > > > > I don't understand you. OK, to simplify, suppose we have 2 global vars > > > > > > > > void *PTR = something_non_null; > > > > unsigned long FLAGS = -1ul; > > > > > > > > Now I think this code > > > > > > > > CPU_0 CPU_1 > > > > > > > > void *ptr = PTR; if (!test_and_set_bit(0, FLAGS)) > > > > clear_bit(0, FLAGS); PTR = NULL; > > > > BUG_ON(!ptr); > > > > > > > > is racy and can hit the BUG_ON(!ptr). > > > > > > This seems different to the above case? > > > > not sure, > > > > > And you can hit BUG_ON with > > > the following execution sequence: > > > > > > [cpu 0] clear_bit(0, FLAGS); > > > [cpu 1] if (!test_and_set_bit(0, FLAGS)) > > > [cpu 1] PTR = NULL; > > > [cpu 0] BUG_ON(!ptr) > > > > I don't understand this part... yes, we can hit this BUG_ON() without mb in > > between, this is what I tried to say. > > I may miss something, Or me... note that CPU_0 loads the global "PTR" into the local "ptr" before clear_bit. Since you have mentioned the program order: yes this lacks READ_ONCE() or barrier(), but the same is true for the code in vhost_worker(). So I still don't understand. > but the above is the sequence that is executed > by the processor (for each CPU, it's just the program order). So where > do you expect to place an mb can help? before clear_bit: CPU_0 void *ptr = PTR; mb(); // implies compiler barrier as well clear_bit(0, FLAGS); BUG_ON(!ptr); just in case... mb() in the code above is only for illustration, we can use smp_mb__before_atomic() + clear_bit(). Or just clear_bit_unlock(), iiuc the one-way barrier is fine in this case. > > > In vhost code, there's a condition before the clear_bit() which sits > > > inside llist_for_each_entry_safe(): > > > > > > #define llist_for_each_entry_safe(pos, n, node, member) \ > > > for (pos = llist_entry((node), typeof(*pos), member); \ > > > member_address_is_nonnull(pos, member) && \ > > > (n = llist_entry(pos->member.next, typeof(*n), member), true); \ > > > pos = n) > > > > > > The clear_bit() is a store which is not speculated, so there's a > > > control dependency, the store can't be executed until the condition > > > expression is evaluated which requires pos->member.next > > > (work->node.next) to be loaded. > > > > But llist_for_each_entry_safe() doesn't check "n", I mean, it is not that we have > > something like > > > > n = llist_entry(...); > > if (n) > > clear_bit(...); > > > > so I do not see how can we rely on the load-store control dependency. > > Just to make sure we are on the same page, the condition expression is > > member_address_is_nonnull(pos, member) && (n = > llist_entry(pos->member.next, typeof(*n), member), true) > > So it's something like: > > if (work->node && (work_next = work->node->next, true)) > clear_bit(&work->flags); > > So two loads from both work->node and work->node->next, and there's a > store which is clear_bit, then it's a load-store control dependencies? I guess you missed the comma expression... Let me rewrite your pseudo-code above, it is equivalent to if (work->node) { if ((work_next = work->node->next, true)) clear_bit(&work->flags); } another rewrite: if (work->node) { work_next = work->node->next; if ((work, true)) clear_bit(&work->flags); } and the final rewrite: if (work->node) { work_next = work->node->next; if (true) clear_bit(&work->flags); } so again, I do not see the load-store control dependency. Not to mention this code lacks READ_ONCE(). If we had something like if (work->node) { work_next = READ_ONCE(work->node->next); if (work_next) clear_bit(&work->flags); } instead, then yes, we could rely on the LOAD-STORE dependency. Oleg. _______________________________________________ Virtualization mailing list Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linuxfoundation.org/mailman/listinfo/virtualization