| Message ID | 20230919074915.103110-6-hengqi@linux.alibaba.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | virtio-net: Fix and update interrupt moderation | expand |
On Tue, Sep 19, 2023 at 3:49 PM Heng Qi <hengqi@linux.alibaba.com> wrote: > > According to the definition of virtqueue coalescing spec[1]: > > Upon disabling and re-enabling a transmit virtqueue, the device MUST set > the coalescing parameters of the virtqueue to those configured through the > VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command, or, if the driver did not set > any TX coalescing parameters, to 0. > > Upon disabling and re-enabling a receive virtqueue, the device MUST set > the coalescing parameters of the virtqueue to those configured through the > VIRTIO_NET_CTRL_NOTF_COAL_RX_SET command, or, if the driver did not set > any RX coalescing parameters, to 0. > > We need to add this setting for vq resize (ethtool -G) where vq_reset happens. > > [1] https://lists.oasis-open.org/archives/virtio-dev/202303/msg00415.html > > Fixes: 394bd87764b6 ("virtio_net: support per queue interrupt coalesce command") I'm not sure this is a real fix as spec allows it to go zero? Thanks
在 2023/9/22 下午12:29, Jason Wang 写道: > On Tue, Sep 19, 2023 at 3:49 PM Heng Qi <hengqi@linux.alibaba.com> wrote: >> According to the definition of virtqueue coalescing spec[1]: >> >> Upon disabling and re-enabling a transmit virtqueue, the device MUST set >> the coalescing parameters of the virtqueue to those configured through the >> VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command, or, if the driver did not set >> any TX coalescing parameters, to 0. >> >> Upon disabling and re-enabling a receive virtqueue, the device MUST set >> the coalescing parameters of the virtqueue to those configured through the >> VIRTIO_NET_CTRL_NOTF_COAL_RX_SET command, or, if the driver did not set >> any RX coalescing parameters, to 0. >> >> We need to add this setting for vq resize (ethtool -G) where vq_reset happens. >> >> [1] https://lists.oasis-open.org/archives/virtio-dev/202303/msg00415.html >> >> Fixes: 394bd87764b6 ("virtio_net: support per queue interrupt coalesce command") > I'm not sure this is a real fix as spec allows it to go zero? The spec says that if the user has configured interrupt coalescing parameters, parameters need to be restored after vq_reset, otherwise set to 0. vi->intr_coal_tx and vi->intr_coal_rx always save the newest global parameters, regardless of whether the command is sent or not. So I think we need this patch it complies with the specification requirements. Thanks! > > Thanks
On Fri, Sep 22, 2023 at 1:02 PM Heng Qi <hengqi@linux.alibaba.com> wrote: > > > > 在 2023/9/22 下午12:29, Jason Wang 写道: > > On Tue, Sep 19, 2023 at 3:49 PM Heng Qi <hengqi@linux.alibaba.com> wrote: > >> According to the definition of virtqueue coalescing spec[1]: > >> > >> Upon disabling and re-enabling a transmit virtqueue, the device MUST set > >> the coalescing parameters of the virtqueue to those configured through the > >> VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command, or, if the driver did not set > >> any TX coalescing parameters, to 0. > >> > >> Upon disabling and re-enabling a receive virtqueue, the device MUST set > >> the coalescing parameters of the virtqueue to those configured through the > >> VIRTIO_NET_CTRL_NOTF_COAL_RX_SET command, or, if the driver did not set > >> any RX coalescing parameters, to 0. > >> > >> We need to add this setting for vq resize (ethtool -G) where vq_reset happens. > >> > >> [1] https://lists.oasis-open.org/archives/virtio-dev/202303/msg00415.html > >> > >> Fixes: 394bd87764b6 ("virtio_net: support per queue interrupt coalesce command") > > I'm not sure this is a real fix as spec allows it to go zero? > > The spec says that if the user has configured interrupt coalescing > parameters, > parameters need to be restored after vq_reset, otherwise set to 0. > vi->intr_coal_tx and vi->intr_coal_rx always save the newest global > parameters, > regardless of whether the command is sent or not. So I think we need > this patch > it complies with the specification requirements. How can we make sure the old coalescing parameters still make sense for the new ring size? Thanks > > Thanks! > > > > > Thanks >
在 2023/9/22 下午3:32, Jason Wang 写道: > On Fri, Sep 22, 2023 at 1:02 PM Heng Qi <hengqi@linux.alibaba.com> wrote: >> >> >> 在 2023/9/22 下午12:29, Jason Wang 写道: >>> On Tue, Sep 19, 2023 at 3:49 PM Heng Qi <hengqi@linux.alibaba.com> wrote: >>>> According to the definition of virtqueue coalescing spec[1]: >>>> >>>> Upon disabling and re-enabling a transmit virtqueue, the device MUST set >>>> the coalescing parameters of the virtqueue to those configured through the >>>> VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command, or, if the driver did not set >>>> any TX coalescing parameters, to 0. >>>> >>>> Upon disabling and re-enabling a receive virtqueue, the device MUST set >>>> the coalescing parameters of the virtqueue to those configured through the >>>> VIRTIO_NET_CTRL_NOTF_COAL_RX_SET command, or, if the driver did not set >>>> any RX coalescing parameters, to 0. >>>> >>>> We need to add this setting for vq resize (ethtool -G) where vq_reset happens. >>>> >>>> [1] https://lists.oasis-open.org/archives/virtio-dev/202303/msg00415.html >>>> >>>> Fixes: 394bd87764b6 ("virtio_net: support per queue interrupt coalesce command") >>> I'm not sure this is a real fix as spec allows it to go zero? >> The spec says that if the user has configured interrupt coalescing >> parameters, >> parameters need to be restored after vq_reset, otherwise set to 0. >> vi->intr_coal_tx and vi->intr_coal_rx always save the newest global >> parameters, >> regardless of whether the command is sent or not. So I think we need >> this patch >> it complies with the specification requirements. > How can we make sure the old coalescing parameters still make sense > for the new ring size? I'm not sure, ringsize has a wider range of changes. Maybe we should only keep coalescing parameters in cases where only vq_reset occurs (no ring size change involved)? Thanks! > > Thanks > >> Thanks! >> >>> Thanks
On Fri, 22 Sep 2023 15:32:39 +0800, Jason Wang <jasowang@redhat.com> wrote: > On Fri, Sep 22, 2023 at 1:02 PM Heng Qi <hengqi@linux.alibaba.com> wrote: > > > > > > > > 在 2023/9/22 下午12:29, Jason Wang 写道: > > > On Tue, Sep 19, 2023 at 3:49 PM Heng Qi <hengqi@linux.alibaba.com> wrote: > > >> According to the definition of virtqueue coalescing spec[1]: > > >> > > >> Upon disabling and re-enabling a transmit virtqueue, the device MUST set > > >> the coalescing parameters of the virtqueue to those configured through the > > >> VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command, or, if the driver did not set > > >> any TX coalescing parameters, to 0. > > >> > > >> Upon disabling and re-enabling a receive virtqueue, the device MUST set > > >> the coalescing parameters of the virtqueue to those configured through the > > >> VIRTIO_NET_CTRL_NOTF_COAL_RX_SET command, or, if the driver did not set > > >> any RX coalescing parameters, to 0. > > >> > > >> We need to add this setting for vq resize (ethtool -G) where vq_reset happens. > > >> > > >> [1] https://lists.oasis-open.org/archives/virtio-dev/202303/msg00415.html > > >> > > >> Fixes: 394bd87764b6 ("virtio_net: support per queue interrupt coalesce command") > > > I'm not sure this is a real fix as spec allows it to go zero? > > > > The spec says that if the user has configured interrupt coalescing > > parameters, > > parameters need to be restored after vq_reset, otherwise set to 0. > > vi->intr_coal_tx and vi->intr_coal_rx always save the newest global > > parameters, > > regardless of whether the command is sent or not. So I think we need > > this patch > > it complies with the specification requirements. > > How can we make sure the old coalescing parameters still make sense > for the new ring size? For the user, I don't think we should drop the config for the coalescing. Maybe the config does not make sense for the new ring size, but when the user just change the ring size, the config for the coalesing is missing, I think that is not good. Thanks. > > Thanks > > > > > Thanks! > > > > > > > > Thanks > > >
On Fri, Sep 22, 2023 at 3:58 PM Heng Qi <hengqi@linux.alibaba.com> wrote: > > > > 在 2023/9/22 下午3:32, Jason Wang 写道: > > On Fri, Sep 22, 2023 at 1:02 PM Heng Qi <hengqi@linux.alibaba.com> wrote: > >> > >> > >> 在 2023/9/22 下午12:29, Jason Wang 写道: > >>> On Tue, Sep 19, 2023 at 3:49 PM Heng Qi <hengqi@linux.alibaba.com> wrote: > >>>> According to the definition of virtqueue coalescing spec[1]: > >>>> > >>>> Upon disabling and re-enabling a transmit virtqueue, the device MUST set > >>>> the coalescing parameters of the virtqueue to those configured through the > >>>> VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command, or, if the driver did not set > >>>> any TX coalescing parameters, to 0. > >>>> > >>>> Upon disabling and re-enabling a receive virtqueue, the device MUST set > >>>> the coalescing parameters of the virtqueue to those configured through the > >>>> VIRTIO_NET_CTRL_NOTF_COAL_RX_SET command, or, if the driver did not set > >>>> any RX coalescing parameters, to 0. > >>>> > >>>> We need to add this setting for vq resize (ethtool -G) where vq_reset happens. > >>>> > >>>> [1] https://lists.oasis-open.org/archives/virtio-dev/202303/msg00415.html > >>>> > >>>> Fixes: 394bd87764b6 ("virtio_net: support per queue interrupt coalesce command") > >>> I'm not sure this is a real fix as spec allows it to go zero? > >> The spec says that if the user has configured interrupt coalescing > >> parameters, > >> parameters need to be restored after vq_reset, otherwise set to 0. > >> vi->intr_coal_tx and vi->intr_coal_rx always save the newest global > >> parameters, > >> regardless of whether the command is sent or not. So I think we need > >> this patch > >> it complies with the specification requirements. > > How can we make sure the old coalescing parameters still make sense > > for the new ring size? > > I'm not sure, ringsize has a wider range of changes. Maybe we should > only keep coalescing > parameters in cases where only vq_reset occurs (no ring size change > involved)? Probably but do we actually have a user other than resize now? Thanks > > Thanks! > > > > > Thanks > > > >> Thanks! > >> > >>> Thanks >
On Fri, Sep 22, 2023 at 5:56 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote: > > On Fri, 22 Sep 2023 15:32:39 +0800, Jason Wang <jasowang@redhat.com> wrote: > > On Fri, Sep 22, 2023 at 1:02 PM Heng Qi <hengqi@linux.alibaba.com> wrote: > > > > > > > > > > > > 在 2023/9/22 下午12:29, Jason Wang 写道: > > > > On Tue, Sep 19, 2023 at 3:49 PM Heng Qi <hengqi@linux.alibaba.com> wrote: > > > >> According to the definition of virtqueue coalescing spec[1]: > > > >> > > > >> Upon disabling and re-enabling a transmit virtqueue, the device MUST set > > > >> the coalescing parameters of the virtqueue to those configured through the > > > >> VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command, or, if the driver did not set > > > >> any TX coalescing parameters, to 0. > > > >> > > > >> Upon disabling and re-enabling a receive virtqueue, the device MUST set > > > >> the coalescing parameters of the virtqueue to those configured through the > > > >> VIRTIO_NET_CTRL_NOTF_COAL_RX_SET command, or, if the driver did not set > > > >> any RX coalescing parameters, to 0. > > > >> > > > >> We need to add this setting for vq resize (ethtool -G) where vq_reset happens. > > > >> > > > >> [1] https://lists.oasis-open.org/archives/virtio-dev/202303/msg00415.html > > > >> > > > >> Fixes: 394bd87764b6 ("virtio_net: support per queue interrupt coalesce command") > > > > I'm not sure this is a real fix as spec allows it to go zero? > > > > > > The spec says that if the user has configured interrupt coalescing > > > parameters, > > > parameters need to be restored after vq_reset, otherwise set to 0. > > > vi->intr_coal_tx and vi->intr_coal_rx always save the newest global > > > parameters, > > > regardless of whether the command is sent or not. So I think we need > > > this patch > > > it complies with the specification requirements. > > > > How can we make sure the old coalescing parameters still make sense > > for the new ring size? > > For the user, I don't think we should drop the config for the coalescing. > Maybe the config does not make sense for the new ring size, but when the user > just change the ring size, the config for the coalesing is missing, I think > that is not good. How did other drivers deal with this? Thanks > > Thanks. > > > > > > > > Thanks > > > > > > > > Thanks! > > > > > > > > > > > Thanks > > > > > >
在 2023/9/25 上午10:29, Jason Wang 写道: > On Fri, Sep 22, 2023 at 5:56 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote: >> On Fri, 22 Sep 2023 15:32:39 +0800, Jason Wang <jasowang@redhat.com> wrote: >>> On Fri, Sep 22, 2023 at 1:02 PM Heng Qi <hengqi@linux.alibaba.com> wrote: >>>> >>>> >>>> 在 2023/9/22 下午12:29, Jason Wang 写道: >>>>> On Tue, Sep 19, 2023 at 3:49 PM Heng Qi <hengqi@linux.alibaba.com> wrote: >>>>>> According to the definition of virtqueue coalescing spec[1]: >>>>>> >>>>>> Upon disabling and re-enabling a transmit virtqueue, the device MUST set >>>>>> the coalescing parameters of the virtqueue to those configured through the >>>>>> VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command, or, if the driver did not set >>>>>> any TX coalescing parameters, to 0. >>>>>> >>>>>> Upon disabling and re-enabling a receive virtqueue, the device MUST set >>>>>> the coalescing parameters of the virtqueue to those configured through the >>>>>> VIRTIO_NET_CTRL_NOTF_COAL_RX_SET command, or, if the driver did not set >>>>>> any RX coalescing parameters, to 0. >>>>>> >>>>>> We need to add this setting for vq resize (ethtool -G) where vq_reset happens. >>>>>> >>>>>> [1] https://lists.oasis-open.org/archives/virtio-dev/202303/msg00415.html >>>>>> >>>>>> Fixes: 394bd87764b6 ("virtio_net: support per queue interrupt coalesce command") >>>>> I'm not sure this is a real fix as spec allows it to go zero? >>>> The spec says that if the user has configured interrupt coalescing >>>> parameters, >>>> parameters need to be restored after vq_reset, otherwise set to 0. >>>> vi->intr_coal_tx and vi->intr_coal_rx always save the newest global >>>> parameters, >>>> regardless of whether the command is sent or not. So I think we need >>>> this patch >>>> it complies with the specification requirements. >>> How can we make sure the old coalescing parameters still make sense >>> for the new ring size? >> For the user, I don't think we should drop the config for the coalescing. >> Maybe the config does not make sense for the new ring size, but when the user >> just change the ring size, the config for the coalesing is missing, I think >> that is not good. > How did other drivers deal with this? Test on mlx5, the behavior is the same as what this patch does: #1 ethtool -g eth0 ... Current hardware settings: RX: 8192 RX Mini: n/a RX Jumbo: n/a Tx: 1024 ... #2 ethtool -c eth0 Coalesce parameters for eth0: Adaptive RX: off TX: off ... rx-usecs: 11 rx-frames: 32 rx-usecs-irq: n/a rx-frames-irq: n/a ... #3 sudo ethtool -G eth0 rx 4096 #4 ethtool -g eth0 Ring parameters for eth0: ... Current hardware settings: RX: 4096 RX Mini: n/a RX Jumbo: n/a TX: 1024 ... #4 ethtool -c eth0 Coalesce parameters for eth0: Adaptive RX: off TX: off ... rx-usecs: 11 rx-frames: 32 rx-usecs-irq: n/a rx-frames-irq: n/a ... And I check the code, except for the parameters related to ring size, which are changed, other parameters in structure mlx5e_params are retained. Thanks > > Thanks > >> Thanks. >> >> >> >> >>> Thanks >>> >>>> Thanks! >>>> >>>>> Thanks
在 2023/9/25 上午10:29, Jason Wang 写道: > On Fri, Sep 22, 2023 at 3:58 PM Heng Qi <hengqi@linux.alibaba.com> wrote: >> >> >> 在 2023/9/22 下午3:32, Jason Wang 写道: >>> On Fri, Sep 22, 2023 at 1:02 PM Heng Qi <hengqi@linux.alibaba.com> wrote: >>>> >>>> 在 2023/9/22 下午12:29, Jason Wang 写道: >>>>> On Tue, Sep 19, 2023 at 3:49 PM Heng Qi <hengqi@linux.alibaba.com> wrote: >>>>>> According to the definition of virtqueue coalescing spec[1]: >>>>>> >>>>>> Upon disabling and re-enabling a transmit virtqueue, the device MUST set >>>>>> the coalescing parameters of the virtqueue to those configured through the >>>>>> VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command, or, if the driver did not set >>>>>> any TX coalescing parameters, to 0. >>>>>> >>>>>> Upon disabling and re-enabling a receive virtqueue, the device MUST set >>>>>> the coalescing parameters of the virtqueue to those configured through the >>>>>> VIRTIO_NET_CTRL_NOTF_COAL_RX_SET command, or, if the driver did not set >>>>>> any RX coalescing parameters, to 0. >>>>>> >>>>>> We need to add this setting for vq resize (ethtool -G) where vq_reset happens. >>>>>> >>>>>> [1] https://lists.oasis-open.org/archives/virtio-dev/202303/msg00415.html >>>>>> >>>>>> Fixes: 394bd87764b6 ("virtio_net: support per queue interrupt coalesce command") >>>>> I'm not sure this is a real fix as spec allows it to go zero? >>>> The spec says that if the user has configured interrupt coalescing >>>> parameters, >>>> parameters need to be restored after vq_reset, otherwise set to 0. >>>> vi->intr_coal_tx and vi->intr_coal_rx always save the newest global >>>> parameters, >>>> regardless of whether the command is sent or not. So I think we need >>>> this patch >>>> it complies with the specification requirements. >>> How can we make sure the old coalescing parameters still make sense >>> for the new ring size? >> I'm not sure, ringsize has a wider range of changes. Maybe we should >> only keep coalescing >> parameters in cases where only vq_reset occurs (no ring size change >> involved)? > Probably but do we actually have a user other than resize now? Not yet. What I mean is if there is one in the future, for example in xdp's processing etc., we can reserve parameters for it. Thanks > > Thanks > >> Thanks! >> >>> Thanks >>> >>>> Thanks! >>>> >>>>> Thanks
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index f9a7f6afd099..9888218abab4 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -2855,6 +2855,9 @@ static void virtnet_get_ringparam(struct net_device *dev, ring->tx_pending = virtqueue_get_vring_size(vi->sq[0].vq); } +static int virtnet_send_ctrl_coal_vq_cmd(struct virtnet_info *vi, + u16 vqn, u32 max_usecs, u32 max_packets); + static int virtnet_set_ringparam(struct net_device *dev, struct ethtool_ringparam *ring, struct kernel_ethtool_ringparam *kernel_ring, @@ -2890,12 +2893,36 @@ static int virtnet_set_ringparam(struct net_device *dev, err = virtnet_tx_resize(vi, sq, ring->tx_pending); if (err) return err; + + /* Upon disabling and re-enabling a transmit virtqueue, the device must + * set the coalescing parameters of the virtqueue to those configured + * through the VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command, or, if the driver + * did not set any TX coalescing parameters, to 0. + */ + err = virtnet_send_ctrl_coal_vq_cmd(vi, txq2vq(i), + vi->intr_coal_tx.max_usecs, + vi->intr_coal_tx.max_packets); + if (err) + return err; + /* Save parameters */ + vi->sq[i].intr_coal.max_usecs = vi->intr_coal_tx.max_usecs; + vi->sq[i].intr_coal.max_packets = vi->intr_coal_tx.max_packets; } if (ring->rx_pending != rx_pending) { err = virtnet_rx_resize(vi, rq, ring->rx_pending); if (err) return err; + + /* The reason is same as the transmit virtqueue reset */ + err = virtnet_send_ctrl_coal_vq_cmd(vi, rxq2vq(i), + vi->intr_coal_rx.max_usecs, + vi->intr_coal_rx.max_packets); + if (err) + return err; + /* Save parameters */ + vi->rq[i].intr_coal.max_usecs = vi->intr_coal_rx.max_usecs; + vi->rq[i].intr_coal.max_packets = vi->intr_coal_rx.max_packets; } }
According to the definition of virtqueue coalescing spec[1]: Upon disabling and re-enabling a transmit virtqueue, the device MUST set the coalescing parameters of the virtqueue to those configured through the VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command, or, if the driver did not set any TX coalescing parameters, to 0. Upon disabling and re-enabling a receive virtqueue, the device MUST set the coalescing parameters of the virtqueue to those configured through the VIRTIO_NET_CTRL_NOTF_COAL_RX_SET command, or, if the driver did not set any RX coalescing parameters, to 0. We need to add this setting for vq resize (ethtool -G) where vq_reset happens. [1] https://lists.oasis-open.org/archives/virtio-dev/202303/msg00415.html Fixes: 394bd87764b6 ("virtio_net: support per queue interrupt coalesce command") Cc: Gavin Li <gavinl@nvidia.com> Signed-off-by: Heng Qi <hengqi@linux.alibaba.com> --- drivers/net/virtio_net.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)