diff mbox series

[v4,4/8] vdpa: Avoid using vhost_vdpa_net_load_*() outside vhost_vdpa_net_load()

Message ID a56d91c3cc2ab46f9be1770074c920f5375ddb5e.1693287885.git.yin31149@gmail.com
State New
Headers show
Series [v4,1/8] vhost: Add count argument to vhost_svq_poll() | expand

Commit Message

Hawkins Jiawei Aug. 29, 2023, 5:54 a.m. UTC 
Next patches in this series will refactor vhost_vdpa_net_load_cmd()
to iterate through the control commands shadow buffers, allowing QEMU
to send CVQ state load commands in parallel at device startup.

Considering that QEMU always forwards the CVQ command serialized
outside of vhost_vdpa_net_load(), it is more elegant to send the
CVQ commands directly without invoking vhost_vdpa_net_load_*() helpers.

Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
---
v4:
  - pack CVQ command by iov_from_buf() instead of accessing
`out` directly suggested by Eugenio

v3: https://lore.kernel.org/all/428a8fac2a29b37757fa15ca747be93c0226cb1f.1689748694.git.yin31149@gmail.com/

 net/vhost-vdpa.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

Comments

Eugenio Pérez Oct. 3, 2023, 5:48 p.m. UTC | #1 
On Tue, Aug 29, 2023 at 7:55 AM Hawkins Jiawei <yin31149@gmail.com> wrote:
>
> Next patches in this series will refactor vhost_vdpa_net_load_cmd()
> to iterate through the control commands shadow buffers, allowing QEMU
> to send CVQ state load commands in parallel at device startup.
>
> Considering that QEMU always forwards the CVQ command serialized
> outside of vhost_vdpa_net_load(), it is more elegant to send the
> CVQ commands directly without invoking vhost_vdpa_net_load_*() helpers.
>
> Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
> ---
> v4:
>   - pack CVQ command by iov_from_buf() instead of accessing
> `out` directly suggested by Eugenio
>
> v3: https://lore.kernel.org/all/428a8fac2a29b37757fa15ca747be93c0226cb1f.1689748694.git.yin31149@gmail.com/
>
>  net/vhost-vdpa.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index e6342b213f..7c67063469 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -1097,12 +1097,14 @@ static NetClientInfo net_vhost_vdpa_cvq_info = {
>   */
>  static int vhost_vdpa_net_excessive_mac_filter_cvq_add(VhostVDPAState *s,
>                                                         VirtQueueElement *elem,
> -                                                       struct iovec *out)
> +                                                       struct iovec *out,
> +                                                       const struct iovec *in)
>  {
>      struct virtio_net_ctrl_mac mac_data, *mac_ptr;
>      struct virtio_net_ctrl_hdr *hdr_ptr;
>      uint32_t cursor;
>      ssize_t r;
> +    uint8_t on = 1;
>
>      /* parse the non-multicast MAC address entries from CVQ command */
>      cursor = sizeof(*hdr_ptr);
> @@ -1150,7 +1152,15 @@ static int vhost_vdpa_net_excessive_mac_filter_cvq_add(VhostVDPAState *s,
>       * filter table to the vdpa device, it should send the
>       * VIRTIO_NET_CTRL_RX_PROMISC CVQ command to enable promiscuous mode
>       */
> -    r = vhost_vdpa_net_load_rx_mode(s, VIRTIO_NET_CTRL_RX_PROMISC, 1);
> +    cursor = 0;

I think this is redundant, as "cursor" is not used by the new code and
it is already set to 0 before its usage, isn't it?

> +    hdr_ptr = out->iov_base;
> +    out->iov_len = sizeof(*hdr_ptr) + sizeof(on);
> +    assert(out->iov_len < vhost_vdpa_net_cvq_cmd_page_len());
> +

I think we can assume this assertion is never hit, as out->iov_len is
controlled by this function.

Apart from these nits,

Acked-by: Eugenio Pérez <eperezma@redhat.com>

> +    hdr_ptr->class = VIRTIO_NET_CTRL_RX;
> +    hdr_ptr->cmd = VIRTIO_NET_CTRL_RX_PROMISC;
> +    iov_from_buf(out, 1, sizeof(*hdr_ptr), &on, sizeof(on));
> +    r = vhost_vdpa_net_cvq_add(s, out, 1, in, 1);
>      if (unlikely(r < 0)) {
>          return r;
>      }
> @@ -1268,7 +1278,7 @@ static int vhost_vdpa_net_handle_ctrl_avail(VhostShadowVirtqueue *svq,
>           * the CVQ command direclty.
>           */
>          dev_written = vhost_vdpa_net_excessive_mac_filter_cvq_add(s, elem,
> -                                                                  &out);
> +                                                            &out, &vdpa_in);
>          if (unlikely(dev_written < 0)) {
>              goto out;
>          }
> --
> 2.25.1
>
Hawkins Jiawei Oct. 8, 2023, 1:38 a.m. UTC | #2 
在 2023/10/4 01:48, Eugenio Perez Martin 写道:
> On Tue, Aug 29, 2023 at 7:55 AM Hawkins Jiawei <yin31149@gmail.com> wrote:
>>
>> Next patches in this series will refactor vhost_vdpa_net_load_cmd()
>> to iterate through the control commands shadow buffers, allowing QEMU
>> to send CVQ state load commands in parallel at device startup.
>>
>> Considering that QEMU always forwards the CVQ command serialized
>> outside of vhost_vdpa_net_load(), it is more elegant to send the
>> CVQ commands directly without invoking vhost_vdpa_net_load_*() helpers.
>>
>> Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
>> ---
>> v4:
>>    - pack CVQ command by iov_from_buf() instead of accessing
>> `out` directly suggested by Eugenio
>>
>> v3: https://lore.kernel.org/all/428a8fac2a29b37757fa15ca747be93c0226cb1f.1689748694.git.yin31149@gmail.com/
>>
>>   net/vhost-vdpa.c | 16 +++++++++++++---
>>   1 file changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
>> index e6342b213f..7c67063469 100644
>> --- a/net/vhost-vdpa.c
>> +++ b/net/vhost-vdpa.c
>> @@ -1097,12 +1097,14 @@ static NetClientInfo net_vhost_vdpa_cvq_info = {
>>    */
>>   static int vhost_vdpa_net_excessive_mac_filter_cvq_add(VhostVDPAState *s,
>>                                                          VirtQueueElement *elem,
>> -                                                       struct iovec *out)
>> +                                                       struct iovec *out,
>> +                                                       const struct iovec *in)
>>   {
>>       struct virtio_net_ctrl_mac mac_data, *mac_ptr;
>>       struct virtio_net_ctrl_hdr *hdr_ptr;
>>       uint32_t cursor;
>>       ssize_t r;
>> +    uint8_t on = 1;
>>
>>       /* parse the non-multicast MAC address entries from CVQ command */
>>       cursor = sizeof(*hdr_ptr);
>> @@ -1150,7 +1152,15 @@ static int vhost_vdpa_net_excessive_mac_filter_cvq_add(VhostVDPAState *s,
>>        * filter table to the vdpa device, it should send the
>>        * VIRTIO_NET_CTRL_RX_PROMISC CVQ command to enable promiscuous mode
>>        */
>> -    r = vhost_vdpa_net_load_rx_mode(s, VIRTIO_NET_CTRL_RX_PROMISC, 1);
>> +    cursor = 0;
>
> I think this is redundant, as "cursor" is not used by the new code and
> it is already set to 0 before its usage, isn't it?
>

You are right, I will remove this code in the v5 patch.

>> +    hdr_ptr = out->iov_base;
>> +    out->iov_len = sizeof(*hdr_ptr) + sizeof(on);
>> +    assert(out->iov_len < vhost_vdpa_net_cvq_cmd_page_len());
>> +
>
> I think we can assume this assertion is never hit, as out->iov_len is
> controlled by this function.
>

Thanks for your suggestion, I will remove this assertion.

Thanks!


> Apart from these nits,
>
> Acked-by: Eugenio Pérez <eperezma@redhat.com>
>
>> +    hdr_ptr->class = VIRTIO_NET_CTRL_RX;
>> +    hdr_ptr->cmd = VIRTIO_NET_CTRL_RX_PROMISC;
>> +    iov_from_buf(out, 1, sizeof(*hdr_ptr), &on, sizeof(on));
>> +    r = vhost_vdpa_net_cvq_add(s, out, 1, in, 1);
>>       if (unlikely(r < 0)) {
>>           return r;
>>       }
>> @@ -1268,7 +1278,7 @@ static int vhost_vdpa_net_handle_ctrl_avail(VhostShadowVirtqueue *svq,
>>            * the CVQ command direclty.
>>            */
>>           dev_written = vhost_vdpa_net_excessive_mac_filter_cvq_add(s, elem,
>> -                                                                  &out);
>> +                                                            &out, &vdpa_in);
>>           if (unlikely(dev_written < 0)) {
>>               goto out;
>>           }
>> --
>> 2.25.1
>>
>
diff mbox series

Patch

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index e6342b213f..7c67063469 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -1097,12 +1097,14 @@  static NetClientInfo net_vhost_vdpa_cvq_info = {
  */
 static int vhost_vdpa_net_excessive_mac_filter_cvq_add(VhostVDPAState *s,
                                                        VirtQueueElement *elem,
-                                                       struct iovec *out)
+                                                       struct iovec *out,
+                                                       const struct iovec *in)
 {
     struct virtio_net_ctrl_mac mac_data, *mac_ptr;
     struct virtio_net_ctrl_hdr *hdr_ptr;
     uint32_t cursor;
     ssize_t r;
+    uint8_t on = 1;
 
     /* parse the non-multicast MAC address entries from CVQ command */
     cursor = sizeof(*hdr_ptr);
@@ -1150,7 +1152,15 @@  static int vhost_vdpa_net_excessive_mac_filter_cvq_add(VhostVDPAState *s,
      * filter table to the vdpa device, it should send the
      * VIRTIO_NET_CTRL_RX_PROMISC CVQ command to enable promiscuous mode
      */
-    r = vhost_vdpa_net_load_rx_mode(s, VIRTIO_NET_CTRL_RX_PROMISC, 1);
+    cursor = 0;
+    hdr_ptr = out->iov_base;
+    out->iov_len = sizeof(*hdr_ptr) + sizeof(on);
+    assert(out->iov_len < vhost_vdpa_net_cvq_cmd_page_len());
+
+    hdr_ptr->class = VIRTIO_NET_CTRL_RX;
+    hdr_ptr->cmd = VIRTIO_NET_CTRL_RX_PROMISC;
+    iov_from_buf(out, 1, sizeof(*hdr_ptr), &on, sizeof(on));
+    r = vhost_vdpa_net_cvq_add(s, out, 1, in, 1);
     if (unlikely(r < 0)) {
         return r;
     }
@@ -1268,7 +1278,7 @@  static int vhost_vdpa_net_handle_ctrl_avail(VhostShadowVirtqueue *svq,
          * the CVQ command direclty.
          */
         dev_written = vhost_vdpa_net_excessive_mac_filter_cvq_add(s, elem,
-                                                                  &out);
+                                                            &out, &vdpa_in);
         if (unlikely(dev_written < 0)) {
             goto out;
         }