netdev - Re: [PATCH net-next v4 3/6] virtio_net: Add a lock for the command VQ.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a8ffbe97-22d1-4afe-bc6a-b4f9e7a8089a@linux.alibaba.com>
Date: Fri, 19 Apr 2024 00:12:39 +0800
From: Heng Qi <hengqi@...ux.alibaba.com>
To: Paolo Abeni <pabeni@...hat.com>
Cc: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
 "mst@...hat.com" <mst@...hat.com>,
 "xuanzhuo@...ux.alibaba.com" <xuanzhuo@...ux.alibaba.com>,
 "virtualization@...ts.linux.dev" <virtualization@...ts.linux.dev>,
 "davem@...emloft.net" <davem@...emloft.net>,
 "edumazet@...gle.com" <edumazet@...gle.com>,
 "kuba@...nel.org" <kuba@...nel.org>, Jiri Pirko <jiri@...dia.com>,
 Dan Jurgens <danielj@...dia.com>, Jason Wang <jasowang@...hat.com>
Subject: Re: [PATCH net-next v4 3/6] virtio_net: Add a lock for the command
 VQ.



在 2024/4/18 下午11:48, Paolo Abeni 写道:
> On Thu, 2024-04-18 at 15:38 +0000, Dan Jurgens wrote:
>>> From: Paolo Abeni <pabeni@...hat.com>
>>> 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@...dia.com>
>>> 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@...dia.com>
>>>>>> Reviewed-by: Jiri Pirko <jiri@...dia.com>
>>>>>> ---
>>>>>>    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.

>
>>> 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@nvidia.com/
> 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