diff mbox series

[v2,3/4] vdpa: Restore vlan filtering state

Message ID e76a29f77bb3f386e4a643c8af94b77b775d1752.1690100802.git.yin31149@gmail.com
State New
Headers show
Series Vhost-vdpa Shadow Virtqueue VLAN support | expand

Commit Message

Hawkins Jiawei July 23, 2023, 9:26 a.m. UTC 
This patch introduces vhost_vdpa_net_load_single_vlan()
and vhost_vdpa_net_load_vlan() to restore the vlan
filtering state at device's startup.

Co-developed-by: Eugenio Pérez <eperezma@redhat.com>
Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
---
v2:
 - remove the extra line pointed out by Eugenio

v1: https://lore.kernel.org/all/0a568cc8a8d2b750c2e09b2237e9f05cece07c3f.1689690854.git.yin31149@gmail.com/

 net/vhost-vdpa.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

Comments

Jason Wang July 25, 2023, 6:47 a.m. UTC | #1 
On Sun, Jul 23, 2023 at 5:28 PM Hawkins Jiawei <yin31149@gmail.com> wrote:
>
> This patch introduces vhost_vdpa_net_load_single_vlan()
> and vhost_vdpa_net_load_vlan() to restore the vlan
> filtering state at device's startup.
>
> Co-developed-by: Eugenio Pérez <eperezma@redhat.com>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>

Acked-by: Jason Wang <jasowang@redhat.com>

But this seems to be a source of latency killer as it may at most send
1024 commands.

As discussed in the past, we need a better cvq command to do this: for
example, a single command to carray a bitmap.

Thanks

> ---
> v2:
>  - remove the extra line pointed out by Eugenio
>
> v1: https://lore.kernel.org/all/0a568cc8a8d2b750c2e09b2237e9f05cece07c3f.1689690854.git.yin31149@gmail.com/
>
>  net/vhost-vdpa.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
>
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index 9795306742..347241796d 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -965,6 +965,50 @@ static int vhost_vdpa_net_load_rx(VhostVDPAState *s,
>      return 0;
>  }
>
> +static int vhost_vdpa_net_load_single_vlan(VhostVDPAState *s,
> +                                           const VirtIONet *n,
> +                                           uint16_t vid)
> +{
> +    const struct iovec data = {
> +        .iov_base = &vid,
> +        .iov_len = sizeof(vid),
> +    };
> +    ssize_t dev_written = vhost_vdpa_net_load_cmd(s, VIRTIO_NET_CTRL_VLAN,
> +                                                  VIRTIO_NET_CTRL_VLAN_ADD,
> +                                                  &data, 1);
> +    if (unlikely(dev_written < 0)) {
> +        return dev_written;
> +    }
> +    if (unlikely(*s->status != VIRTIO_NET_OK)) {
> +        return -EIO;
> +    }
> +
> +    return 0;
> +}
> +
> +static int vhost_vdpa_net_load_vlan(VhostVDPAState *s,
> +                                    const VirtIONet *n)
> +{
> +    int r;
> +
> +    if (!virtio_vdev_has_feature(&n->parent_obj, VIRTIO_NET_F_CTRL_VLAN)) {
> +        return 0;
> +    }
> +
> +    for (int i = 0; i < MAX_VLAN >> 5; i++) {
> +        for (int j = 0; n->vlans[i] && j <= 0x1f; j++) {
> +            if (n->vlans[i] & (1U << j)) {
> +                r = vhost_vdpa_net_load_single_vlan(s, n, (i << 5) + j);
> +                if (unlikely(r != 0)) {
> +                    return r;
> +                }
> +            }
> +        }
> +    }
> +
> +    return 0;
> +}
> +
>  static int vhost_vdpa_net_load(NetClientState *nc)
>  {
>      VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
> @@ -995,6 +1039,10 @@ static int vhost_vdpa_net_load(NetClientState *nc)
>      if (unlikely(r)) {
>          return r;
>      }
> +    r = vhost_vdpa_net_load_vlan(s, n);
> +    if (unlikely(r)) {
> +        return r;
> +    }
>
>      return 0;
>  }
> --
> 2.25.1
>
Hawkins Jiawei July 25, 2023, 7:48 a.m. UTC | #2 
On 2023/7/25 14:47, Jason Wang wrote:
> On Sun, Jul 23, 2023 at 5:28 PM Hawkins Jiawei <yin31149@gmail.com> wrote:
>>
>> This patch introduces vhost_vdpa_net_load_single_vlan()
>> and vhost_vdpa_net_load_vlan() to restore the vlan
>> filtering state at device's startup.
>>
>> Co-developed-by: Eugenio Pérez <eperezma@redhat.com>
>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>> Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
>
> Acked-by: Jason Wang <jasowang@redhat.com>
>
> But this seems to be a source of latency killer as it may at most send
> 1024 commands.
>
> As discussed in the past, we need a better cvq command to do this: for
> example, a single command to carray a bitmap.

Hi Jason,

Thanks for your review.

You are right, we need some improvement here.

Therefore, I have submitted another patch series titled "vdpa: Send all
CVQ state load commands in parallel" at [1], which allows QEMU to delay
polling and checking the device used buffer until either the SVQ is full
or control commands shadow buffers are full, so that QEMU can send all
the SVQ control commands in parallel, which has better performance
improvement.

To test that patch series, I created 4094 VLANS in guest to build an
environment for sending multiple CVQ state load commands. According to
the result on the real vdpa device at [2], this patch series can improve
latency from 23296 us to 6539 us.

Thanks!

[1]. https://lists.gnu.org/archive/html/qemu-devel/2023-07/msg03726.html
[2]. https://lists.gnu.org/archive/html/qemu-devel/2023-07/msg03947.html


>
> Thanks
>
>> ---
>> v2:
>>   - remove the extra line pointed out by Eugenio
>>
>> v1: https://lore.kernel.org/all/0a568cc8a8d2b750c2e09b2237e9f05cece07c3f.1689690854.git.yin31149@gmail.com/
>>
>>   net/vhost-vdpa.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 48 insertions(+)
>>
>> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
>> index 9795306742..347241796d 100644
>> --- a/net/vhost-vdpa.c
>> +++ b/net/vhost-vdpa.c
>> @@ -965,6 +965,50 @@ static int vhost_vdpa_net_load_rx(VhostVDPAState *s,
>>       return 0;
>>   }
>>
>> +static int vhost_vdpa_net_load_single_vlan(VhostVDPAState *s,
>> +                                           const VirtIONet *n,
>> +                                           uint16_t vid)
>> +{
>> +    const struct iovec data = {
>> +        .iov_base = &vid,
>> +        .iov_len = sizeof(vid),
>> +    };
>> +    ssize_t dev_written = vhost_vdpa_net_load_cmd(s, VIRTIO_NET_CTRL_VLAN,
>> +                                                  VIRTIO_NET_CTRL_VLAN_ADD,
>> +                                                  &data, 1);
>> +    if (unlikely(dev_written < 0)) {
>> +        return dev_written;
>> +    }
>> +    if (unlikely(*s->status != VIRTIO_NET_OK)) {
>> +        return -EIO;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static int vhost_vdpa_net_load_vlan(VhostVDPAState *s,
>> +                                    const VirtIONet *n)
>> +{
>> +    int r;
>> +
>> +    if (!virtio_vdev_has_feature(&n->parent_obj, VIRTIO_NET_F_CTRL_VLAN)) {
>> +        return 0;
>> +    }
>> +
>> +    for (int i = 0; i < MAX_VLAN >> 5; i++) {
>> +        for (int j = 0; n->vlans[i] && j <= 0x1f; j++) {
>> +            if (n->vlans[i] & (1U << j)) {
>> +                r = vhost_vdpa_net_load_single_vlan(s, n, (i << 5) + j);
>> +                if (unlikely(r != 0)) {
>> +                    return r;
>> +                }
>> +            }
>> +        }
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>   static int vhost_vdpa_net_load(NetClientState *nc)
>>   {
>>       VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
>> @@ -995,6 +1039,10 @@ static int vhost_vdpa_net_load(NetClientState *nc)
>>       if (unlikely(r)) {
>>           return r;
>>       }
>> +    r = vhost_vdpa_net_load_vlan(s, n);
>> +    if (unlikely(r)) {
>> +        return r;
>> +    }
>>
>>       return 0;
>>   }
>> --
>> 2.25.1
>>
>
Jason Wang July 26, 2023, 2:18 a.m. UTC | #3 
On Tue, Jul 25, 2023 at 3:48 PM Hawkins Jiawei <yin31149@gmail.com> wrote:
>
> On 2023/7/25 14:47, Jason Wang wrote:
> > On Sun, Jul 23, 2023 at 5:28 PM Hawkins Jiawei <yin31149@gmail.com> wrote:
> >>
> >> This patch introduces vhost_vdpa_net_load_single_vlan()
> >> and vhost_vdpa_net_load_vlan() to restore the vlan
> >> filtering state at device's startup.
> >>
> >> Co-developed-by: Eugenio Pérez <eperezma@redhat.com>
> >> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> >> Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
> >
> > Acked-by: Jason Wang <jasowang@redhat.com>
> >
> > But this seems to be a source of latency killer as it may at most send
> > 1024 commands.
> >
> > As discussed in the past, we need a better cvq command to do this: for
> > example, a single command to carray a bitmap.
>
> Hi Jason,
>
> Thanks for your review.
>
> You are right, we need some improvement here.
>
> Therefore, I have submitted another patch series titled "vdpa: Send all
> CVQ state load commands in parallel" at [1], which allows QEMU to delay
> polling and checking the device used buffer until either the SVQ is full
> or control commands shadow buffers are full, so that QEMU can send all
> the SVQ control commands in parallel, which has better performance
> improvement.
>
> To test that patch series, I created 4094 VLANS in guest to build an
> environment for sending multiple CVQ state load commands. According to
> the result on the real vdpa device at [2], this patch series can improve
> latency from 23296 us to 6539 us.
>
> Thanks!
>
> [1]. https://lists.gnu.org/archive/html/qemu-devel/2023-07/msg03726.html
> [2]. https://lists.gnu.org/archive/html/qemu-devel/2023-07/msg03947.html
>

That's great, and if we can use a single command to get/set vid it
would be still helpful (I meant we can extend the virtio spec).

Thanks

>
> >
> > Thanks
> >
> >> ---
> >> v2:
> >>   - remove the extra line pointed out by Eugenio
> >>
> >> v1: https://lore.kernel.org/all/0a568cc8a8d2b750c2e09b2237e9f05cece07c3f.1689690854.git.yin31149@gmail.com/
> >>
> >>   net/vhost-vdpa.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
> >>   1 file changed, 48 insertions(+)
> >>
> >> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> >> index 9795306742..347241796d 100644
> >> --- a/net/vhost-vdpa.c
> >> +++ b/net/vhost-vdpa.c
> >> @@ -965,6 +965,50 @@ static int vhost_vdpa_net_load_rx(VhostVDPAState *s,
> >>       return 0;
> >>   }
> >>
> >> +static int vhost_vdpa_net_load_single_vlan(VhostVDPAState *s,
> >> +                                           const VirtIONet *n,
> >> +                                           uint16_t vid)
> >> +{
> >> +    const struct iovec data = {
> >> +        .iov_base = &vid,
> >> +        .iov_len = sizeof(vid),
> >> +    };
> >> +    ssize_t dev_written = vhost_vdpa_net_load_cmd(s, VIRTIO_NET_CTRL_VLAN,
> >> +                                                  VIRTIO_NET_CTRL_VLAN_ADD,
> >> +                                                  &data, 1);
> >> +    if (unlikely(dev_written < 0)) {
> >> +        return dev_written;
> >> +    }
> >> +    if (unlikely(*s->status != VIRTIO_NET_OK)) {
> >> +        return -EIO;
> >> +    }
> >> +
> >> +    return 0;
> >> +}
> >> +
> >> +static int vhost_vdpa_net_load_vlan(VhostVDPAState *s,
> >> +                                    const VirtIONet *n)
> >> +{
> >> +    int r;
> >> +
> >> +    if (!virtio_vdev_has_feature(&n->parent_obj, VIRTIO_NET_F_CTRL_VLAN)) {
> >> +        return 0;
> >> +    }
> >> +
> >> +    for (int i = 0; i < MAX_VLAN >> 5; i++) {
> >> +        for (int j = 0; n->vlans[i] && j <= 0x1f; j++) {
> >> +            if (n->vlans[i] & (1U << j)) {
> >> +                r = vhost_vdpa_net_load_single_vlan(s, n, (i << 5) + j);
> >> +                if (unlikely(r != 0)) {
> >> +                    return r;
> >> +                }
> >> +            }
> >> +        }
> >> +    }
> >> +
> >> +    return 0;
> >> +}
> >> +
> >>   static int vhost_vdpa_net_load(NetClientState *nc)
> >>   {
> >>       VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
> >> @@ -995,6 +1039,10 @@ static int vhost_vdpa_net_load(NetClientState *nc)
> >>       if (unlikely(r)) {
> >>           return r;
> >>       }
> >> +    r = vhost_vdpa_net_load_vlan(s, n);
> >> +    if (unlikely(r)) {
> >> +        return r;
> >> +    }
> >>
> >>       return 0;
> >>   }
> >> --
> >> 2.25.1
> >>
> >
>
diff mbox series

Patch

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 9795306742..347241796d 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -965,6 +965,50 @@  static int vhost_vdpa_net_load_rx(VhostVDPAState *s,
     return 0;
 }
 
+static int vhost_vdpa_net_load_single_vlan(VhostVDPAState *s,
+                                           const VirtIONet *n,
+                                           uint16_t vid)
+{
+    const struct iovec data = {
+        .iov_base = &vid,
+        .iov_len = sizeof(vid),
+    };
+    ssize_t dev_written = vhost_vdpa_net_load_cmd(s, VIRTIO_NET_CTRL_VLAN,
+                                                  VIRTIO_NET_CTRL_VLAN_ADD,
+                                                  &data, 1);
+    if (unlikely(dev_written < 0)) {
+        return dev_written;
+    }
+    if (unlikely(*s->status != VIRTIO_NET_OK)) {
+        return -EIO;
+    }
+
+    return 0;
+}
+
+static int vhost_vdpa_net_load_vlan(VhostVDPAState *s,
+                                    const VirtIONet *n)
+{
+    int r;
+
+    if (!virtio_vdev_has_feature(&n->parent_obj, VIRTIO_NET_F_CTRL_VLAN)) {
+        return 0;
+    }
+
+    for (int i = 0; i < MAX_VLAN >> 5; i++) {
+        for (int j = 0; n->vlans[i] && j <= 0x1f; j++) {
+            if (n->vlans[i] & (1U << j)) {
+                r = vhost_vdpa_net_load_single_vlan(s, n, (i << 5) + j);
+                if (unlikely(r != 0)) {
+                    return r;
+                }
+            }
+        }
+    }
+
+    return 0;
+}
+
 static int vhost_vdpa_net_load(NetClientState *nc)
 {
     VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
@@ -995,6 +1039,10 @@  static int vhost_vdpa_net_load(NetClientState *nc)
     if (unlikely(r)) {
         return r;
     }
+    r = vhost_vdpa_net_load_vlan(s, n);
+    if (unlikely(r)) {
+        return r;
+    }
 
     return 0;
 }