Message ID | BLU436-SMTP1791A6BD70260D97089EDBFBFF90@phx.gbl (mailing list archive) |
---|---|
State | Rejected, 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 4C195C32A; Sat, 11 Apr 2015 17:33:57 +0200 (CEST) Received: from BLU004-OMC4S27.hotmail.com (blu004-omc4s27.hotmail.com [65.55.111.166]) by dpdk.org (Postfix) with ESMTP id 864E5C322 for <dev@dpdk.org>; Sat, 11 Apr 2015 17:33:55 +0200 (CEST) Received: from BLU436-SMTP179 ([65.55.111.136]) by BLU004-OMC4S27.hotmail.com over TLS secured channel with Microsoft SMTPSVC(7.5.7601.22751); Sat, 11 Apr 2015 08:33:54 -0700 X-TMN: [3bvQFERcMSq3+Z23mwl8mOStg0Iji3mu] X-Originating-Email: [dong.wang.pro@hotmail.com] Message-ID: <BLU436-SMTP1791A6BD70260D97089EDBFBFF90@phx.gbl> From: WangDong <dong.wang.pro@hotmail.com> To: dev@dpdk.org Date: Sat, 11 Apr 2015 23:33:35 +0800 X-Mailer: git-send-email 1.9.1 X-OriginalArrivalTime: 11 Apr 2015 15:33:54.0112 (UTC) FILETIME=[EAF77400:01D0746C] MIME-Version: 1.0 Content-Type: text/plain Subject: [dpdk-dev] [PATCH] ixgbe:Add write memory barrier for recv pkts. 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
WangDong
April 11, 2015, 3:33 p.m. UTC
Like transmit packets, before update receive descriptor's tail pointer, rte_wmb() should be added after writing recv descriptor.
Signed-off-by: Dong Wang <dong.wang.pro@hotmail.com>
---
lib/librte_pmd_ixgbe/ixgbe_rxtx.c | 5 +++++
1 file changed, 5 insertions(+)
Comments
Hi, > -----Original Message----- > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of WangDong > Sent: Saturday, April 11, 2015 4:34 PM > To: dev@dpdk.org > Subject: [dpdk-dev] [PATCH] ixgbe:Add write memory barrier for recv pkts. > > Like transmit packets, before update receive descriptor's tail pointer, rte_wmb() should be added after writing recv descriptor. > > Signed-off-by: Dong Wang <dong.wang.pro@hotmail.com> > --- > lib/librte_pmd_ixgbe/ixgbe_rxtx.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c > index 9da2c7e..d504688 100644 > --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c > +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c > @@ -1338,6 +1338,9 @@ ixgbe_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, > */ > rx_pkts[nb_rx++] = rxm; > } > + > + rte_wmb(); > + Why do you think it is necessary? I can't see any good reason to put wmb() here. I would understand if, at least you'll try to insert it just before updating RDT: rx_id = (uint16_t) ((rx_id == 0) ? (rxq->nb_rx_desc - 1) : (rx_id - 1)); + rte_wmb(); IXGBE_PCI_REG_WRITE(rxq->rdt_reg_addr, rx_id); That is not needed IA with current implementation, but would make sense for machines with relaxed memory ordering. Though right now DPDK IXGBE PMD is supported only on IA, anyway. Same for ixgbe_recv_scattered_pkts(). Konstantin > rxq->rx_tail = rx_id; > > /* > @@ -1595,6 +1598,8 @@ ixgbe_recv_scattered_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, > first_seg = NULL; > } > > + rte_wmb(); > + > /* > * Record index of the next RX descriptor to probe. > */ > -- > 1.9.1
> Hi, > >> -----Original Message----- >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of WangDong >> Sent: Saturday, April 11, 2015 4:34 PM >> To: dev@dpdk.org >> Subject: [dpdk-dev] [PATCH] ixgbe:Add write memory barrier for recv pkts. >> >> Like transmit packets, before update receive descriptor's tail pointer, rte_wmb() should be added after writing recv descriptor. >> >> Signed-off-by: Dong Wang <dong.wang.pro@hotmail.com> >> --- >> lib/librte_pmd_ixgbe/ixgbe_rxtx.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c >> index 9da2c7e..d504688 100644 >> --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c >> +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c >> @@ -1338,6 +1338,9 @@ ixgbe_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, >> */ >> rx_pkts[nb_rx++] = rxm; >> } >> + >> + rte_wmb(); >> + > > Why do you think it is necessary? > I can't see any good reason to put wmb() here. > I would understand if, at least you'll try to insert it just before updating RDT: > rx_id = (uint16_t) ((rx_id == 0) ? > (rxq->nb_rx_desc - 1) : (rx_id - 1)); > + rte_wmb(); > IXGBE_PCI_REG_WRITE(rxq->rdt_reg_addr, rx_id); > > That is not needed IA with current implementation, but would make sense for machines with relaxed memory ordering. > Though right now DPDK IXGBE PMD is supported only on IA, anyway. > Same for ixgbe_recv_scattered_pkts(). > > Konstantin Yes, current implementation works well with IA, and the transmit packets function's rte_wmb() is also unneccessary. But there are two reasons for adding rte_wmb() in recv pkts function: 1) The memory barrier in recv pkts function and xmit pkts function are inconsistent, rte_wmb() should be added to recv pkts function or be removed from xmit pkts function. 2) DPDK will support PowerPC processor (Other developers are working on it), I check the memory ordering of PowerPC, there was no mention of store-store instruction's principle in MPC8544 Reference Manual, only said it is weak memory ordering. So, I think it is neccessary to add rte_wmb() to recv pkts function. Dong > > >> rxq->rx_tail = rx_id; >> >> /* >> @@ -1595,6 +1598,8 @@ ixgbe_recv_scattered_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, >> first_seg = NULL; >> } >> >> + rte_wmb(); >> + >> /* >> * Record index of the next RX descriptor to probe. >> */ >> -- >> 1.9.1 >
On Wed, 15 Apr 2015 21:46:27 +0800 Dong.Wang <dong.wang.pro@hotmail.com> wrote: > Yes, current implementation works well with IA, and the transmit packets > function's rte_wmb() is also unneccessary. > > But there are two reasons for adding rte_wmb() in recv pkts function: > 1) The memory barrier in recv pkts function and xmit pkts function are > inconsistent, rte_wmb() should be added to recv pkts function or be > removed from xmit pkts function. > 2) DPDK will support PowerPC processor (Other developers are working on > it), I check the memory ordering of PowerPC, there was no mention of > store-store instruction's principle in MPC8544 Reference Manual, only > said it is weak memory ordering. > > So, I think it is neccessary to add rte_wmb() to recv pkts function. > > Dong If PowerPC requires additional memory barriers then it should introduce a new generic set of memory barrier macros that are no-ops on other architectures. Please don't penalize x86 for places where other CPU's have weaker consistency.
> -----Original Message----- > From: outlook_739db8e1c4bc6fae@outlook.com [mailto:outlook_739db8e1c4bc6fae@outlook.com] On Behalf Of Dong.Wang > Sent: Wednesday, April 15, 2015 2:46 PM > To: Ananyev, Konstantin; dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH] ixgbe:Add write memory barrier for recv pkts. > > > > > Hi, > > > >> -----Original Message----- > >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of WangDong > >> Sent: Saturday, April 11, 2015 4:34 PM > >> To: dev@dpdk.org > >> Subject: [dpdk-dev] [PATCH] ixgbe:Add write memory barrier for recv pkts. > >> > >> Like transmit packets, before update receive descriptor's tail pointer, rte_wmb() should be added after writing recv descriptor. > >> > >> Signed-off-by: Dong Wang <dong.wang.pro@hotmail.com> > >> --- > >> lib/librte_pmd_ixgbe/ixgbe_rxtx.c | 5 +++++ > >> 1 file changed, 5 insertions(+) > >> > >> diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c > >> index 9da2c7e..d504688 100644 > >> --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c > >> +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c > >> @@ -1338,6 +1338,9 @@ ixgbe_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, > >> */ > >> rx_pkts[nb_rx++] = rxm; > >> } > >> + > >> + rte_wmb(); > >> + > > > > Why do you think it is necessary? > > I can't see any good reason to put wmb() here. > > I would understand if, at least you'll try to insert it just before updating RDT: > > rx_id = (uint16_t) ((rx_id == 0) ? > > (rxq->nb_rx_desc - 1) : (rx_id - 1)); > > + rte_wmb(); > > IXGBE_PCI_REG_WRITE(rxq->rdt_reg_addr, rx_id); > > > > That is not needed IA with current implementation, but would make sense for machines with relaxed memory ordering. > > Though right now DPDK IXGBE PMD is supported only on IA, anyway. > > Same for ixgbe_recv_scattered_pkts(). > > > > Konstantin > > Yes, current implementation works well with IA, and the transmit packets > function's rte_wmb() is also unneccessary. > > But there are two reasons for adding rte_wmb() in recv pkts function: > 1) The memory barrier in recv pkts function and xmit pkts function are > inconsistent, rte_wmb() should be added to recv pkts function or be > removed from xmit pkts function. > 2) DPDK will support PowerPC processor (Other developers are working on > it), I check the memory ordering of PowerPC, there was no mention of > store-store instruction's principle in MPC8544 Reference Manual, only > said it is weak memory ordering. > > So, I think it is neccessary to add rte_wmb() to recv pkts function. > > Dong What I was trying to say: 1. I think you put barrier in a wrong place. Even for machines with weak memory ordering, we need a barrier only when we are goint to update RDT, i.e: if (nb_hold > rxq->rx_free_thresh) { ... ; barrier; IXGBE_PCI_REG_WRITE(rxq->rdt_reg_addr, ...); } 2. Even with putting wmb() here, you wouldn't fix ixgbe_recv_pkts() to work on machines with weak memory ordering. I think that to make it work properly, you'll need an rmb() bewtween reading DD bit and rest of RXD: rxdp = &rx_ring[rx_id]; staterr = rxdp->wb.upper.status_error; + rte_rmb(); if (! (staterr & rte_cpu_to_le_32(IXGBE_RXDADV_STAT_DD))) break; rxd = *rxdp; 3. As Stephen pointed in his mail, we shouldn't penalise IA implementation with unnecessary barriers As was discussed at that thread: http://dpdk.org/ml/archives/dev/2015-March/015202.html probably the best is to introduce a new macros: rte_smp_*mb (or something) that would be architecture dependent: compiler_barrier on IA, proper HW barrier on machines with weak memory ordering and update the code to use it. So, if you like to fix that issue, please do that in a proper way. BTW, I think that for PPC support even before touching ixgbe or any other PMD, step 3 (or similar) need to be done on rte_ring enqueue/dequeue code. Konstantin > > > > > >> rxq->rx_tail = rx_id; > >> > >> /* > >> @@ -1595,6 +1598,8 @@ ixgbe_recv_scattered_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, > >> first_seg = NULL; > >> } > >> > >> + rte_wmb(); > >> + > >> /* > >> * Record index of the next RX descriptor to probe. > >> */ > >> -- > >> 1.9.1 > >
> On Wed, 15 Apr 2015 21:46:27 +0800 > Dong.Wang <dong.wang.pro@hotmail.com> wrote: > >> Yes, current implementation works well with IA, and the transmit packets >> function's rte_wmb() is also unneccessary. >> >> But there are two reasons for adding rte_wmb() in recv pkts function: >> 1) The memory barrier in recv pkts function and xmit pkts function are >> inconsistent, rte_wmb() should be added to recv pkts function or be >> removed from xmit pkts function. >> 2) DPDK will support PowerPC processor (Other developers are working on >> it), I check the memory ordering of PowerPC, there was no mention of >> store-store instruction's principle in MPC8544 Reference Manual, only >> said it is weak memory ordering. >> >> So, I think it is neccessary to add rte_wmb() to recv pkts function. >> >> Dong > > If PowerPC requires additional memory barriers then it should > introduce a new generic set of memory barrier macros that are no-ops > on other architectures. > > Please don't penalize x86 for places where other CPU's have > weaker consistency. > Yes, put rte_wmb() here will penalize x86, I was inconsiderate of it. Maybe a new set of memory barrier macros will be introduced, it be discussed in another thread. For now, add rte_wmb() is unnessary, I'm waiting for new memory barrier macros. Dong
> > >> -----Original Message----- >> From: outlook_739db8e1c4bc6fae@outlook.com [mailto:outlook_739db8e1c4bc6fae@outlook.com] On Behalf Of Dong.Wang >> Sent: Wednesday, April 15, 2015 2:46 PM >> To: Ananyev, Konstantin; dev@dpdk.org >> Subject: Re: [dpdk-dev] [PATCH] ixgbe:Add write memory barrier for recv pkts. >> >> >> >>> Hi, >>> >>>> -----Original Message----- >>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of WangDong >>>> Sent: Saturday, April 11, 2015 4:34 PM >>>> To: dev@dpdk.org >>>> Subject: [dpdk-dev] [PATCH] ixgbe:Add write memory barrier for recv pkts. >>>> >>>> Like transmit packets, before update receive descriptor's tail pointer, rte_wmb() should be added after writing recv descriptor. >>>> >>>> Signed-off-by: Dong Wang <dong.wang.pro@hotmail.com> >>>> --- >>>> lib/librte_pmd_ixgbe/ixgbe_rxtx.c | 5 +++++ >>>> 1 file changed, 5 insertions(+) >>>> >>>> diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c >>>> index 9da2c7e..d504688 100644 >>>> --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c >>>> +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c >>>> @@ -1338,6 +1338,9 @@ ixgbe_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, >>>> */ >>>> rx_pkts[nb_rx++] = rxm; >>>> } >>>> + >>>> + rte_wmb(); >>>> + >>> >>> Why do you think it is necessary? >>> I can't see any good reason to put wmb() here. >>> I would understand if, at least you'll try to insert it just before updating RDT: >>> rx_id = (uint16_t) ((rx_id == 0) ? >>> (rxq->nb_rx_desc - 1) : (rx_id - 1)); >>> + rte_wmb(); >>> IXGBE_PCI_REG_WRITE(rxq->rdt_reg_addr, rx_id); >>> >>> That is not needed IA with current implementation, but would make sense for machines with relaxed memory ordering. >>> Though right now DPDK IXGBE PMD is supported only on IA, anyway. >>> Same for ixgbe_recv_scattered_pkts(). >>> >>> Konstantin >> >> Yes, current implementation works well with IA, and the transmit packets >> function's rte_wmb() is also unneccessary. >> >> But there are two reasons for adding rte_wmb() in recv pkts function: >> 1) The memory barrier in recv pkts function and xmit pkts function are >> inconsistent, rte_wmb() should be added to recv pkts function or be >> removed from xmit pkts function. >> 2) DPDK will support PowerPC processor (Other developers are working on >> it), I check the memory ordering of PowerPC, there was no mention of >> store-store instruction's principle in MPC8544 Reference Manual, only >> said it is weak memory ordering. >> >> So, I think it is neccessary to add rte_wmb() to recv pkts function. >> >> Dong > > What I was trying to say: > > 1. I think you put barrier in a wrong place. > Even for machines with weak memory ordering, we need a barrier only when we are goint to update RDT, i.e: > if (nb_hold > rxq->rx_free_thresh) { ... ; barrier; IXGBE_PCI_REG_WRITE(rxq->rdt_reg_addr, ...); } Yes, I put it in a wrong place, it will reduce performance. It's better to place it in that you suggested. > > 2. Even with putting wmb() here, you wouldn't fix ixgbe_recv_pkts() to work on machines with weak memory ordering. > I think that to make it work properly, you'll need an rmb() bewtween reading DD bit and rest of RXD: > > rxdp = &rx_ring[rx_id]; > staterr = rxdp->wb.upper.status_error; > + rte_rmb(); > if (! (staterr & rte_cpu_to_le_32(IXGBE_RXDADV_STAT_DD))) > break; > rxd = *rxdp; Yes, it seems wmb is not enough for weak memory ordering processor. Both rmb and wmb are needed. > > 3. As Stephen pointed in his mail, we shouldn't penalise IA implementation with unnecessary barriers > As was discussed at that thread: http://dpdk.org/ml/archives/dev/2015-March/015202.html > probably the best is to introduce a new macros: rte_smp_*mb (or something) that would be architecture dependent: > compiler_barrier on IA, proper HW barrier on machines with weak memory ordering and update the code to use it. > > So, if you like to fix that issue, please do that in a proper way. > > BTW, I think that for PPC support even before touching ixgbe or any other PMD, > step 3 (or similar) need to be done on rte_ring enqueue/dequeue code. > > Konstantin Yes, a new set of macros should be introduced first, then we can update PMD code. Did anyone are working on it now ? Dong > >>> >>> >>>> rxq->rx_tail = rx_id; >>>> >>>> /* >>>> @@ -1595,6 +1598,8 @@ ixgbe_recv_scattered_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, >>>> first_seg = NULL; >>>> } >>>> >>>> + rte_wmb(); >>>> + >>>> /* >>>> * Record index of the next RX descriptor to probe. >>>> */ >>>> -- >>>> 1.9.1 >>>
> -----Original Message----- > From: outlook_739db8e1c4bc6fae@outlook.com [mailto:outlook_739db8e1c4bc6fae@outlook.com] On Behalf Of Wang Dong > Sent: Thursday, April 16, 2015 12:36 PM > To: Ananyev, Konstantin; dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH] ixgbe:Add write memory barrier for recv pkts. > > > > > > >> -----Original Message----- > >> From: outlook_739db8e1c4bc6fae@outlook.com [mailto:outlook_739db8e1c4bc6fae@outlook.com] On Behalf Of Dong.Wang > >> Sent: Wednesday, April 15, 2015 2:46 PM > >> To: Ananyev, Konstantin; dev@dpdk.org > >> Subject: Re: [dpdk-dev] [PATCH] ixgbe:Add write memory barrier for recv pkts. > >> > >> > >> > >>> Hi, > >>> > >>>> -----Original Message----- > >>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of WangDong > >>>> Sent: Saturday, April 11, 2015 4:34 PM > >>>> To: dev@dpdk.org > >>>> Subject: [dpdk-dev] [PATCH] ixgbe:Add write memory barrier for recv pkts. > >>>> > >>>> Like transmit packets, before update receive descriptor's tail pointer, rte_wmb() should be added after writing recv descriptor. > >>>> > >>>> Signed-off-by: Dong Wang <dong.wang.pro@hotmail.com> > >>>> --- > >>>> lib/librte_pmd_ixgbe/ixgbe_rxtx.c | 5 +++++ > >>>> 1 file changed, 5 insertions(+) > >>>> > >>>> diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c > >>>> index 9da2c7e..d504688 100644 > >>>> --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c > >>>> +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c > >>>> @@ -1338,6 +1338,9 @@ ixgbe_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, > >>>> */ > >>>> rx_pkts[nb_rx++] = rxm; > >>>> } > >>>> + > >>>> + rte_wmb(); > >>>> + > >>> > >>> Why do you think it is necessary? > >>> I can't see any good reason to put wmb() here. > >>> I would understand if, at least you'll try to insert it just before updating RDT: > >>> rx_id = (uint16_t) ((rx_id == 0) ? > >>> (rxq->nb_rx_desc - 1) : (rx_id - 1)); > >>> + rte_wmb(); > >>> IXGBE_PCI_REG_WRITE(rxq->rdt_reg_addr, rx_id); > >>> > >>> That is not needed IA with current implementation, but would make sense for machines with relaxed memory ordering. > >>> Though right now DPDK IXGBE PMD is supported only on IA, anyway. > >>> Same for ixgbe_recv_scattered_pkts(). > >>> > >>> Konstantin > >> > >> Yes, current implementation works well with IA, and the transmit packets > >> function's rte_wmb() is also unneccessary. > >> > >> But there are two reasons for adding rte_wmb() in recv pkts function: > >> 1) The memory barrier in recv pkts function and xmit pkts function are > >> inconsistent, rte_wmb() should be added to recv pkts function or be > >> removed from xmit pkts function. > >> 2) DPDK will support PowerPC processor (Other developers are working on > >> it), I check the memory ordering of PowerPC, there was no mention of > >> store-store instruction's principle in MPC8544 Reference Manual, only > >> said it is weak memory ordering. > >> > >> So, I think it is neccessary to add rte_wmb() to recv pkts function. > >> > >> Dong > > > > What I was trying to say: > > > > 1. I think you put barrier in a wrong place. > > Even for machines with weak memory ordering, we need a barrier only when we are goint to update RDT, i.e: > > if (nb_hold > rxq->rx_free_thresh) { ... ; barrier; IXGBE_PCI_REG_WRITE(rxq->rdt_reg_addr, ...); } > Yes, I put it in a wrong place, it will reduce performance. It's better > to place it in that you suggested. > > > > 2. Even with putting wmb() here, you wouldn't fix ixgbe_recv_pkts() to work on machines with weak memory ordering. > > I think that to make it work properly, you'll need an rmb() bewtween reading DD bit and rest of RXD: > > > > rxdp = &rx_ring[rx_id]; > > staterr = rxdp->wb.upper.status_error; > > + rte_rmb(); > > if (! (staterr & rte_cpu_to_le_32(IXGBE_RXDADV_STAT_DD))) > > break; > > rxd = *rxdp; > Yes, it seems wmb is not enough for weak memory ordering processor. Both > rmb and wmb are needed. > > > > 3. As Stephen pointed in his mail, we shouldn't penalise IA implementation with unnecessary barriers > > As was discussed at that thread: http://dpdk.org/ml/archives/dev/2015-March/015202.html > > probably the best is to introduce a new macros: rte_smp_*mb (or something) that would be architecture dependent: > > compiler_barrier on IA, proper HW barrier on machines with weak memory ordering and update the code to use it. > > > > So, if you like to fix that issue, please do that in a proper way. > > > > BTW, I think that for PPC support even before touching ixgbe or any other PMD, > > step 3 (or similar) need to be done on rte_ring enqueue/dequeue code. > > > > Konstantin > Yes, a new set of macros should be introduced first, then we can update > PMD code. Did anyone are working on it now ? As far as I know, no one is working on it right now. So, I suppose, you are welcome to start :) Konstantin > > Dong > > > >>> > >>> > >>>> rxq->rx_tail = rx_id; > >>>> > >>>> /* > >>>> @@ -1595,6 +1598,8 @@ ixgbe_recv_scattered_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, > >>>> first_seg = NULL; > >>>> } > >>>> > >>>> + rte_wmb(); > >>>> + > >>>> /* > >>>> * Record index of the next RX descriptor to probe. > >>>> */ > >>>> -- > >>>> 1.9.1 > >>>
On Thu, Apr 16, 2015 at 5:14 PM, Ananyev, Konstantin < konstantin.ananyev@intel.com> wrote: > > Yes, a new set of macros should be introduced first, then we can update > > PMD code. Did anyone are working on it now ? > > As far as I know, no one is working on it right now. > So, I suppose, you are welcome to start :) > Not working on it, so yes if you volunteer, you are welcome.
Hi, Konstantin, > > >> -----Original Message----- >> From: outlook_739db8e1c4bc6fae@outlook.com [mailto:outlook_739db8e1c4bc6fae@outlook.com] On Behalf Of Wang Dong >> Sent: Thursday, April 16, 2015 12:36 PM >> To: Ananyev, Konstantin; dev@dpdk.org >> Subject: Re: [dpdk-dev] [PATCH] ixgbe:Add write memory barrier for recv pkts. >> >>> >>> >>>> -----Original Message----- >>>> From: outlook_739db8e1c4bc6fae@outlook.com [mailto:outlook_739db8e1c4bc6fae@outlook.com] On Behalf Of Dong.Wang >>>> Sent: Wednesday, April 15, 2015 2:46 PM >>>> To: Ananyev, Konstantin; dev@dpdk.org >>>> Subject: Re: [dpdk-dev] [PATCH] ixgbe:Add write memory barrier for recv pkts. >>>> >>>> >>>> >>>>> Hi, >>>>> >>>>>> -----Original Message----- >>>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of WangDong >>>>>> Sent: Saturday, April 11, 2015 4:34 PM >>>>>> To: dev@dpdk.org >>>>>> Subject: [dpdk-dev] [PATCH] ixgbe:Add write memory barrier for recv pkts. >>>>>> >>>>>> Like transmit packets, before update receive descriptor's tail pointer, rte_wmb() should be added after writing recv descriptor. >>>>>> >>>>>> Signed-off-by: Dong Wang <dong.wang.pro@hotmail.com> >>>>>> --- >>>>>> lib/librte_pmd_ixgbe/ixgbe_rxtx.c | 5 +++++ >>>>>> 1 file changed, 5 insertions(+) >>>>>> >>>>>> diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c >>>>>> index 9da2c7e..d504688 100644 >>>>>> --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c >>>>>> +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c >>>>>> @@ -1338,6 +1338,9 @@ ixgbe_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, >>>>>> */ >>>>>> rx_pkts[nb_rx++] = rxm; >>>>>> } >>>>>> + >>>>>> + rte_wmb(); >>>>>> + >>>>> >>>>> Why do you think it is necessary? >>>>> I can't see any good reason to put wmb() here. >>>>> I would understand if, at least you'll try to insert it just before updating RDT: >>>>> rx_id = (uint16_t) ((rx_id == 0) ? >>>>> (rxq->nb_rx_desc - 1) : (rx_id - 1)); >>>>> + rte_wmb(); >>>>> IXGBE_PCI_REG_WRITE(rxq->rdt_reg_addr, rx_id); >>>>> >>>>> That is not needed IA with current implementation, but would make sense for machines with relaxed memory ordering. >>>>> Though right now DPDK IXGBE PMD is supported only on IA, anyway. >>>>> Same for ixgbe_recv_scattered_pkts(). >>>>> >>>>> Konstantin >>>> >>>> Yes, current implementation works well with IA, and the transmit packets >>>> function's rte_wmb() is also unneccessary. >>>> >>>> But there are two reasons for adding rte_wmb() in recv pkts function: >>>> 1) The memory barrier in recv pkts function and xmit pkts function are >>>> inconsistent, rte_wmb() should be added to recv pkts function or be >>>> removed from xmit pkts function. >>>> 2) DPDK will support PowerPC processor (Other developers are working on >>>> it), I check the memory ordering of PowerPC, there was no mention of >>>> store-store instruction's principle in MPC8544 Reference Manual, only >>>> said it is weak memory ordering. >>>> >>>> So, I think it is neccessary to add rte_wmb() to recv pkts function. >>>> >>>> Dong >>> >>> What I was trying to say: >>> >>> 1. I think you put barrier in a wrong place. >>> Even for machines with weak memory ordering, we need a barrier only when we are goint to update RDT, i.e: >>> if (nb_hold > rxq->rx_free_thresh) { ... ; barrier; IXGBE_PCI_REG_WRITE(rxq->rdt_reg_addr, ...); } >> Yes, I put it in a wrong place, it will reduce performance. It's better >> to place it in that you suggested. >>> >>> 2. Even with putting wmb() here, you wouldn't fix ixgbe_recv_pkts() to work on machines with weak memory ordering. >>> I think that to make it work properly, you'll need an rmb() bewtween reading DD bit and rest of RXD: >>> >>> rxdp = &rx_ring[rx_id]; >>> staterr = rxdp->wb.upper.status_error; >>> + rte_rmb(); >>> if (! (staterr & rte_cpu_to_le_32(IXGBE_RXDADV_STAT_DD))) >>> break; >>> rxd = *rxdp; >> Yes, it seems wmb is not enough for weak memory ordering processor. Both >> rmb and wmb are needed. >>> >>> 3. As Stephen pointed in his mail, we shouldn't penalise IA implementation with unnecessary barriers >>> As was discussed at that thread: http://dpdk.org/ml/archives/dev/2015-March/015202.html >>> probably the best is to introduce a new macros: rte_smp_*mb (or something) that would be architecture dependent: >>> compiler_barrier on IA, proper HW barrier on machines with weak memory ordering and update the code to use it. I was trying to add a new macro, but I found it didn't need a new memory barrier macro, may be a macro that can distinguish the memory barrier (rte_wmb/rte_rmb) of IA and AMD is useful. Other architecture still use the rte_wmb() and rte_rmb(). I send a patch about it, please take a look at it...... Dong >>> >>> So, if you like to fix that issue, please do that in a proper way. >>> >>> BTW, I think that for PPC support even before touching ixgbe or any other PMD, >>> step 3 (or similar) need to be done on rte_ring enqueue/dequeue code. >>> >>> Konstantin >> Yes, a new set of macros should be introduced first, then we can update >> PMD code. Did anyone are working on it now ? > > As far as I know, no one is working on it right now. > So, I suppose, you are welcome to start :) > Konstantin > >> >> Dong >>> >>>>> >>>>> >>>>>> rxq->rx_tail = rx_id; >>>>>> >>>>>> /* >>>>>> @@ -1595,6 +1598,8 @@ ixgbe_recv_scattered_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, >>>>>> first_seg = NULL; >>>>>> } >>>>>> >>>>>> + rte_wmb(); >>>>>> + >>>>>> /* >>>>>> * Record index of the next RX descriptor to probe. >>>>>> */ >>>>>> -- >>>>>> 1.9.1 >>>>>
diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c index 9da2c7e..d504688 100644 --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c @@ -1338,6 +1338,9 @@ ixgbe_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, */ rx_pkts[nb_rx++] = rxm; } + + rte_wmb(); + rxq->rx_tail = rx_id; /* @@ -1595,6 +1598,8 @@ ixgbe_recv_scattered_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, first_seg = NULL; } + rte_wmb(); + /* * Record index of the next RX descriptor to probe. */