Re: [PATCH 3/3] fork, vhost: Use CLONE_THREAD to fix freezer/ps regression

[Jason Wang]

On Wed, May 31, 2023 at 5:14 PM Oleg Nesterov <oleg@xxxxxxxxxx> wrote:
>
> On 05/31, Jason Wang wrote:
> >
> > On Wed, May 31, 2023 at 3:25 PM Oleg Nesterov <oleg@xxxxxxxxxx> wrote:
> > >
> > > On 05/31, Jason Wang wrote:
> > > >
> > > > 在 2023/5/23 20:15, Oleg Nesterov 写道:
> > > > >
> > > > >             /* make sure flag is seen after deletion */
> > > > >             smp_wmb();
> > > > >             llist_for_each_entry_safe(work, work_next, node, node) {
> > > > >                     clear_bit(VHOST_WORK_QUEUED, &work->flags);
> > > > >
> > > > >I am not sure about smp_wmb + clear_bit. Once we clear VHOST_WORK_QUEUED,
> > > > >vhost_work_queue() can add this work again and change work->node->next.
> > > > >
> > > > >That is why we use _safe, but we need to ensure that llist_for_each_safe()
> > > > >completes LOAD(work->node->next) before VHOST_WORK_QUEUED is cleared.
> > > >
> > > > This should be fine since store is not speculated, so work->node->next needs
> > > > to be loaded before VHOST_WORK_QUEUED is cleared to meet the loop condition.
> > >
> > > 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, 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?

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

Thanks

>
> Oleg.
>