[dpdk-dev] net/enic: decrement Tx mbuf reference count before recycling
Commit Message
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(-)
Comments
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
> -----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
@@ -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);