diff mbox series

[[RFC,v3,02/12] virtio: redefine structure & memory cache for packed ring

Message ID 1539266915-15216-3-git-send-email-wexu@redhat.com (mailing list archive)
State New, archived
Headers show
Series packed ring virtio-net userspace backend support | expand

Commit Message

Wei Xu Oct. 11, 2018, 2:08 p.m. UTC 
From: Wei Xu <wexu@redhat.com>

Redefine packed ring structure according to qemu nomenclature,
also supported data(event index, wrap counter, etc) are introduced.

Signed-off-by: Wei Xu <wexu@redhat.com>
---
 hw/virtio/virtio.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

Comments

Jason Wang Oct. 15, 2018, 3:03 a.m. UTC | #1 
On 2018年10月11日 22:08, wexu@redhat.com wrote:
> From: Wei Xu <wexu@redhat.com>
>
> Redefine packed ring structure according to qemu nomenclature,
> also supported data(event index, wrap counter, etc) are introduced.
>
> Signed-off-by: Wei Xu <wexu@redhat.com>
> ---
>   hw/virtio/virtio.c | 26 ++++++++++++++++++++++++--
>   1 file changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 94f5c8e..500eecf 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -39,6 +39,13 @@ typedef struct VRingDesc
>       uint16_t next;
>   } VRingDesc;
>   
> +typedef struct VRingPackedDesc {
> +    uint64_t addr;
> +    uint32_t len;
> +    uint16_t id;
> +    uint16_t flags;
> +} VRingPackedDesc;
> +
>   typedef struct VRingAvail
>   {
>       uint16_t flags;
> @@ -62,8 +69,14 @@ typedef struct VRingUsed
>   typedef struct VRingMemoryRegionCaches {
>       struct rcu_head rcu;
>       MemoryRegionCache desc;
> -    MemoryRegionCache avail;
> -    MemoryRegionCache used;
> +    union {
> +        MemoryRegionCache avail;
> +        MemoryRegionCache driver;
> +    };

Can we reuse avail and used?

> +    union {
> +        MemoryRegionCache used;
> +        MemoryRegionCache device;
> +    };
>   } VRingMemoryRegionCaches;
>   
>   typedef struct VRing
> @@ -77,6 +90,11 @@ typedef struct VRing
>       VRingMemoryRegionCaches *caches;
>   } VRing;
>   
> +typedef struct VRingPackedDescEvent {
> +    uint16_t off_wrap;
> +    uint16_t flags;
> +} VRingPackedDescEvent ;
> +
>   struct VirtQueue
>   {
>       VRing vring;
> @@ -87,6 +105,10 @@ struct VirtQueue
>       /* Last avail_idx read from VQ. */
>       uint16_t shadow_avail_idx;
>   
> +    uint16_t event_idx;

Need a comment to explain this field.

Thanks

> +    bool event_wrap_counter;
> +    bool avail_wrap_counter;
> +
>       uint16_t used_idx;
>   
>       /* Last used index value we have signalled on */
Wei Xu Oct. 15, 2018, 7:26 a.m. UTC | #2 
On Mon, Oct 15, 2018 at 11:03:52AM +0800, Jason Wang wrote:
> 
> 
> On 2018年10月11日 22:08, wexu@redhat.com wrote:
> >From: Wei Xu <wexu@redhat.com>
> >
> >Redefine packed ring structure according to qemu nomenclature,
> >also supported data(event index, wrap counter, etc) are introduced.
> >
> >Signed-off-by: Wei Xu <wexu@redhat.com>
> >---
> >  hw/virtio/virtio.c | 26 ++++++++++++++++++++++++--
> >  1 file changed, 24 insertions(+), 2 deletions(-)
> >
> >diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> >index 94f5c8e..500eecf 100644
> >--- a/hw/virtio/virtio.c
> >+++ b/hw/virtio/virtio.c
> >@@ -39,6 +39,13 @@ typedef struct VRingDesc
> >      uint16_t next;
> >  } VRingDesc;
> >+typedef struct VRingPackedDesc {
> >+    uint64_t addr;
> >+    uint32_t len;
> >+    uint16_t id;
> >+    uint16_t flags;
> >+} VRingPackedDesc;
> >+
> >  typedef struct VRingAvail
> >  {
> >      uint16_t flags;
> >@@ -62,8 +69,14 @@ typedef struct VRingUsed
> >  typedef struct VRingMemoryRegionCaches {
> >      struct rcu_head rcu;
> >      MemoryRegionCache desc;
> >-    MemoryRegionCache avail;
> >-    MemoryRegionCache used;
> >+    union {
> >+        MemoryRegionCache avail;
> >+        MemoryRegionCache driver;
> >+    };
> 
> Can we reuse avail and used?

Sure.

> 
> >+    union {
> >+        MemoryRegionCache used;
> >+        MemoryRegionCache device;
> >+    };
> >  } VRingMemoryRegionCaches;
> >  typedef struct VRing
> >@@ -77,6 +90,11 @@ typedef struct VRing
> >      VRingMemoryRegionCaches *caches;
> >  } VRing;
> >+typedef struct VRingPackedDescEvent {
> >+    uint16_t off_wrap;
> >+    uint16_t flags;
> >+} VRingPackedDescEvent ;
> >+
> >  struct VirtQueue
> >  {
> >      VRing vring;
> >@@ -87,6 +105,10 @@ struct VirtQueue
> >      /* Last avail_idx read from VQ. */
> >      uint16_t shadow_avail_idx;
> >+    uint16_t event_idx;
> 
> Need a comment to explain this field.

Yes, it is the unified name for interrupt which is what I want to see
if we can reuse 'shadow' and 'used' index in current code, for Tx
queue, it should be the 'used' index after finished sending the last
desc. For Rx queue, it should be the 'shadow' index when no enough
descriptors which might be a few descriptors ahead of the 'used' index,
there are a few indexes already so this makes code a bit redundant.

Will see if I can remove this in next version, any comments?

Wei


> 
> Thanks
> 
> >+    bool event_wrap_counter;
> >+    bool avail_wrap_counter;
> >+
> >      uint16_t used_idx;
> >      /* Last used index value we have signalled on */
> 
>
Jason Wang Oct. 15, 2018, 8:03 a.m. UTC | #3 
On 2018年10月15日 15:26, Wei Xu wrote:
> On Mon, Oct 15, 2018 at 11:03:52AM +0800, Jason Wang wrote:
>>
>> On 2018年10月11日 22:08, wexu@redhat.com wrote:
>>> From: Wei Xu <wexu@redhat.com>
>>>
>>> Redefine packed ring structure according to qemu nomenclature,
>>> also supported data(event index, wrap counter, etc) are introduced.
>>>
>>> Signed-off-by: Wei Xu <wexu@redhat.com>
>>> ---
>>>   hw/virtio/virtio.c | 26 ++++++++++++++++++++++++--
>>>   1 file changed, 24 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>>> index 94f5c8e..500eecf 100644
>>> --- a/hw/virtio/virtio.c
>>> +++ b/hw/virtio/virtio.c
>>> @@ -39,6 +39,13 @@ typedef struct VRingDesc
>>>       uint16_t next;
>>>   } VRingDesc;
>>> +typedef struct VRingPackedDesc {
>>> +    uint64_t addr;
>>> +    uint32_t len;
>>> +    uint16_t id;
>>> +    uint16_t flags;
>>> +} VRingPackedDesc;
>>> +
>>>   typedef struct VRingAvail
>>>   {
>>>       uint16_t flags;
>>> @@ -62,8 +69,14 @@ typedef struct VRingUsed
>>>   typedef struct VRingMemoryRegionCaches {
>>>       struct rcu_head rcu;
>>>       MemoryRegionCache desc;
>>> -    MemoryRegionCache avail;
>>> -    MemoryRegionCache used;
>>> +    union {
>>> +        MemoryRegionCache avail;
>>> +        MemoryRegionCache driver;
>>> +    };
>> Can we reuse avail and used?
> Sure.
>
>>> +    union {
>>> +        MemoryRegionCache used;
>>> +        MemoryRegionCache device;
>>> +    };
>>>   } VRingMemoryRegionCaches;
>>>   typedef struct VRing
>>> @@ -77,6 +90,11 @@ typedef struct VRing
>>>       VRingMemoryRegionCaches *caches;
>>>   } VRing;
>>> +typedef struct VRingPackedDescEvent {
>>> +    uint16_t off_wrap;
>>> +    uint16_t flags;
>>> +} VRingPackedDescEvent ;
>>> +
>>>   struct VirtQueue
>>>   {
>>>       VRing vring;
>>> @@ -87,6 +105,10 @@ struct VirtQueue
>>>       /* Last avail_idx read from VQ. */
>>>       uint16_t shadow_avail_idx;
>>> +    uint16_t event_idx;
>> Need a comment to explain this field.
> Yes, it is the unified name for interrupt which is what I want to see
> if we can reuse 'shadow' and 'used' index in current code, for Tx
> queue, it should be the 'used' index after finished sending the last
> desc. For Rx queue, it should be the 'shadow' index when no enough
> descriptors which might be a few descriptors ahead of the 'used' index,
> there are a few indexes already so this makes code a bit redundant.

If I understand your meaning correctly, you can refer vhost codes:

https://github.com/jasowang/net/commit/9b7b0d75d88d980a3c8954db0aa754b124facd0e

It maintains both last_wrap_counter and avail_wrap_counter/avail_idx. 
avail_wrap_counter/avail_idx is guaranteed to be increased 
monotonically, this is useful when we need to move avail index backwards.

Besides this, for qemu, I think you need a better name e.g 
last_avail_idx here.

Thanks

>
> Will see if I can remove this in next version, any comments?
>
> Wei
>
>
>> Thanks
>>
>>> +    bool event_wrap_counter;
>>> +    bool avail_wrap_counter;
>>> +
>>>       uint16_t used_idx;
>>>       /* Last used index value we have signalled on */
>>
diff mbox series

Patch

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 94f5c8e..500eecf 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -39,6 +39,13 @@  typedef struct VRingDesc
     uint16_t next;
 } VRingDesc;
 
+typedef struct VRingPackedDesc {
+    uint64_t addr;
+    uint32_t len;
+    uint16_t id;
+    uint16_t flags;
+} VRingPackedDesc;
+
 typedef struct VRingAvail
 {
     uint16_t flags;
@@ -62,8 +69,14 @@  typedef struct VRingUsed
 typedef struct VRingMemoryRegionCaches {
     struct rcu_head rcu;
     MemoryRegionCache desc;
-    MemoryRegionCache avail;
-    MemoryRegionCache used;
+    union {
+        MemoryRegionCache avail;
+        MemoryRegionCache driver;
+    };
+    union {
+        MemoryRegionCache used;
+        MemoryRegionCache device;
+    };
 } VRingMemoryRegionCaches;
 
 typedef struct VRing
@@ -77,6 +90,11 @@  typedef struct VRing
     VRingMemoryRegionCaches *caches;
 } VRing;
 
+typedef struct VRingPackedDescEvent {
+    uint16_t off_wrap;
+    uint16_t flags;
+} VRingPackedDescEvent ;
+
 struct VirtQueue
 {
     VRing vring;
@@ -87,6 +105,10 @@  struct VirtQueue
     /* Last avail_idx read from VQ. */
     uint16_t shadow_avail_idx;
 
+    uint16_t event_idx;
+    bool event_wrap_counter;
+    bool avail_wrap_counter;
+
     uint16_t used_idx;
 
     /* Last used index value we have signalled on */