DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] net/enic: decrement Tx mbuf reference count before recycling
@ 2016-07-08 22:22 John Daley
  2016-07-08 22:22 ` [dpdk-dev] [PATCH] net/enic: increment filter failure counter John Daley
  2016-07-11 10:04 ` [dpdk-dev] [PATCH] net/enic: decrement Tx mbuf reference count before recycling Olivier Matz
  0 siblings, 2 replies; 7+ messages in thread
From: John Daley @ 2016-07-08 22:22 UTC (permalink / raw)
  To: dev; +Cc: bruce.richardson, John Daley

In the Tx cleanup function, the reference count in mbufs to be
returned to the pool should to be decremented before they are
returned. Decrementing is not done by rte_mempool_put_bulk()
so it must be done separately using __rte_pktmbuf_prefree_seg().
If decrementing does not result in a 0 reference count the mbuf
is not returned to the pool and whatever has the last reference
is responsible for freeing.

Fixes: 36935afbc53c ("net/enic: refactor Tx mbuf recycling")
Reviewed-by: Nelson Escobar <neescoba@cisco.com>
Signed-off-by: John Daley <johndale@cisco.com>
---
Since reference counts are set to 0 when mbufs are reallocated from the
pool, and sending packets with reference count not equal to 1 is probably
an application error, this patch may not be critical. But a debug ASSERT
caught it and it would be nice to have it fixed in 16.07.

 drivers/net/enic/enic_rxtx.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/net/enic/enic_rxtx.c b/drivers/net/enic/enic_rxtx.c
index 5ac1d69..96be478 100644
--- a/drivers/net/enic/enic_rxtx.c
+++ b/drivers/net/enic/enic_rxtx.c
@@ -398,7 +398,14 @@ static inline void enic_free_wq_bufs(struct vnic_wq *wq, u16 completed_index)
 	pool = ((struct rte_mbuf *)buf->mb)->pool;
 	for (i = 0; i < nb_to_free; i++) {
 		buf = &wq->bufs[tail_idx];
-		m = (struct rte_mbuf *)(buf->mb);
+		m = __rte_pktmbuf_prefree_seg((struct rte_mbuf *)(buf->mb));
+		buf->mb = NULL;
+
+		if (unlikely(m == NULL)) {
+			tail_idx = enic_ring_incr(desc_count, tail_idx);
+			continue;
+		}
+
 		if (likely(m->pool == pool)) {
 			ENIC_ASSERT(nb_free < ENIC_MAX_WQ_DESCS);
 			free[nb_free++] = m;
@@ -409,7 +416,6 @@ static inline void enic_free_wq_bufs(struct vnic_wq *wq, u16 completed_index)
 			pool = m->pool;
 		}
 		tail_idx = enic_ring_incr(desc_count, tail_idx);
-		buf->mb = NULL;
 	}
 
 	rte_mempool_put_bulk(pool, (void **)free, nb_free);
-- 
2.7.0

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [dpdk-dev] [PATCH] net/enic: increment filter failure counter
  2016-07-08 22:22 [dpdk-dev] [PATCH] net/enic: decrement Tx mbuf reference count before recycling John Daley
@ 2016-07-08 22:22 ` John Daley
  2016-07-15 21:26   ` Thomas Monjalon
  2016-07-11 10:04 ` [dpdk-dev] [PATCH] net/enic: decrement Tx mbuf reference count before recycling Olivier Matz
  1 sibling, 1 reply; 7+ messages in thread
From: John Daley @ 2016-07-08 22:22 UTC (permalink / raw)
  To: dev; +Cc: bruce.richardson, John Daley

One instance of a filter add failure was not incrementing the
the fail counter.

Fixes: 4c2c7bf41f5a ("net/enic: fix negative array index write")
Signed-off-by: John Daley <johndale@cisco.com>
---
 drivers/net/enic/enic_clsf.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/enic/enic_clsf.c b/drivers/net/enic/enic_clsf.c
index 3365176..e6f57be 100644
--- a/drivers/net/enic/enic_clsf.c
+++ b/drivers/net/enic/enic_clsf.c
@@ -218,6 +218,7 @@ int enic_fdir_add_fltr(struct enic *enic, struct rte_eth_fdir_filter *params)
 
 	pos = rte_hash_add_key(enic->fdir.hash, params);
 	if (pos < 0) {
+		enic->fdir.stats.f_add++;
 		dev_err(enic, "Add hash key failed\n");
 		return pos;
 	}
-- 
2.7.0

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH] net/enic: decrement Tx mbuf reference count before recycling
  2016-07-08 22:22 [dpdk-dev] [PATCH] net/enic: decrement Tx mbuf reference count before recycling John Daley
  2016-07-08 22:22 ` [dpdk-dev] [PATCH] net/enic: increment filter failure counter John Daley
@ 2016-07-11 10:04 ` Olivier Matz
  2016-07-11 19:41   ` John Daley (johndale)
  2016-07-11 19:45   ` [dpdk-dev] [PATCH v2] " John Daley
  1 sibling, 2 replies; 7+ messages in thread
From: Olivier Matz @ 2016-07-11 10:04 UTC (permalink / raw)
  To: John Daley, dev; +Cc: bruce.richardson

Hi John,

On 07/09/2016 12:22 AM, John Daley wrote:
> In the Tx cleanup function, the reference count in mbufs to be
> returned to the pool should to be decremented before they are
> returned. Decrementing is not done by rte_mempool_put_bulk()
> so it must be done separately using __rte_pktmbuf_prefree_seg().
> If decrementing does not result in a 0 reference count the mbuf
> is not returned to the pool and whatever has the last reference
> is responsible for freeing.
> 
> Fixes: 36935afbc53c ("net/enic: refactor Tx mbuf recycling")
> Reviewed-by: Nelson Escobar <neescoba@cisco.com>
> Signed-off-by: John Daley <johndale@cisco.com>
> ---
> Since reference counts are set to 0 when mbufs are reallocated from the
> pool, and sending packets with reference count not equal to 1 is probably
> an application error, this patch may not be critical. But a debug ASSERT
> caught it and it would be nice to have it fixed in 16.07.

Sending a packet with refcnt != 1 is not an error. It can happen when
using mbuf clones. So indeed it would be better to have in 16.07.

For the same reason, I also wonder if enic_free_wq_buf() should also be
updated with:

-       rte_mempool_put(mbuf->pool, mbuf);
+       rte_pktmbuf_free(mbuf);


Regards,
Olivier

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH] net/enic: decrement Tx mbuf reference count before recycling
  2016-07-11 10:04 ` [dpdk-dev] [PATCH] net/enic: decrement Tx mbuf reference count before recycling Olivier Matz
@ 2016-07-11 19:41   ` John Daley (johndale)
  2016-07-11 19:45   ` [dpdk-dev] [PATCH v2] " John Daley
  1 sibling, 0 replies; 7+ messages in thread
From: John Daley (johndale) @ 2016-07-11 19:41 UTC (permalink / raw)
  To: Olivier Matz, dev; +Cc: bruce.richardson



> -----Original Message-----
> From: Olivier Matz [mailto:olivier.matz@6wind.com]
> Sent: Monday, July 11, 2016 3:04 AM
> To: John Daley (johndale) <johndale@cisco.com>; dev@dpdk.org
> Cc: bruce.richardson@intel.com
> Subject: Re: [dpdk-dev] [PATCH] net/enic: decrement Tx mbuf reference
> count before recycling
> 
> Hi John,
> 
> On 07/09/2016 12:22 AM, John Daley wrote:
> > In the Tx cleanup function, the reference count in mbufs to be
> > returned to the pool should to be decremented before they are
> > returned. Decrementing is not done by rte_mempool_put_bulk() so it
> > must be done separately using __rte_pktmbuf_prefree_seg().
> > If decrementing does not result in a 0 reference count the mbuf is not
> > returned to the pool and whatever has the last reference is
> > responsible for freeing.
> >
> > Fixes: 36935afbc53c ("net/enic: refactor Tx mbuf recycling")
> > Reviewed-by: Nelson Escobar <neescoba@cisco.com>
> > Signed-off-by: John Daley <johndale@cisco.com>
> > ---
> > Since reference counts are set to 0 when mbufs are reallocated from
> > the pool, and sending packets with reference count not equal to 1 is
> > probably an application error, this patch may not be critical. But a
> > debug ASSERT caught it and it would be nice to have it fixed in 16.07.
> 
> Sending a packet with refcnt != 1 is not an error. It can happen when using
> mbuf clones. So indeed it would be better to have in 16.07.
> 
> For the same reason, I also wonder if enic_free_wq_buf() should also be
> updated with:
> 
> -       rte_mempool_put(mbuf->pool, mbuf);
> +       rte_pktmbuf_free(mbuf);
That is a very good point, thank you. I'll use rte_pktmubf_free_seg(mbuf) though, since we are walking an array of all mbuf segments. V2 coming momentarily.
-john
> 
> 
> Regards,
> Olivier

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [dpdk-dev] [PATCH v2] net/enic: decrement Tx mbuf reference count before recycling
  2016-07-11 10:04 ` [dpdk-dev] [PATCH] net/enic: decrement Tx mbuf reference count before recycling Olivier Matz
  2016-07-11 19:41   ` John Daley (johndale)
@ 2016-07-11 19:45   ` John Daley
  2016-07-15 21:27     ` Thomas Monjalon
  1 sibling, 1 reply; 7+ messages in thread
From: John Daley @ 2016-07-11 19:45 UTC (permalink / raw)
  To: dev; +Cc: bruce.richardson, olivier.matz, John Daley

In the burst Tx cleanup function, the reference count in mbufs
returned to the pool should to be decremented before they are
returned. Decrementing is not done by rte_mempool_put_bulk()
so it must be done separately using __rte_pktmbuf_prefree_seg().

Also when returning unsent buffers when the device is stopped
use rte_mbuf_free_seg() instead of rte_mempool_put() so that
reference counts are properly decremented.

Fixes: 36935afbc53c ("net/enic: refactor Tx mbuf recycling")

Reviewed-by: Nelson Escobar <neescoba@cisco.com>
Signed-off-by: John Daley <johndale@cisco.com>
---
v2: Use rte_pktmbuf_free_seg when returning mbufs when the device is
stoped. Suggested by Olivier Matz <olivier.matz@6wind.com>.

 drivers/net/enic/enic_main.c |  2 +-
 drivers/net/enic/enic_rxtx.c | 10 ++++++++--
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/net/enic/enic_main.c b/drivers/net/enic/enic_main.c
index d8669cc..54aaa25 100644
--- a/drivers/net/enic/enic_main.c
+++ b/drivers/net/enic/enic_main.c
@@ -106,7 +106,7 @@ static void enic_free_wq_buf(struct vnic_wq_buf *buf)
 {
 	struct rte_mbuf *mbuf = (struct rte_mbuf *)buf->mb;
 
-	rte_mempool_put(mbuf->pool, mbuf);
+	rte_pktmbuf_free_seg(mbuf);
 	buf->mb = NULL;
 }
 
diff --git a/drivers/net/enic/enic_rxtx.c b/drivers/net/enic/enic_rxtx.c
index 2f4a08c..845a8e6 100644
--- a/drivers/net/enic/enic_rxtx.c
+++ b/drivers/net/enic/enic_rxtx.c
@@ -398,7 +398,14 @@ static inline void enic_free_wq_bufs(struct vnic_wq *wq, u16 completed_index)
 	pool = ((struct rte_mbuf *)buf->mb)->pool;
 	for (i = 0; i < nb_to_free; i++) {
 		buf = &wq->bufs[tail_idx];
-		m = (struct rte_mbuf *)(buf->mb);
+		m = __rte_pktmbuf_prefree_seg((struct rte_mbuf *)(buf->mb));
+		buf->mb = NULL;
+
+		if (unlikely(m == NULL)) {
+			tail_idx = enic_ring_incr(desc_count, tail_idx);
+			continue;
+		}
+
 		if (likely(m->pool == pool)) {
 			RTE_ASSERT(nb_free < ENIC_MAX_WQ_DESCS);
 			free[nb_free++] = m;
@@ -409,7 +416,6 @@ static inline void enic_free_wq_bufs(struct vnic_wq *wq, u16 completed_index)
 			pool = m->pool;
 		}
 		tail_idx = enic_ring_incr(desc_count, tail_idx);
-		buf->mb = NULL;
 	}
 
 	rte_mempool_put_bulk(pool, (void **)free, nb_free);
-- 
2.7.0

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH] net/enic: increment filter failure counter
  2016-07-08 22:22 ` [dpdk-dev] [PATCH] net/enic: increment filter failure counter John Daley
@ 2016-07-15 21:26   ` Thomas Monjalon
  0 siblings, 0 replies; 7+ messages in thread
From: Thomas Monjalon @ 2016-07-15 21:26 UTC (permalink / raw)
  To: John Daley; +Cc: dev, bruce.richardson

2016-07-08 15:22, John Daley:
> One instance of a filter add failure was not incrementing the
> the fail counter.
> 
> Fixes: 4c2c7bf41f5a ("net/enic: fix negative array index write")
> Signed-off-by: John Daley <johndale@cisco.com>

Applied, thanks

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH v2] net/enic: decrement Tx mbuf reference count before recycling
  2016-07-11 19:45   ` [dpdk-dev] [PATCH v2] " John Daley
@ 2016-07-15 21:27     ` Thomas Monjalon
  0 siblings, 0 replies; 7+ messages in thread
From: Thomas Monjalon @ 2016-07-15 21:27 UTC (permalink / raw)
  To: John Daley; +Cc: dev, bruce.richardson, olivier.matz

2016-07-11 12:45, John Daley:
> In the burst Tx cleanup function, the reference count in mbufs
> returned to the pool should to be decremented before they are
> returned. Decrementing is not done by rte_mempool_put_bulk()
> so it must be done separately using __rte_pktmbuf_prefree_seg().
> 
> Also when returning unsent buffers when the device is stopped
> use rte_mbuf_free_seg() instead of rte_mempool_put() so that
> reference counts are properly decremented.
> 
> Fixes: 36935afbc53c ("net/enic: refactor Tx mbuf recycling")
> 
> Reviewed-by: Nelson Escobar <neescoba@cisco.com>
> Signed-off-by: John Daley <johndale@cisco.com>

Applied, thanks

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2016-07-15 21:27 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-08 22:22 [dpdk-dev] [PATCH] net/enic: decrement Tx mbuf reference count before recycling John Daley
2016-07-08 22:22 ` [dpdk-dev] [PATCH] net/enic: increment filter failure counter John Daley
2016-07-15 21:26   ` Thomas Monjalon
2016-07-11 10:04 ` [dpdk-dev] [PATCH] net/enic: decrement Tx mbuf reference count before recycling Olivier Matz
2016-07-11 19:41   ` John Daley (johndale)
2016-07-11 19:45   ` [dpdk-dev] [PATCH v2] " John Daley
2016-07-15 21:27     ` Thomas Monjalon

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).