Re: [dpdk-dev] [PATCH v2] mbuf: optimize rte_mbuf_refcnt_update

["Hanoch Haim (hhaim)" <hhaim@cisco.com>]

Hi Bruce,

I'm Hanoch from Cisco Systems works for  the https://github.com/cisco-system-traffic-generator/trex-core traffic generator project.

While upgrading from DPDK 1.8 to 2.2 Ido found that the following commit creates a mbuf corruption and result in Tx hang



commit f20b50b946da9070d21e392e4dbc7d9f68bc983e

Author: Olivier Matz <olivier.matz@6wind.com>

Date:   Mon Jun 8 16:57:22 2015 +0200



Looking at the change it is clear why there is an issue, wanted to get your input.



Init

-----

alloc const mbuf  ==> mbuf-a (ref=1)



Simple case that works

---------------------



thread 1 , tx: alloc-mbuf->attach(mbuf-a) (ref=2)  inc- non atomic

thread 1 , tx: alloc-mbuf->attach(mbuf-a) (ref32)  inc- atomic

thread 1 , drv : free()                    (ref=2) dec- atomic

thread 1 , drv : free()                    (ref=3) dec - non atomic





Simple case that does not work

---------------------



Both do that in parallel



thread 2 tx : alloc-mbuf->attach(mbuf-a)  (ref=2)  inc- non atomic

thread 1 tx : alloc-mbuf->attach(mbuf-a)  (ref=2)  inc- non atomic



  ==> ref in corrupted



thread 1 drv : free()                    (ref=0)   - free

thread 1 drv : free()                    (ref=-1)  - ???





Just as a side note that in our case we could benefit from this optimization by making a small change in our code, as our common case could be alloc/free of simple mbuf and in such scenario there is no atomic operation in both cases. But I think the general case is broken.



Could you explain what was your use case or this optimization? Is it a valid example the aforementioned example



thanks,

Hanoh

Cisco Systems







On Mon, Jun 08, 2015 at 04:57:22PM +0200, Olivier Matz wrote:

> In __rte_pktmbuf_prefree_seg(), there was an optimization to avoid using

> a costly atomic operation when updating the mbuf reference counter if

> its value is 1. Indeed, it means that we are the only owner of the mbuf,

> and therefore nobody can change it at the same time.

>

> We can generalize this optimization directly in rte_mbuf_refcnt_update()

> so the other callers of this function, like rte_pktmbuf_attach(), can

> also take advantage of this optimization.

>

> Signed-off-by: Olivier Matz <olivier.matz at 6wind.com<http://dpdk.org/ml/listinfo/dev>>



Acked-by: Bruce Richardson <bruce.richardson at intel.com<http://dpdk.org/ml/listinfo/dev>>


Hanoh