* [dpdk-dev] [PATCH] virtio: fix used ring address calculation
@ 2015-09-21 3:39 Huawei Xie
2015-09-21 9:19 ` Tan, Jianfeng
2015-09-24 7:30 ` Xie, Huawei
0 siblings, 2 replies; 8+ messages in thread
From: Huawei Xie @ 2015-09-21 3:39 UTC (permalink / raw)
To: dev
used event idx is put at the end of available ring. It isn't taken into account
when we calculate the address of used ring. Fortunately, it doesn't introduce
the bug with fixed queue number 256 and 4KB alignment.
Signed-off-by: hxie5 <huawei.xie@intel.com>
---
drivers/net/virtio/virtio_ring.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/virtio/virtio_ring.h b/drivers/net/virtio/virtio_ring.h
index a16c499..92e430d 100644
--- a/drivers/net/virtio/virtio_ring.h
+++ b/drivers/net/virtio/virtio_ring.h
@@ -145,7 +145,7 @@ vring_init(struct vring *vr, unsigned int num, uint8_t *p,
vr->avail = (struct vring_avail *) (p +
num * sizeof(struct vring_desc));
vr->used = (void *)
- RTE_ALIGN_CEIL((uintptr_t)(&vr->avail->ring[num]), align);
+ RTE_ALIGN_CEIL((uintptr_t)(&vr->avail->ring[num + 1]), align);
}
/*
--
1.9.3
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [dpdk-dev] [PATCH] virtio: fix used ring address calculation
2015-09-21 3:39 [dpdk-dev] [PATCH] virtio: fix used ring address calculation Huawei Xie
@ 2015-09-21 9:19 ` Tan, Jianfeng
2015-09-24 7:30 ` Xie, Huawei
1 sibling, 0 replies; 8+ messages in thread
From: Tan, Jianfeng @ 2015-09-21 9:19 UTC (permalink / raw)
To: Xie, Huawei, dev
-----Original Message-----
From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Huawei Xie
Sent: Monday, September 21, 2015 11:39 AM
To: dev@dpdk.org
Subject: [dpdk-dev] [PATCH] virtio: fix used ring address calculation
used event idx is put at the end of available ring. It isn't taken into account when we calculate the address of used ring. Fortunately, it doesn't introduce the bug with fixed queue number 256 and 4KB alignment.
Signed-off-by: hxie5 <huawei.xie@intel.com>
---
drivers/net/virtio/virtio_ring.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/virtio/virtio_ring.h b/drivers/net/virtio/virtio_ring.h
index a16c499..92e430d 100644
--- a/drivers/net/virtio/virtio_ring.h
+++ b/drivers/net/virtio/virtio_ring.h
@@ -145,7 +145,7 @@ vring_init(struct vring *vr, unsigned int num, uint8_t *p,
vr->avail = (struct vring_avail *) (p +
num * sizeof(struct vring_desc));
vr->used = (void *)
- RTE_ALIGN_CEIL((uintptr_t)(&vr->avail->ring[num]), align);
+ RTE_ALIGN_CEIL((uintptr_t)(&vr->avail->ring[num + 1]), align);
}
/*
--
Acked-by: Jianfeng Tan <Jianfeng.tan@intel.com>
1.9.3
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [dpdk-dev] [PATCH] virtio: fix used ring address calculation
2015-09-21 3:39 [dpdk-dev] [PATCH] virtio: fix used ring address calculation Huawei Xie
2015-09-21 9:19 ` Tan, Jianfeng
@ 2015-09-24 7:30 ` Xie, Huawei
2015-09-24 16:36 ` Stephen Hemminger
1 sibling, 1 reply; 8+ messages in thread
From: Xie, Huawei @ 2015-09-24 7:30 UTC (permalink / raw)
To: dev, Tan, Jianfeng
On 9/21/2015 11:39 AM, Xie, Huawei wrote:
vring_size calculation should consider both used_event_idx at the tail
of avail ring and avail_event_idx at the tail of used ring.
Will merge those two fixes and send a new patch.
> used event idx is put at the end of available ring. It isn't taken into account
> when we calculate the address of used ring. Fortunately, it doesn't introduce
> the bug with fixed queue number 256 and 4KB alignment.
>
> Signed-off-by: hxie5 <huawei.xie@intel.com>
> ---
> drivers/net/virtio/virtio_ring.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/virtio/virtio_ring.h b/drivers/net/virtio/virtio_ring.h
> index a16c499..92e430d 100644
> --- a/drivers/net/virtio/virtio_ring.h
> +++ b/drivers/net/virtio/virtio_ring.h
> @@ -145,7 +145,7 @@ vring_init(struct vring *vr, unsigned int num, uint8_t *p,
> vr->avail = (struct vring_avail *) (p +
> num * sizeof(struct vring_desc));
> vr->used = (void *)
> - RTE_ALIGN_CEIL((uintptr_t)(&vr->avail->ring[num]), align);
> + RTE_ALIGN_CEIL((uintptr_t)(&vr->avail->ring[num + 1]), align);
> }
>
> /*
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [dpdk-dev] [PATCH] virtio: fix used ring address calculation
2015-09-24 7:30 ` Xie, Huawei
@ 2015-09-24 16:36 ` Stephen Hemminger
2015-09-24 18:35 ` Xie, Huawei
0 siblings, 1 reply; 8+ messages in thread
From: Stephen Hemminger @ 2015-09-24 16:36 UTC (permalink / raw)
To: Xie, Huawei; +Cc: dev
On Thu, 24 Sep 2015 07:30:41 +0000
"Xie, Huawei" <huawei.xie@intel.com> wrote:
> On 9/21/2015 11:39 AM, Xie, Huawei wrote:
> vring_size calculation should consider both used_event_idx at the tail
> of avail ring and avail_event_idx at the tail of used ring.
> Will merge those two fixes and send a new patch.
> > used event idx is put at the end of available ring. It isn't taken into account
> > when we calculate the address of used ring. Fortunately, it doesn't introduce
> > the bug with fixed queue number 256 and 4KB alignment.
> >
> > Signed-off-by: hxie5 <huawei.xie@intel.com>
> > ---
> > drivers/net/virtio/virtio_ring.h | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/virtio/virtio_ring.h b/drivers/net/virtio/virtio_ring.h
> > index a16c499..92e430d 100644
> > --- a/drivers/net/virtio/virtio_ring.h
> > +++ b/drivers/net/virtio/virtio_ring.h
> > @@ -145,7 +145,7 @@ vring_init(struct vring *vr, unsigned int num, uint8_t *p,
> > vr->avail = (struct vring_avail *) (p +
> > num * sizeof(struct vring_desc));
> > vr->used = (void *)
> > - RTE_ALIGN_CEIL((uintptr_t)(&vr->avail->ring[num]), align);
> > + RTE_ALIGN_CEIL((uintptr_t)(&vr->avail->ring[num + 1]), align);
> > }
> >
> > /*
>
Why aren't we just using the standard Linux includes for this?
See <linux/virtio_ring.h> and the function vring_init()
Keeping parallel copies of headers is prone to failures.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [dpdk-dev] [PATCH] virtio: fix used ring address calculation
2015-09-24 16:36 ` Stephen Hemminger
@ 2015-09-24 18:35 ` Xie, Huawei
2015-09-24 21:01 ` Stephen Hemminger
0 siblings, 1 reply; 8+ messages in thread
From: Xie, Huawei @ 2015-09-24 18:35 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: dev
On 9/25/2015 12:36 AM, Stephen Hemminger wrote:
> On Thu, 24 Sep 2015 07:30:41 +0000
> "Xie, Huawei" <huawei.xie@intel.com> wrote:
>
>> On 9/21/2015 11:39 AM, Xie, Huawei wrote:
>> vring_size calculation should consider both used_event_idx at the tail
>> of avail ring and avail_event_idx at the tail of used ring.
>> Will merge those two fixes and send a new patch.
>>> used event idx is put at the end of available ring. It isn't taken into account
>>> when we calculate the address of used ring. Fortunately, it doesn't introduce
>>> the bug with fixed queue number 256 and 4KB alignment.
>>>
>>> Signed-off-by: hxie5 <huawei.xie@intel.com>
>>> ---
>>> drivers/net/virtio/virtio_ring.h | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/virtio/virtio_ring.h b/drivers/net/virtio/virtio_ring.h
>>> index a16c499..92e430d 100644
>>> --- a/drivers/net/virtio/virtio_ring.h
>>> +++ b/drivers/net/virtio/virtio_ring.h
>>> @@ -145,7 +145,7 @@ vring_init(struct vring *vr, unsigned int num, uint8_t *p,
>>> vr->avail = (struct vring_avail *) (p +
>>> num * sizeof(struct vring_desc));
>>> vr->used = (void *)
>>> - RTE_ALIGN_CEIL((uintptr_t)(&vr->avail->ring[num]), align);
>>> + RTE_ALIGN_CEIL((uintptr_t)(&vr->avail->ring[num + 1]), align);
>>> }
>>>
>>> /*
> Why aren't we just using the standard Linux includes for this?
> See <linux/virtio_ring.h> and the function vring_init()
>
> Keeping parallel copies of headers is prone to failures.
Agree.
Using standard Linux includes then at least we don't need to redefine
the feature and other related MACRO.
This applies to vhost as well.
For vring, vring_init, we could also reuse the linux implementation
unless we have strong reason to define our own structure.
One reason was to support both FreeBSD and Linux. FreeBSD should have
its own header file. To avoid the case they have different vring
structure or VIRTIO_F_xx macro name, they are redefined here.
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [dpdk-dev] [PATCH] virtio: fix used ring address calculation
2015-09-24 18:35 ` Xie, Huawei
@ 2015-09-24 21:01 ` Stephen Hemminger
2015-09-25 15:46 ` Xie, Huawei
0 siblings, 1 reply; 8+ messages in thread
From: Stephen Hemminger @ 2015-09-24 21:01 UTC (permalink / raw)
To: Xie, Huawei; +Cc: dev
On Thu, 24 Sep 2015 18:35:37 +0000
"Xie, Huawei" <huawei.xie@intel.com> wrote:
> On 9/25/2015 12:36 AM, Stephen Hemminger wrote:
> > On Thu, 24 Sep 2015 07:30:41 +0000
> > "Xie, Huawei" <huawei.xie@intel.com> wrote:
> >
> >> On 9/21/2015 11:39 AM, Xie, Huawei wrote:
> >> vring_size calculation should consider both used_event_idx at the tail
> >> of avail ring and avail_event_idx at the tail of used ring.
> >> Will merge those two fixes and send a new patch.
> >>> used event idx is put at the end of available ring. It isn't taken into account
> >>> when we calculate the address of used ring. Fortunately, it doesn't introduce
> >>> the bug with fixed queue number 256 and 4KB alignment.
> >>>
> >>> Signed-off-by: hxie5 <huawei.xie@intel.com>
> >>> ---
> >>> drivers/net/virtio/virtio_ring.h | 2 +-
> >>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/net/virtio/virtio_ring.h b/drivers/net/virtio/virtio_ring.h
> >>> index a16c499..92e430d 100644
> >>> --- a/drivers/net/virtio/virtio_ring.h
> >>> +++ b/drivers/net/virtio/virtio_ring.h
> >>> @@ -145,7 +145,7 @@ vring_init(struct vring *vr, unsigned int num, uint8_t *p,
> >>> vr->avail = (struct vring_avail *) (p +
> >>> num * sizeof(struct vring_desc));
> >>> vr->used = (void *)
> >>> - RTE_ALIGN_CEIL((uintptr_t)(&vr->avail->ring[num]), align);
> >>> + RTE_ALIGN_CEIL((uintptr_t)(&vr->avail->ring[num + 1]), align);
> >>> }
> >>>
> >>> /*
> > Why aren't we just using the standard Linux includes for this?
> > See <linux/virtio_ring.h> and the function vring_init()
> >
> > Keeping parallel copies of headers is prone to failures.
> Agree.
> Using standard Linux includes then at least we don't need to redefine
> the feature and other related MACRO.
> This applies to vhost as well.
> For vring, vring_init, we could also reuse the linux implementation
> unless we have strong reason to define our own structure.
> One reason was to support both FreeBSD and Linux. FreeBSD should have
> its own header file. To avoid the case they have different vring
> structure or VIRTIO_F_xx macro name, they are redefined here.
>
> >
>
The Linux headers for virtio are explictly BSD licensed.
You could at least just have a local copy of same code.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [dpdk-dev] [PATCH] virtio: fix used ring address calculation
2015-09-24 21:01 ` Stephen Hemminger
@ 2015-09-25 15:46 ` Xie, Huawei
2015-09-25 17:48 ` Stephen Hemminger
0 siblings, 1 reply; 8+ messages in thread
From: Xie, Huawei @ 2015-09-25 15:46 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: dev
On 9/25/2015 5:01 AM, Stephen Hemminger wrote:
> On Thu, 24 Sep 2015 18:35:37 +0000
> "Xie, Huawei" <huawei.xie@intel.com> wrote:
>
>> On 9/25/2015 12:36 AM, Stephen Hemminger wrote:
>>> On Thu, 24 Sep 2015 07:30:41 +0000
>>> "Xie, Huawei" <huawei.xie@intel.com> wrote:
>>>
>>>> On 9/21/2015 11:39 AM, Xie, Huawei wrote:
>>>> vring_size calculation should consider both used_event_idx at the tail
>>>> of avail ring and avail_event_idx at the tail of used ring.
>>>> Will merge those two fixes and send a new patch.
>>>>> used event idx is put at the end of available ring. It isn't taken into account
>>>>> when we calculate the address of used ring. Fortunately, it doesn't introduce
>>>>> the bug with fixed queue number 256 and 4KB alignment.
>>>>>
>>>>> Signed-off-by: hxie5 <huawei.xie@intel.com>
>>>>> ---
>>>>> drivers/net/virtio/virtio_ring.h | 2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/net/virtio/virtio_ring.h b/drivers/net/virtio/virtio_ring.h
>>>>> index a16c499..92e430d 100644
>>>>> --- a/drivers/net/virtio/virtio_ring.h
>>>>> +++ b/drivers/net/virtio/virtio_ring.h
>>>>> @@ -145,7 +145,7 @@ vring_init(struct vring *vr, unsigned int num, uint8_t *p,
>>>>> vr->avail = (struct vring_avail *) (p +
>>>>> num * sizeof(struct vring_desc));
>>>>> vr->used = (void *)
>>>>> - RTE_ALIGN_CEIL((uintptr_t)(&vr->avail->ring[num]), align);
>>>>> + RTE_ALIGN_CEIL((uintptr_t)(&vr->avail->ring[num + 1]), align);
>>>>> }
>>>>>
>>>>> /*
>>> Why aren't we just using the standard Linux includes for this?
>>> See <linux/virtio_ring.h> and the function vring_init()
>>>
>>> Keeping parallel copies of headers is prone to failures.
>> Agree.
>> Using standard Linux includes then at least we don't need to redefine
>> the feature and other related MACRO.
>> This applies to vhost as well.
>> For vring, vring_init, we could also reuse the linux implementation
>> unless we have strong reason to define our own structure.
>> One reason was to support both FreeBSD and Linux. FreeBSD should have
>> its own header file. To avoid the case they have different vring
>> structure or VIRTIO_F_xx macro name, they are redefined here.
>>
> The Linux headers for virtio are explictly BSD licensed.
> You could at least just have a local copy of same code.
>
Exactly the same code (if no dependency and no other issue) or copy and
convert it to DPDK style? By DPDK style, i mean like using RTE_ALIGN macro.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [dpdk-dev] [PATCH] virtio: fix used ring address calculation
2015-09-25 15:46 ` Xie, Huawei
@ 2015-09-25 17:48 ` Stephen Hemminger
0 siblings, 0 replies; 8+ messages in thread
From: Stephen Hemminger @ 2015-09-25 17:48 UTC (permalink / raw)
To: Xie, Huawei; +Cc: dev
On Fri, 25 Sep 2015 15:46:34 +0000
"Xie, Huawei" <huawei.xie@intel.com> wrote:
> On 9/25/2015 5:01 AM, Stephen Hemminger wrote:
> > On Thu, 24 Sep 2015 18:35:37 +0000
> > "Xie, Huawei" <huawei.xie@intel.com> wrote:
> >
> >> On 9/25/2015 12:36 AM, Stephen Hemminger wrote:
> >>> On Thu, 24 Sep 2015 07:30:41 +0000
> >>> "Xie, Huawei" <huawei.xie@intel.com> wrote:
> >>>
> >>>> On 9/21/2015 11:39 AM, Xie, Huawei wrote:
> >>>> vring_size calculation should consider both used_event_idx at the tail
> >>>> of avail ring and avail_event_idx at the tail of used ring.
> >>>> Will merge those two fixes and send a new patch.
> >>>>> used event idx is put at the end of available ring. It isn't taken into account
> >>>>> when we calculate the address of used ring. Fortunately, it doesn't introduce
> >>>>> the bug with fixed queue number 256 and 4KB alignment.
> >>>>>
> >>>>> Signed-off-by: hxie5 <huawei.xie@intel.com>
> >>>>> ---
> >>>>> drivers/net/virtio/virtio_ring.h | 2 +-
> >>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/drivers/net/virtio/virtio_ring.h b/drivers/net/virtio/virtio_ring.h
> >>>>> index a16c499..92e430d 100644
> >>>>> --- a/drivers/net/virtio/virtio_ring.h
> >>>>> +++ b/drivers/net/virtio/virtio_ring.h
> >>>>> @@ -145,7 +145,7 @@ vring_init(struct vring *vr, unsigned int num, uint8_t *p,
> >>>>> vr->avail = (struct vring_avail *) (p +
> >>>>> num * sizeof(struct vring_desc));
> >>>>> vr->used = (void *)
> >>>>> - RTE_ALIGN_CEIL((uintptr_t)(&vr->avail->ring[num]), align);
> >>>>> + RTE_ALIGN_CEIL((uintptr_t)(&vr->avail->ring[num + 1]), align);
> >>>>> }
> >>>>>
> >>>>> /*
> >>> Why aren't we just using the standard Linux includes for this?
> >>> See <linux/virtio_ring.h> and the function vring_init()
> >>>
> >>> Keeping parallel copies of headers is prone to failures.
> >> Agree.
> >> Using standard Linux includes then at least we don't need to redefine
> >> the feature and other related MACRO.
> >> This applies to vhost as well.
> >> For vring, vring_init, we could also reuse the linux implementation
> >> unless we have strong reason to define our own structure.
> >> One reason was to support both FreeBSD and Linux. FreeBSD should have
> >> its own header file. To avoid the case they have different vring
> >> structure or VIRTIO_F_xx macro name, they are redefined here.
> >>
> > The Linux headers for virtio are explictly BSD licensed.
> > You could at least just have a local copy of same code.
> >
> Exactly the same code (if no dependency and no other issue) or copy and
> convert it to DPDK style? By DPDK style, i mean like using RTE_ALIGN macro.
No. keep the Linux code as is. Just copy the headers.
Don't introduce DPDK style.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2015-09-25 17:48 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-09-21 3:39 [dpdk-dev] [PATCH] virtio: fix used ring address calculation Huawei Xie
2015-09-21 9:19 ` Tan, Jianfeng
2015-09-24 7:30 ` Xie, Huawei
2015-09-24 16:36 ` Stephen Hemminger
2015-09-24 18:35 ` Xie, Huawei
2015-09-24 21:01 ` Stephen Hemminger
2015-09-25 15:46 ` Xie, Huawei
2015-09-25 17:48 ` Stephen Hemminger
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).