| Message ID | 1442806742-32547-1-git-send-email-huawei.xie@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@dpdk.org Delivered-To: patchwork@dpdk.org Received: from [92.243.14.124] (localhost [IPv6:::1]) by dpdk.org (Postfix) with ESMTP id BAE6A5958; Mon, 21 Sep 2015 05:39:13 +0200 (CEST) Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id 7414958EF for <dev@dpdk.org>; Mon, 21 Sep 2015 05:39:12 +0200 (CEST) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga103.fm.intel.com with ESMTP; 20 Sep 2015 20:39:12 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.17,565,1437462000"; d="scan'208";a="773222736" Received: from shvmail01.sh.intel.com ([10.239.29.42]) by orsmga001.jf.intel.com with ESMTP; 20 Sep 2015 20:39:10 -0700 Received: from shecgisg003.sh.intel.com (shecgisg003.sh.intel.com [10.239.29.90]) by shvmail01.sh.intel.com with ESMTP id t8L3d8TN025024; Mon, 21 Sep 2015 11:39:08 +0800 Received: from shecgisg003.sh.intel.com (localhost [127.0.0.1]) by shecgisg003.sh.intel.com (8.13.6/8.13.6/SuSE Linux 0.8) with ESMTP id t8L3d5SW032582; Mon, 21 Sep 2015 11:39:07 +0800 Received: (from hxie5@localhost) by shecgisg003.sh.intel.com (8.13.6/8.13.6/Submit) id t8L3d5UV032578; Mon, 21 Sep 2015 11:39:05 +0800 From: Huawei Xie <huawei.xie@intel.com> To: dev@dpdk.org Date: Mon, 21 Sep 2015 11:39:02 +0800 Message-Id: <1442806742-32547-1-git-send-email-huawei.xie@intel.com> X-Mailer: git-send-email 1.7.4.1 Subject: [dpdk-dev] [PATCH] virtio: fix used ring address calculation X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK <dev.dpdk.org> List-Unsubscribe: <http://dpdk.org/ml/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://dpdk.org/ml/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <http://dpdk.org/ml/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> |
Commit Message
Huawei Xie
Sept. 21, 2015, 3:39 a.m. UTC
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(-)
Comments
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); > } > > /*
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.
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. >
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.
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.
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.
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); } /*