Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230522025124.5863-1-michael.christie@oracle.com>
 <20230522025124.5863-4-michael.christie@oracle.com> <20230522123029.GA22159@redhat.com>
 <cfca7764-d210-6df9-e182-2c093101c6cf@oracle.com> <20230522174757.GC22159@redhat.com>
 <20230523121506.GA6562@redhat.com> <26c87be0-8e19-d677-a51b-e6821e6f7ae4@redhat.com>
 <20230531072449.GA25046@redhat.com> <CACGkMEv2kB9J1qGYkGkywk1YHV2gU2fMr7qx4vEv9L5f6qL5mg@mail.gmail.com>
 <20230531091432.GB25046@redhat.com>
In-Reply-To: <20230531091432.GB25046@redhat.com>
From:   Jason Wang <jasowang@redhat.com>
Date:   Thu, 1 Jun 2023 10:44:01 +0800
Message-ID: <CACGkMEvNrC5gc4ppp0QG-SNSbs_snrqwPkNBotffRRDJA1VJjQ@mail.gmail.com>
Subject: Re: [PATCH 3/3] fork, vhost: Use CLONE_THREAD to fix freezer/ps regression
To:     Oleg Nesterov <oleg@redhat.com>
Cc:     Mike Christie <michael.christie@oracle.com>, linux@leemhuis.info,
        nicolas.dichtel@6wind.com, axboe@kernel.dk, ebiederm@xmission.com,
        torvalds@linux-foundation.org, linux-kernel@vger.kernel.org,
        virtualization@lists.linux-foundation.org, mst@redhat.com,
        sgarzare@redhat.com, stefanha@redhat.com, brauner@kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, May 31, 2023 at 5:14=E2=80=AFPM Oleg Nesterov <oleg@redhat.com> wro=
te:
>
> On 05/31, Jason Wang wrote:
> >
> > On Wed, May 31, 2023 at 3:25=E2=80=AFPM Oleg Nesterov <oleg@redhat.com>=
 wrote:
> > >
> > > On 05/31, Jason Wang wrote:
> > > >
> > > > =E5=9C=A8 2023/5/23 20:15, Oleg Nesterov =E5=86=99=E9=81=93:
> > > > >
> > > > >             /* 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_eac=
h_safe()
> > > > >completes LOAD(work->node->next) before VHOST_WORK_QUEUED is clear=
ed.
> > > >
> > > > This should be fine since store is not speculated, so work->node->n=
ext needs
> > > > to be loaded before VHOST_WORK_QUEUED is cleared to meet the loop c=
ondition.
> > >
> > > I don't understand you. OK, to simplify, suppose we have 2 global var=
s
> > >
> > >         void *PTR =3D something_non_null;
> > >         unsigned long FLAGS =3D -1ul;
> > >
> > > Now I think this code
> > >
> > >         CPU_0                           CPU_1
> > >
> > >         void *ptr =3D PTR;                if (!test_and_set_bit(0, FL=
AGS))
> > >         clear_bit(0, FLAGS);                    PTR =3D 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 =3D 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 =3D llist_entry((node), typeof(*pos), member);        =
          \
> >              member_address_is_nonnull(pos, member) &&                 =
        \
> >                 (n =3D llist_entry(pos->member.next, typeof(*n), member=
), true); \
> >              pos =3D 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 =3D 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 =3D
llist_entry(pos->member.next, typeof(*n), member), true)

So it's something like:

if (work->node && (work_next =3D 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.
>