| Message ID | 20240325214912.323749-1-danielj@nvidia.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Remove RTNL lock protection of CVQ | expand |
For series: Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> On Mon, 25 Mar 2024 16:49:07 -0500, Daniel Jurgens <danielj@nvidia.com> wrote: > Currently the buffer used for control VQ commands is protected by the > RTNL lock. Previously this wasn't a major concern because the control > VQ was only used during device setup and user interaction. With the > recent addition of dynamic interrupt moderation the control VQ may be > used frequently during normal operation. > > This series removes the RNTL lock dependancy by introducing a spin lock > to protect the control buffer and writing SGs to the control VQ. > > Daniel Jurgens (4): > virtio_net: Store RSS setting in virtnet_info > virtio_net: Remove command data from control_buf > virtio_net: Add a lock for the command VQ. > virtio_net: Remove rtnl lock protection of command buffers > > drivers/net/virtio_net.c | 185 ++++++++++++++++++++++----------------- > 1 file changed, 104 insertions(+), 81 deletions(-) > > -- > 2.42.0 >
在 2024/3/26 上午5:49, Daniel Jurgens 写道: > Currently the buffer used for control VQ commands is protected by the > RTNL lock. Previously this wasn't a major concern because the control > VQ was only used during device setup and user interaction. With the > recent addition of dynamic interrupt moderation the control VQ may be > used frequently during normal operation. > > This series removes the RNTL lock dependancy by introducing a spin lock > to protect the control buffer and writing SGs to the control VQ. Hi Daniel. It's a nice piece of work, but now that we're talking about ctrlq adding interrupts, spin lock has some conflicts with its goals. For example, we expect the ethtool command to be blocked. Therefore, a mutex lock may be more suitable. Any how, the final conclusion may require some waiting. Regards, Heng > > Daniel Jurgens (4): > virtio_net: Store RSS setting in virtnet_info > virtio_net: Remove command data from control_buf > virtio_net: Add a lock for the command VQ. > virtio_net: Remove rtnl lock protection of command buffers > > drivers/net/virtio_net.c | 185 ++++++++++++++++++++++----------------- > 1 file changed, 104 insertions(+), 81 deletions(-) >
> From: Heng Qi <hengqi@linux.alibaba.com> > Sent: Monday, March 25, 2024 9:54 PM > To: Dan Jurgens <danielj@nvidia.com>; netdev@vger.kernel.org > Cc: mst@redhat.com; jasowang@redhat.com; xuanzhuo@linux.alibaba.com; > virtualization@lists.linux.dev; davem@davemloft.net; > edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; Jiri Pirko > <jiri@nvidia.com> > Subject: Re: [PATCH net-next 0/4] Remove RTNL lock protection of CVQ > > > > 在 2024/3/26 上午5:49, Daniel Jurgens 写道: > > Currently the buffer used for control VQ commands is protected by the > > RTNL lock. Previously this wasn't a major concern because the control > > VQ was only used during device setup and user interaction. With the > > recent addition of dynamic interrupt moderation the control VQ may be > > used frequently during normal operation. > > > > This series removes the RNTL lock dependancy by introducing a spin > > lock to protect the control buffer and writing SGs to the control VQ. > > Hi Daniel. > > It's a nice piece of work, but now that we're talking about ctrlq adding > interrupts, spin lock has some conflicts with its goals. For example, we expect > the ethtool command to be blocked. > Therefore, a mutex lock may be more suitable. > > Any how, the final conclusion may require some waiting. Thanks, Heng I took this a step further and made the ctrlq interrupt driven, but an internal reviewer pointed me to this: https://lore.kernel.org/lkml/20230413064027.13267-1-jasowang@redhat.com/ (sorry if it gets safelinked) It seemed there was little appetite to go that route last year, because of set RX mode behavior change, and consumption of an additional IRQ. Either way, I think the spin lock is still needed. In my interrupt driven implantation I was allocating a new control buffer instead of just the data fields. The spin lock was tighter around virtqueue_add_sgs, after the kick it would unlock and wait for a completion that would be triggered from the cvq callback. > > Regards, > Heng > > > > > Daniel Jurgens (4): > > virtio_net: Store RSS setting in virtnet_info > > virtio_net: Remove command data from control_buf > > virtio_net: Add a lock for the command VQ. > > virtio_net: Remove rtnl lock protection of command buffers > > > > drivers/net/virtio_net.c | 185 ++++++++++++++++++++++----------------- > > 1 file changed, 104 insertions(+), 81 deletions(-) > >
在 2024/3/26 下午12:11, Dan Jurgens 写道: >> From: Heng Qi <hengqi@linux.alibaba.com> >> Sent: Monday, March 25, 2024 9:54 PM >> To: Dan Jurgens <danielj@nvidia.com>; netdev@vger.kernel.org >> Cc: mst@redhat.com; jasowang@redhat.com; xuanzhuo@linux.alibaba.com; >> virtualization@lists.linux.dev; davem@davemloft.net; >> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; Jiri Pirko >> <jiri@nvidia.com> >> Subject: Re: [PATCH net-next 0/4] Remove RTNL lock protection of CVQ >> >> >> >> 在 2024/3/26 上午5:49, Daniel Jurgens 写道: >>> Currently the buffer used for control VQ commands is protected by the >>> RTNL lock. Previously this wasn't a major concern because the control >>> VQ was only used during device setup and user interaction. With the >>> recent addition of dynamic interrupt moderation the control VQ may be >>> used frequently during normal operation. >>> >>> This series removes the RNTL lock dependancy by introducing a spin >>> lock to protect the control buffer and writing SGs to the control VQ. >> Hi Daniel. >> >> It's a nice piece of work, but now that we're talking about ctrlq adding >> interrupts, spin lock has some conflicts with its goals. For example, we expect >> the ethtool command to be blocked. >> Therefore, a mutex lock may be more suitable. >> >> Any how, the final conclusion may require some waiting. > Thanks, Heng > > I took this a step further and made the ctrlq interrupt driven, but an internal reviewer pointed me to this: > https://lore.kernel.org/lkml/20230413064027.13267-1-jasowang@redhat.com/ (sorry if it gets safelinked) > > It seemed there was little appetite to go that route last year, because of set RX mode behavior change, and consumption of an additional IRQ. Hi DanielJ. Jason now supports this and wants to make changes to ctrlq. Yes, our requirements for ctrlq have become higher and we need to make updates as expected. So your patches look good: Reviewed-by: Heng Qi <hengqi@linux.alibaba.com> I will make further modifications on top of these. Regards, Heng > > Either way, I think the spin lock is still needed. In my interrupt driven implantation I was allocating a new control buffer instead of just the data fields. The spin lock was tighter around virtqueue_add_sgs, after the kick it would unlock and wait for a completion that would be triggered from the cvq callback. > > >> Regards, >> Heng >> >>> Daniel Jurgens (4): >>> virtio_net: Store RSS setting in virtnet_info >>> virtio_net: Remove command data from control_buf >>> virtio_net: Add a lock for the command VQ. >>> virtio_net: Remove rtnl lock protection of command buffers >>> >>> drivers/net/virtio_net.c | 185 ++++++++++++++++++++++----------------- >>> 1 file changed, 104 insertions(+), 81 deletions(-) >>>