On Fri, Apr 19, 2024 at 12:12 AM Heng Qi <hengqi@xxxxxxxxxxxxxxxxx> wrote:
>
>
>
> 在 2024/4/18 下午11:48, Paolo Abeni 写道:
> > On Thu, 2024-04-18 at 15:38 +0000, Dan Jurgens wrote:
> >>> From: Paolo Abeni <pabeni@xxxxxxxxxx>
> >>> Sent: Thursday, April 18, 2024 5:57 AM
> >>> On Thu, 2024-04-18 at 15:36 +0800, Heng Qi wrote:
> >>>> 在 2024/4/18 下午2:42, Jason Wang 写道:
> >>>>> On Wed, Apr 17, 2024 at 3:31 AM Daniel Jurgens <danielj@xxxxxxxxxx>
> >>> wrote:
> >>>>>> The command VQ will no longer be protected by the RTNL lock. Use a
> >>>>>> spinlock to protect the control buffer header and the VQ.
> >>>>>>
> >>>>>> Signed-off-by: Daniel Jurgens <danielj@xxxxxxxxxx>
> >>>>>> Reviewed-by: Jiri Pirko <jiri@xxxxxxxxxx>
> >>>>>> ---
> >>>>>> drivers/net/virtio_net.c | 6 +++++-
> >>>>>> 1 file changed, 5 insertions(+), 1 deletion(-)
> >>>>>>
> >>>>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> >>>>>> index 0ee192b45e1e..d02f83a919a7 100644
> >>>>>> --- a/drivers/net/virtio_net.c
> >>>>>> +++ b/drivers/net/virtio_net.c
> >>>>>> @@ -282,6 +282,7 @@ struct virtnet_info {
> >>>>>>
> >>>>>> /* Has control virtqueue */
> >>>>>> bool has_cvq;
> >>>>>> + spinlock_t cvq_lock;
> >>>>> Spinlock is instead of mutex which is problematic as there's no
> >>>>> guarantee on when the driver will get a reply. And it became even
> >>>>> more serious after 0d197a147164 ("virtio-net: add cond_resched() to
> >>>>> the command waiting loop").
> >>>>>
> >>>>> Any reason we can't use mutex?
> >>>> Hi Jason,
> >>>>
> >>>> I made a patch set to enable ctrlq's irq on top of this patch set,
> >>>> which removes cond_resched().
> >>>>
> >>>> But I need a little time to test, this is close to fast. So could the
> >>>> topic about cond_resched + spin lock or mutex lock be wait?
> >>> The big problem is that until the cond_resched() is there, replacing the
> >>> mutex with a spinlock can/will lead to scheduling while atomic splats. We
> >>> can't intentionally introduce such scenario.
> >> When I created the series set_rx_mode wasn't moved to a work queue,
> >> and the cond_resched wasn't there.
> > Unfortunately cond_resched() is there right now.
>
> YES.
>
> >
> >> Mutex wasn't possible, then. If the CVQ is made to be event driven, then
> >> the lock can be released right after posting the work to the VQ.
> > That should work.
>
> Yes, I will test my new patches (ctrlq with irq enabled) soon, then the
> combination
> of the this set and mine MAY make deciding between mutex or spin lock
> easier.
>
> Thanks.
So I guess the plan is to let your series come first?
Thanks
>
> >
> >>> Side note: the compiler apparently does not like guard() construct, leading to
> >>> new warning, here and in later patches. I'm unsure if the code simplification
> >>> is worthy.
> >> I didn't see any warnings with GCC or clang. This is used other places in the kernel as well.
> >> gcc version 13.2.1 20230918 (Red Hat 13.2.1-3) (GCC)
> >> clang version 17.0.6 (Fedora 17.0.6-2.fc39)
> >>
> > See:
> >
> > https://patchwork.kernel.org/project/netdevbpf/patch/20240416193039.272997-4-danielj@xxxxxxxxxx/
> > https://netdev.bots.linux.dev/static/nipa/845178/13632442/build_32bit/stderr
> > https://netdev.bots.linux.dev/static/nipa/845178/13632442/build_allmodconfig_warn/stderr
> >
> > Cheers,
> >
> > Paolo
>
