| Message ID | 20240425125855.87025-2-hengqi@linux.alibaba.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | virtio_net: enable the irq for ctrlq | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/apply | fail | Patch does not apply to net-next-1 |
On Thu, Apr 25, 2024 at 8:59 PM Heng Qi <hengqi@linux.alibaba.com> wrote: > > Control vq polling request results consume more CPU. > Especially when dim issues more control requests to the device, > it's beneficial to the guest to enable control vq's irq. > > Suggested-by: Jason Wang <jasowang@redhat.com> > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com> > --- > drivers/net/virtio_net.c | 45 ++++++++++++++++++++++++++++++---------- > 1 file changed, 34 insertions(+), 11 deletions(-) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index a4d3c76654a4..79a1b30c173c 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -287,6 +287,12 @@ struct virtnet_info { > bool has_cvq; > struct mutex cvq_lock; > > + /* Wait for the device to complete the request */ > + struct completion completion; > + > + /* Work struct for acquisition of cvq processing results. */ > + struct work_struct get_cvq; > + > /* Host can handle any s/g split between our header and packet data */ > bool any_header_sg; > > @@ -520,6 +526,13 @@ static bool virtqueue_napi_complete(struct napi_struct *napi, > return false; > } > > +static void virtnet_cvq_done(struct virtqueue *cvq) > +{ > + struct virtnet_info *vi = cvq->vdev->priv; > + > + schedule_work(&vi->get_cvq); > +} > + > static void skb_xmit_done(struct virtqueue *vq) > { > struct virtnet_info *vi = vq->vdev->priv; > @@ -2036,6 +2049,20 @@ static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq, > return !oom; > } > > +static void virtnet_get_cvq_work(struct work_struct *work) > +{ > + struct virtnet_info *vi = > + container_of(work, struct virtnet_info, get_cvq); > + unsigned int tmp; > + void *res; > + > + mutex_lock(&vi->cvq_lock); > + res = virtqueue_get_buf(vi->cvq, &tmp); > + if (res) > + complete(&vi->completion); > + mutex_unlock(&vi->cvq_lock); > +} > + > static void skb_recv_done(struct virtqueue *rvq) > { > struct virtnet_info *vi = rvq->vdev->priv; > @@ -2531,7 +2558,7 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd, > struct scatterlist *out) > { > struct scatterlist *sgs[4], hdr, stat; > - unsigned out_num = 0, tmp; > + unsigned out_num = 0; > int ret; > > /* Caller should know better */ > @@ -2566,16 +2593,10 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd, > return vi->ctrl->status == VIRTIO_NET_OK; > } > > - /* Spin for a response, the kick causes an ioport write, trapping > - * into the hypervisor, so the request should be handled immediately. > - */ > - while (!virtqueue_get_buf(vi->cvq, &tmp) && > - !virtqueue_is_broken(vi->cvq)) { > - cond_resched(); > - cpu_relax(); > - } > - > mutex_unlock(&vi->cvq_lock); > + > + wait_for_completion(&vi->completion); > + A question here, can multiple cvq requests be submitted to the device? If yes, what happens if the device completes them out of order? Thanks > return vi->ctrl->status == VIRTIO_NET_OK; > } > > @@ -4433,7 +4454,7 @@ static int virtnet_find_vqs(struct virtnet_info *vi) > > /* Parameters for control virtqueue, if any */ > if (vi->has_cvq) { > - callbacks[total_vqs - 1] = NULL; > + callbacks[total_vqs - 1] = virtnet_cvq_done; > names[total_vqs - 1] = "control"; > } > > @@ -4952,6 +4973,8 @@ static int virtnet_probe(struct virtio_device *vdev) > if (vi->has_rss || vi->has_rss_hash_report) > virtnet_init_default_rss(vi); > > + INIT_WORK(&vi->get_cvq, virtnet_get_cvq_work); > + init_completion(&vi->completion); > enable_rx_mode_work(vi); > > /* serialize netdev register + virtio_device_ready() with ndo_open() */ > -- > 2.32.0.3.g01195cf9f >
On Tue, 7 May 2024 11:15:22 +0800, Jason Wang <jasowang@redhat.com> wrote: > On Thu, Apr 25, 2024 at 8:59 PM Heng Qi <hengqi@linux.alibaba.com> wrote: > > > > Control vq polling request results consume more CPU. > > Especially when dim issues more control requests to the device, > > it's beneficial to the guest to enable control vq's irq. > > > > Suggested-by: Jason Wang <jasowang@redhat.com> > > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com> > > --- > > drivers/net/virtio_net.c | 45 ++++++++++++++++++++++++++++++---------- > > 1 file changed, 34 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > index a4d3c76654a4..79a1b30c173c 100644 > > --- a/drivers/net/virtio_net.c > > +++ b/drivers/net/virtio_net.c > > @@ -287,6 +287,12 @@ struct virtnet_info { > > bool has_cvq; > > struct mutex cvq_lock; > > > > + /* Wait for the device to complete the request */ > > + struct completion completion; > > + > > + /* Work struct for acquisition of cvq processing results. */ > > + struct work_struct get_cvq; > > + > > /* Host can handle any s/g split between our header and packet data */ > > bool any_header_sg; > > > > @@ -520,6 +526,13 @@ static bool virtqueue_napi_complete(struct napi_struct *napi, > > return false; > > } > > > > +static void virtnet_cvq_done(struct virtqueue *cvq) > > +{ > > + struct virtnet_info *vi = cvq->vdev->priv; > > + > > + schedule_work(&vi->get_cvq); > > +} > > + > > static void skb_xmit_done(struct virtqueue *vq) > > { > > struct virtnet_info *vi = vq->vdev->priv; > > @@ -2036,6 +2049,20 @@ static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq, > > return !oom; > > } > > > > +static void virtnet_get_cvq_work(struct work_struct *work) > > +{ > > + struct virtnet_info *vi = > > + container_of(work, struct virtnet_info, get_cvq); > > + unsigned int tmp; > > + void *res; > > + > > + mutex_lock(&vi->cvq_lock); > > + res = virtqueue_get_buf(vi->cvq, &tmp); > > + if (res) > > + complete(&vi->completion); > > + mutex_unlock(&vi->cvq_lock); > > +} > > + > > static void skb_recv_done(struct virtqueue *rvq) > > { > > struct virtnet_info *vi = rvq->vdev->priv; > > @@ -2531,7 +2558,7 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd, > > struct scatterlist *out) > > { > > struct scatterlist *sgs[4], hdr, stat; > > - unsigned out_num = 0, tmp; > > + unsigned out_num = 0; > > int ret; > > > > /* Caller should know better */ > > @@ -2566,16 +2593,10 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd, > > return vi->ctrl->status == VIRTIO_NET_OK; > > } > > > > - /* Spin for a response, the kick causes an ioport write, trapping > > - * into the hypervisor, so the request should be handled immediately. > > - */ > > - while (!virtqueue_get_buf(vi->cvq, &tmp) && > > - !virtqueue_is_broken(vi->cvq)) { > > - cond_resched(); > > - cpu_relax(); > > - } > > - > > mutex_unlock(&vi->cvq_lock); > > + > > + wait_for_completion(&vi->completion); > > + > > A question here, can multiple cvq requests be submitted to the device? > If yes, what happens if the device completes them out of order? For user commands (such as ethtool cmds), multiple cvq requests is not allowed. because it holds the netlink lock when waiting for the response. For multiple dim commands and a user command allowed to be sent simultaneously , the corresponding command-specific information(desc_state) will be used to distinguish different responses. Thanks. > > Thanks > > > return vi->ctrl->status == VIRTIO_NET_OK; > > } > > > > @@ -4433,7 +4454,7 @@ static int virtnet_find_vqs(struct virtnet_info *vi) > > > > /* Parameters for control virtqueue, if any */ > > if (vi->has_cvq) { > > - callbacks[total_vqs - 1] = NULL; > > + callbacks[total_vqs - 1] = virtnet_cvq_done; > > names[total_vqs - 1] = "control"; > > } > > > > @@ -4952,6 +4973,8 @@ static int virtnet_probe(struct virtio_device *vdev) > > if (vi->has_rss || vi->has_rss_hash_report) > > virtnet_init_default_rss(vi); > > > > + INIT_WORK(&vi->get_cvq, virtnet_get_cvq_work); > > + init_completion(&vi->completion); > > enable_rx_mode_work(vi); > > > > /* serialize netdev register + virtio_device_ready() with ndo_open() */ > > -- > > 2.32.0.3.g01195cf9f > > >
On Tue, May 7, 2024 at 12:03 PM Heng Qi <hengqi@linux.alibaba.com> wrote: > > On Tue, 7 May 2024 11:15:22 +0800, Jason Wang <jasowang@redhat.com> wrote: > > On Thu, Apr 25, 2024 at 8:59 PM Heng Qi <hengqi@linux.alibaba.com> wrote: > > > > > > Control vq polling request results consume more CPU. > > > Especially when dim issues more control requests to the device, > > > it's beneficial to the guest to enable control vq's irq. > > > > > > Suggested-by: Jason Wang <jasowang@redhat.com> > > > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com> > > > --- > > > drivers/net/virtio_net.c | 45 ++++++++++++++++++++++++++++++---------- > > > 1 file changed, 34 insertions(+), 11 deletions(-) > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > > index a4d3c76654a4..79a1b30c173c 100644 > > > --- a/drivers/net/virtio_net.c > > > +++ b/drivers/net/virtio_net.c > > > @@ -287,6 +287,12 @@ struct virtnet_info { > > > bool has_cvq; > > > struct mutex cvq_lock; > > > > > > + /* Wait for the device to complete the request */ > > > + struct completion completion; > > > + > > > + /* Work struct for acquisition of cvq processing results. */ > > > + struct work_struct get_cvq; > > > + > > > /* Host can handle any s/g split between our header and packet data */ > > > bool any_header_sg; > > > > > > @@ -520,6 +526,13 @@ static bool virtqueue_napi_complete(struct napi_struct *napi, > > > return false; > > > } > > > > > > +static void virtnet_cvq_done(struct virtqueue *cvq) > > > +{ > > > + struct virtnet_info *vi = cvq->vdev->priv; > > > + > > > + schedule_work(&vi->get_cvq); > > > +} > > > + > > > static void skb_xmit_done(struct virtqueue *vq) > > > { > > > struct virtnet_info *vi = vq->vdev->priv; > > > @@ -2036,6 +2049,20 @@ static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq, > > > return !oom; > > > } > > > > > > +static void virtnet_get_cvq_work(struct work_struct *work) > > > +{ > > > + struct virtnet_info *vi = > > > + container_of(work, struct virtnet_info, get_cvq); > > > + unsigned int tmp; > > > + void *res; > > > + > > > + mutex_lock(&vi->cvq_lock); > > > + res = virtqueue_get_buf(vi->cvq, &tmp); > > > + if (res) > > > + complete(&vi->completion); > > > + mutex_unlock(&vi->cvq_lock); > > > +} > > > + > > > static void skb_recv_done(struct virtqueue *rvq) > > > { > > > struct virtnet_info *vi = rvq->vdev->priv; > > > @@ -2531,7 +2558,7 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd, > > > struct scatterlist *out) > > > { > > > struct scatterlist *sgs[4], hdr, stat; > > > - unsigned out_num = 0, tmp; > > > + unsigned out_num = 0; > > > int ret; > > > > > > /* Caller should know better */ > > > @@ -2566,16 +2593,10 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd, > > > return vi->ctrl->status == VIRTIO_NET_OK; > > > } > > > > > > - /* Spin for a response, the kick causes an ioport write, trapping > > > - * into the hypervisor, so the request should be handled immediately. > > > - */ > > > - while (!virtqueue_get_buf(vi->cvq, &tmp) && > > > - !virtqueue_is_broken(vi->cvq)) { > > > - cond_resched(); > > > - cpu_relax(); > > > - } > > > - > > > mutex_unlock(&vi->cvq_lock); > > > + > > > + wait_for_completion(&vi->completion); > > > + > > > > A question here, can multiple cvq requests be submitted to the device? > > If yes, what happens if the device completes them out of order? > > For user commands (such as ethtool cmds), multiple cvq requests is not allowed. > because it holds the netlink lock when waiting for the response. > > For multiple dim commands and a user command allowed to be sent simultaneously > , the corresponding command-specific information(desc_state) will be used to > distinguish different responses. Just to make sure we are on the same page. I meant at least we are still use the global completion which seems to be problematic. wait_for_completion(&vi->completion); Thanks > > Thanks. > > > > > Thanks > > > > > return vi->ctrl->status == VIRTIO_NET_OK; > > > } > > > > > > @@ -4433,7 +4454,7 @@ static int virtnet_find_vqs(struct virtnet_info *vi) > > > > > > /* Parameters for control virtqueue, if any */ > > > if (vi->has_cvq) { > > > - callbacks[total_vqs - 1] = NULL; > > > + callbacks[total_vqs - 1] = virtnet_cvq_done; > > > names[total_vqs - 1] = "control"; > > > } > > > > > > @@ -4952,6 +4973,8 @@ static int virtnet_probe(struct virtio_device *vdev) > > > if (vi->has_rss || vi->has_rss_hash_report) > > > virtnet_init_default_rss(vi); > > > > > > + INIT_WORK(&vi->get_cvq, virtnet_get_cvq_work); > > > + init_completion(&vi->completion); > > > enable_rx_mode_work(vi); > > > > > > /* serialize netdev register + virtio_device_ready() with ndo_open() */ > > > -- > > > 2.32.0.3.g01195cf9f > > > > > >
On Tue, 7 May 2024 14:24:16 +0800, Jason Wang <jasowang@redhat.com> wrote: > On Tue, May 7, 2024 at 12:03 PM Heng Qi <hengqi@linux.alibaba.com> wrote: > > > > On Tue, 7 May 2024 11:15:22 +0800, Jason Wang <jasowang@redhat.com> wrote: > > > On Thu, Apr 25, 2024 at 8:59 PM Heng Qi <hengqi@linux.alibaba.com> wrote: > > > > > > > > Control vq polling request results consume more CPU. > > > > Especially when dim issues more control requests to the device, > > > > it's beneficial to the guest to enable control vq's irq. > > > > > > > > Suggested-by: Jason Wang <jasowang@redhat.com> > > > > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com> > > > > --- > > > > drivers/net/virtio_net.c | 45 ++++++++++++++++++++++++++++++---------- > > > > 1 file changed, 34 insertions(+), 11 deletions(-) > > > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > > > index a4d3c76654a4..79a1b30c173c 100644 > > > > --- a/drivers/net/virtio_net.c > > > > +++ b/drivers/net/virtio_net.c > > > > @@ -287,6 +287,12 @@ struct virtnet_info { > > > > bool has_cvq; > > > > struct mutex cvq_lock; > > > > > > > > + /* Wait for the device to complete the request */ > > > > + struct completion completion; > > > > + > > > > + /* Work struct for acquisition of cvq processing results. */ > > > > + struct work_struct get_cvq; > > > > + > > > > /* Host can handle any s/g split between our header and packet data */ > > > > bool any_header_sg; > > > > > > > > @@ -520,6 +526,13 @@ static bool virtqueue_napi_complete(struct napi_struct *napi, > > > > return false; > > > > } > > > > > > > > +static void virtnet_cvq_done(struct virtqueue *cvq) > > > > +{ > > > > + struct virtnet_info *vi = cvq->vdev->priv; > > > > + > > > > + schedule_work(&vi->get_cvq); > > > > +} > > > > + > > > > static void skb_xmit_done(struct virtqueue *vq) > > > > { > > > > struct virtnet_info *vi = vq->vdev->priv; > > > > @@ -2036,6 +2049,20 @@ static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq, > > > > return !oom; > > > > } > > > > > > > > +static void virtnet_get_cvq_work(struct work_struct *work) > > > > +{ > > > > + struct virtnet_info *vi = > > > > + container_of(work, struct virtnet_info, get_cvq); > > > > + unsigned int tmp; > > > > + void *res; > > > > + > > > > + mutex_lock(&vi->cvq_lock); > > > > + res = virtqueue_get_buf(vi->cvq, &tmp); > > > > + if (res) > > > > + complete(&vi->completion); > > > > + mutex_unlock(&vi->cvq_lock); > > > > +} > > > > + > > > > static void skb_recv_done(struct virtqueue *rvq) > > > > { > > > > struct virtnet_info *vi = rvq->vdev->priv; > > > > @@ -2531,7 +2558,7 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd, > > > > struct scatterlist *out) > > > > { > > > > struct scatterlist *sgs[4], hdr, stat; > > > > - unsigned out_num = 0, tmp; > > > > + unsigned out_num = 0; > > > > int ret; > > > > > > > > /* Caller should know better */ > > > > @@ -2566,16 +2593,10 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd, > > > > return vi->ctrl->status == VIRTIO_NET_OK; > > > > } > > > > > > > > - /* Spin for a response, the kick causes an ioport write, trapping > > > > - * into the hypervisor, so the request should be handled immediately. > > > > - */ > > > > - while (!virtqueue_get_buf(vi->cvq, &tmp) && > > > > - !virtqueue_is_broken(vi->cvq)) { > > > > - cond_resched(); > > > > - cpu_relax(); > > > > - } > > > > - > > > > mutex_unlock(&vi->cvq_lock); > > > > + > > > > + wait_for_completion(&vi->completion); > > > > + > > > > > > A question here, can multiple cvq requests be submitted to the device? > > > If yes, what happens if the device completes them out of order? > > > > For user commands (such as ethtool cmds), multiple cvq requests is not allowed. > > because it holds the netlink lock when waiting for the response. > > > > For multiple dim commands and a user command allowed to be sent simultaneously > > , the corresponding command-specific information(desc_state) will be used to > > distinguish different responses. > > Just to make sure we are on the same page. I meant at least we are > still use the global completion which seems to be problematic. > > wait_for_completion(&vi->completion); This completion is only used by the ethtool command, so it is the global one. dim commands use specific coal_free_list and coal_wait_list to complete multiple command issuance (please see patch 3). Thanks. > > Thanks > > > > > > Thanks. > > > > > > > > Thanks > > > > > > > return vi->ctrl->status == VIRTIO_NET_OK; > > > > } > > > > > > > > @@ -4433,7 +4454,7 @@ static int virtnet_find_vqs(struct virtnet_info *vi) > > > > > > > > /* Parameters for control virtqueue, if any */ > > > > if (vi->has_cvq) { > > > > - callbacks[total_vqs - 1] = NULL; > > > > + callbacks[total_vqs - 1] = virtnet_cvq_done; > > > > names[total_vqs - 1] = "control"; > > > > } > > > > > > > > @@ -4952,6 +4973,8 @@ static int virtnet_probe(struct virtio_device *vdev) > > > > if (vi->has_rss || vi->has_rss_hash_report) > > > > virtnet_init_default_rss(vi); > > > > > > > > + INIT_WORK(&vi->get_cvq, virtnet_get_cvq_work); > > > > + init_completion(&vi->completion); > > > > enable_rx_mode_work(vi); > > > > > > > > /* serialize netdev register + virtio_device_ready() with ndo_open() */ > > > > -- > > > > 2.32.0.3.g01195cf9f > > > > > > > > > >
On Tue, May 7, 2024 at 2:33 PM Heng Qi <hengqi@linux.alibaba.com> wrote: > > On Tue, 7 May 2024 14:24:16 +0800, Jason Wang <jasowang@redhat.com> wrote: > > On Tue, May 7, 2024 at 12:03 PM Heng Qi <hengqi@linux.alibaba.com> wrote: > > > > > > On Tue, 7 May 2024 11:15:22 +0800, Jason Wang <jasowang@redhat.com> wrote: > > > > On Thu, Apr 25, 2024 at 8:59 PM Heng Qi <hengqi@linux.alibaba.com> wrote: > > > > > > > > > > Control vq polling request results consume more CPU. > > > > > Especially when dim issues more control requests to the device, > > > > > it's beneficial to the guest to enable control vq's irq. > > > > > > > > > > Suggested-by: Jason Wang <jasowang@redhat.com> > > > > > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com> > > > > > --- > > > > > drivers/net/virtio_net.c | 45 ++++++++++++++++++++++++++++++---------- > > > > > 1 file changed, 34 insertions(+), 11 deletions(-) > > > > > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > > > > index a4d3c76654a4..79a1b30c173c 100644 > > > > > --- a/drivers/net/virtio_net.c > > > > > +++ b/drivers/net/virtio_net.c > > > > > @@ -287,6 +287,12 @@ struct virtnet_info { > > > > > bool has_cvq; > > > > > struct mutex cvq_lock; > > > > > > > > > > + /* Wait for the device to complete the request */ > > > > > + struct completion completion; > > > > > + > > > > > + /* Work struct for acquisition of cvq processing results. */ > > > > > + struct work_struct get_cvq; > > > > > + > > > > > /* Host can handle any s/g split between our header and packet data */ > > > > > bool any_header_sg; > > > > > > > > > > @@ -520,6 +526,13 @@ static bool virtqueue_napi_complete(struct napi_struct *napi, > > > > > return false; > > > > > } > > > > > > > > > > +static void virtnet_cvq_done(struct virtqueue *cvq) > > > > > +{ > > > > > + struct virtnet_info *vi = cvq->vdev->priv; > > > > > + > > > > > + schedule_work(&vi->get_cvq); > > > > > +} > > > > > + > > > > > static void skb_xmit_done(struct virtqueue *vq) > > > > > { > > > > > struct virtnet_info *vi = vq->vdev->priv; > > > > > @@ -2036,6 +2049,20 @@ static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq, > > > > > return !oom; > > > > > } > > > > > > > > > > +static void virtnet_get_cvq_work(struct work_struct *work) > > > > > +{ > > > > > + struct virtnet_info *vi = > > > > > + container_of(work, struct virtnet_info, get_cvq); > > > > > + unsigned int tmp; > > > > > + void *res; > > > > > + > > > > > + mutex_lock(&vi->cvq_lock); > > > > > + res = virtqueue_get_buf(vi->cvq, &tmp); > > > > > + if (res) > > > > > + complete(&vi->completion); > > > > > + mutex_unlock(&vi->cvq_lock); > > > > > +} > > > > > + > > > > > static void skb_recv_done(struct virtqueue *rvq) > > > > > { > > > > > struct virtnet_info *vi = rvq->vdev->priv; > > > > > @@ -2531,7 +2558,7 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd, > > > > > struct scatterlist *out) > > > > > { > > > > > struct scatterlist *sgs[4], hdr, stat; > > > > > - unsigned out_num = 0, tmp; > > > > > + unsigned out_num = 0; > > > > > int ret; > > > > > > > > > > /* Caller should know better */ > > > > > @@ -2566,16 +2593,10 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd, > > > > > return vi->ctrl->status == VIRTIO_NET_OK; > > > > > } > > > > > > > > > > - /* Spin for a response, the kick causes an ioport write, trapping > > > > > - * into the hypervisor, so the request should be handled immediately. > > > > > - */ > > > > > - while (!virtqueue_get_buf(vi->cvq, &tmp) && > > > > > - !virtqueue_is_broken(vi->cvq)) { > > > > > - cond_resched(); > > > > > - cpu_relax(); > > > > > - } > > > > > - > > > > > mutex_unlock(&vi->cvq_lock); > > > > > + > > > > > + wait_for_completion(&vi->completion); > > > > > + > > > > > > > > A question here, can multiple cvq requests be submitted to the device? > > > > If yes, what happens if the device completes them out of order? > > > > > > For user commands (such as ethtool cmds), multiple cvq requests is not allowed. > > > because it holds the netlink lock when waiting for the response. > > > > > > For multiple dim commands and a user command allowed to be sent simultaneously > > > , the corresponding command-specific information(desc_state) will be used to > > > distinguish different responses. > > > > Just to make sure we are on the same page. I meant at least we are > > still use the global completion which seems to be problematic. > > > > wait_for_completion(&vi->completion); > > > This completion is only used by the ethtool command, so it is the global one. > > dim commands use specific coal_free_list and coal_wait_list to complete > multiple command issuance (please see patch 3). If I was not wrong, at least for this patch the global completion will be used by both dim and rtnl. If yes, it's not good to introduce a bug in patch 1 and fix it in patch 3. Thanks > > Thanks. > > > > > Thanks > > > > > > > > > > Thanks. > > > > > > > > > > > Thanks > > > > > > > > > return vi->ctrl->status == VIRTIO_NET_OK; > > > > > } > > > > > > > > > > @@ -4433,7 +4454,7 @@ static int virtnet_find_vqs(struct virtnet_info *vi) > > > > > > > > > > /* Parameters for control virtqueue, if any */ > > > > > if (vi->has_cvq) { > > > > > - callbacks[total_vqs - 1] = NULL; > > > > > + callbacks[total_vqs - 1] = virtnet_cvq_done; > > > > > names[total_vqs - 1] = "control"; > > > > > } > > > > > > > > > > @@ -4952,6 +4973,8 @@ static int virtnet_probe(struct virtio_device *vdev) > > > > > if (vi->has_rss || vi->has_rss_hash_report) > > > > > virtnet_init_default_rss(vi); > > > > > > > > > > + INIT_WORK(&vi->get_cvq, virtnet_get_cvq_work); > > > > > + init_completion(&vi->completion); > > > > > enable_rx_mode_work(vi); > > > > > > > > > > /* serialize netdev register + virtio_device_ready() with ndo_open() */ > > > > > -- > > > > > 2.32.0.3.g01195cf9f > > > > > > > > > > > > > > >
On Wed, 8 May 2024 10:19:06 +0800, Jason Wang <jasowang@redhat.com> wrote: > On Tue, May 7, 2024 at 2:33 PM Heng Qi <hengqi@linux.alibaba.com> wrote: > > > > On Tue, 7 May 2024 14:24:16 +0800, Jason Wang <jasowang@redhat.com> wrote: > > > On Tue, May 7, 2024 at 12:03 PM Heng Qi <hengqi@linux.alibaba.com> wrote: > > > > > > > > On Tue, 7 May 2024 11:15:22 +0800, Jason Wang <jasowang@redhat.com> wrote: > > > > > On Thu, Apr 25, 2024 at 8:59 PM Heng Qi <hengqi@linux.alibaba.com> wrote: > > > > > > > > > > > > Control vq polling request results consume more CPU. > > > > > > Especially when dim issues more control requests to the device, > > > > > > it's beneficial to the guest to enable control vq's irq. > > > > > > > > > > > > Suggested-by: Jason Wang <jasowang@redhat.com> > > > > > > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com> > > > > > > --- > > > > > > drivers/net/virtio_net.c | 45 ++++++++++++++++++++++++++++++---------- > > > > > > 1 file changed, 34 insertions(+), 11 deletions(-) > > > > > > > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > > > > > index a4d3c76654a4..79a1b30c173c 100644 > > > > > > --- a/drivers/net/virtio_net.c > > > > > > +++ b/drivers/net/virtio_net.c > > > > > > @@ -287,6 +287,12 @@ struct virtnet_info { > > > > > > bool has_cvq; > > > > > > struct mutex cvq_lock; > > > > > > > > > > > > + /* Wait for the device to complete the request */ > > > > > > + struct completion completion; > > > > > > + > > > > > > + /* Work struct for acquisition of cvq processing results. */ > > > > > > + struct work_struct get_cvq; > > > > > > + > > > > > > /* Host can handle any s/g split between our header and packet data */ > > > > > > bool any_header_sg; > > > > > > > > > > > > @@ -520,6 +526,13 @@ static bool virtqueue_napi_complete(struct napi_struct *napi, > > > > > > return false; > > > > > > } > > > > > > > > > > > > +static void virtnet_cvq_done(struct virtqueue *cvq) > > > > > > +{ > > > > > > + struct virtnet_info *vi = cvq->vdev->priv; > > > > > > + > > > > > > + schedule_work(&vi->get_cvq); > > > > > > +} > > > > > > + > > > > > > static void skb_xmit_done(struct virtqueue *vq) > > > > > > { > > > > > > struct virtnet_info *vi = vq->vdev->priv; > > > > > > @@ -2036,6 +2049,20 @@ static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq, > > > > > > return !oom; > > > > > > } > > > > > > > > > > > > +static void virtnet_get_cvq_work(struct work_struct *work) > > > > > > +{ > > > > > > + struct virtnet_info *vi = > > > > > > + container_of(work, struct virtnet_info, get_cvq); > > > > > > + unsigned int tmp; > > > > > > + void *res; > > > > > > + > > > > > > + mutex_lock(&vi->cvq_lock); > > > > > > + res = virtqueue_get_buf(vi->cvq, &tmp); > > > > > > + if (res) > > > > > > + complete(&vi->completion); > > > > > > + mutex_unlock(&vi->cvq_lock); > > > > > > +} > > > > > > + > > > > > > static void skb_recv_done(struct virtqueue *rvq) > > > > > > { > > > > > > struct virtnet_info *vi = rvq->vdev->priv; > > > > > > @@ -2531,7 +2558,7 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd, > > > > > > struct scatterlist *out) > > > > > > { > > > > > > struct scatterlist *sgs[4], hdr, stat; > > > > > > - unsigned out_num = 0, tmp; > > > > > > + unsigned out_num = 0; > > > > > > int ret; > > > > > > > > > > > > /* Caller should know better */ > > > > > > @@ -2566,16 +2593,10 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd, > > > > > > return vi->ctrl->status == VIRTIO_NET_OK; > > > > > > } > > > > > > > > > > > > - /* Spin for a response, the kick causes an ioport write, trapping > > > > > > - * into the hypervisor, so the request should be handled immediately. > > > > > > - */ > > > > > > - while (!virtqueue_get_buf(vi->cvq, &tmp) && > > > > > > - !virtqueue_is_broken(vi->cvq)) { > > > > > > - cond_resched(); > > > > > > - cpu_relax(); > > > > > > - } > > > > > > - > > > > > > mutex_unlock(&vi->cvq_lock); > > > > > > + > > > > > > + wait_for_completion(&vi->completion); > > > > > > + > > > > > > > > > > A question here, can multiple cvq requests be submitted to the device? > > > > > If yes, what happens if the device completes them out of order? > > > > > > > > For user commands (such as ethtool cmds), multiple cvq requests is not allowed. > > > > because it holds the netlink lock when waiting for the response. > > > > > > > > For multiple dim commands and a user command allowed to be sent simultaneously > > > > , the corresponding command-specific information(desc_state) will be used to > > > > distinguish different responses. > > > > > > Just to make sure we are on the same page. I meant at least we are > > > still use the global completion which seems to be problematic. > > > > > > wait_for_completion(&vi->completion); > > > > > > This completion is only used by the ethtool command, so it is the global one. > > > > dim commands use specific coal_free_list and coal_wait_list to complete > > multiple command issuance (please see patch 3). > > If I was not wrong, at least for this patch the global completion will > be used by both dim and rtnl. If yes, it's not good to introduce a bug > in patch 1 and fix it in patch 3. Ok, patches 1 and 3 will be refactored. Thanks. > > Thanks > > > > > Thanks. > > > > > > > > Thanks > > > > > > > > > > > > > > Thanks. > > > > > > > > > > > > > > Thanks > > > > > > > > > > > return vi->ctrl->status == VIRTIO_NET_OK; > > > > > > } > > > > > > > > > > > > @@ -4433,7 +4454,7 @@ static int virtnet_find_vqs(struct virtnet_info *vi) > > > > > > > > > > > > /* Parameters for control virtqueue, if any */ > > > > > > if (vi->has_cvq) { > > > > > > - callbacks[total_vqs - 1] = NULL; > > > > > > + callbacks[total_vqs - 1] = virtnet_cvq_done; > > > > > > names[total_vqs - 1] = "control"; > > > > > > } > > > > > > > > > > > > @@ -4952,6 +4973,8 @@ static int virtnet_probe(struct virtio_device *vdev) > > > > > > if (vi->has_rss || vi->has_rss_hash_report) > > > > > > virtnet_init_default_rss(vi); > > > > > > > > > > > > + INIT_WORK(&vi->get_cvq, virtnet_get_cvq_work); > > > > > > + init_completion(&vi->completion); > > > > > > enable_rx_mode_work(vi); > > > > > > > > > > > > /* serialize netdev register + virtio_device_ready() with ndo_open() */ > > > > > > -- > > > > > > 2.32.0.3.g01195cf9f > > > > > > > > > > > > > > > > > > > > >
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index a4d3c76654a4..79a1b30c173c 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -287,6 +287,12 @@ struct virtnet_info { bool has_cvq; struct mutex cvq_lock; + /* Wait for the device to complete the request */ + struct completion completion; + + /* Work struct for acquisition of cvq processing results. */ + struct work_struct get_cvq; + /* Host can handle any s/g split between our header and packet data */ bool any_header_sg; @@ -520,6 +526,13 @@ static bool virtqueue_napi_complete(struct napi_struct *napi, return false; } +static void virtnet_cvq_done(struct virtqueue *cvq) +{ + struct virtnet_info *vi = cvq->vdev->priv; + + schedule_work(&vi->get_cvq); +} + static void skb_xmit_done(struct virtqueue *vq) { struct virtnet_info *vi = vq->vdev->priv; @@ -2036,6 +2049,20 @@ static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq, return !oom; } +static void virtnet_get_cvq_work(struct work_struct *work) +{ + struct virtnet_info *vi = + container_of(work, struct virtnet_info, get_cvq); + unsigned int tmp; + void *res; + + mutex_lock(&vi->cvq_lock); + res = virtqueue_get_buf(vi->cvq, &tmp); + if (res) + complete(&vi->completion); + mutex_unlock(&vi->cvq_lock); +} + static void skb_recv_done(struct virtqueue *rvq) { struct virtnet_info *vi = rvq->vdev->priv; @@ -2531,7 +2558,7 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd, struct scatterlist *out) { struct scatterlist *sgs[4], hdr, stat; - unsigned out_num = 0, tmp; + unsigned out_num = 0; int ret; /* Caller should know better */ @@ -2566,16 +2593,10 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd, return vi->ctrl->status == VIRTIO_NET_OK; } - /* Spin for a response, the kick causes an ioport write, trapping - * into the hypervisor, so the request should be handled immediately. - */ - while (!virtqueue_get_buf(vi->cvq, &tmp) && - !virtqueue_is_broken(vi->cvq)) { - cond_resched(); - cpu_relax(); - } - mutex_unlock(&vi->cvq_lock); + + wait_for_completion(&vi->completion); + return vi->ctrl->status == VIRTIO_NET_OK; } @@ -4433,7 +4454,7 @@ static int virtnet_find_vqs(struct virtnet_info *vi) /* Parameters for control virtqueue, if any */ if (vi->has_cvq) { - callbacks[total_vqs - 1] = NULL; + callbacks[total_vqs - 1] = virtnet_cvq_done; names[total_vqs - 1] = "control"; } @@ -4952,6 +4973,8 @@ static int virtnet_probe(struct virtio_device *vdev) if (vi->has_rss || vi->has_rss_hash_report) virtnet_init_default_rss(vi); + INIT_WORK(&vi->get_cvq, virtnet_get_cvq_work); + init_completion(&vi->completion); enable_rx_mode_work(vi); /* serialize netdev register + virtio_device_ready() with ndo_open() */
Control vq polling request results consume more CPU. Especially when dim issues more control requests to the device, it's beneficial to the guest to enable control vq's irq. Suggested-by: Jason Wang <jasowang@redhat.com> Signed-off-by: Heng Qi <hengqi@linux.alibaba.com> --- drivers/net/virtio_net.c | 45 ++++++++++++++++++++++++++++++---------- 1 file changed, 34 insertions(+), 11 deletions(-)