RE: [dpdk-dev] [PATCH v6 2/5] mbuf: detach mbuf with pinned externalbuffer

["Morten Brørup" <mb@smartsharesystems.com>]

Triggered by another discussion, I have identified a potential bug related to this patch.

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Viacheslav
> Ovsiienko
> Sent: Monday, 20 January 2020 20.16
> 
> Update detach routine to check the mbuf pool type.
> Introduce the special internal version of detach routine to handle
> the special case of pinned external bufferon mbuf freeing.
> 
> Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> Acked-by: Olivier Matz <olivier.matz@6wind.com>
> ---

[...]

> @@ -1201,8 +1278,13 @@ static inline void rte_pktmbuf_detach(struct
> rte_mbuf *m)
> 
>  	if (likely(rte_mbuf_refcnt_read(m) == 1)) {
> 
> -		if (!RTE_MBUF_DIRECT(m))
> -			rte_pktmbuf_detach(m);
> +		if (!RTE_MBUF_DIRECT(m)) {
> +			if (!RTE_MBUF_HAS_EXTBUF(m) ||
> +			    !RTE_MBUF_HAS_PINNED_EXTBUF(m))
> +				rte_pktmbuf_detach(m);
> +			else if (__rte_pktmbuf_pinned_extbuf_decref(m))
> +				return NULL;

When NULL is returned here, m->refcnt is still 1.

> +		}
> 
>  		if (m->next != NULL) {
>  			m->next = NULL;
> @@ -1213,8 +1295,13 @@ static inline void rte_pktmbuf_detach(struct
> rte_mbuf *m)
> 
>  	} else if (__rte_mbuf_refcnt_update(m, -1) == 0) {
> 
> -		if (!RTE_MBUF_DIRECT(m))
> -			rte_pktmbuf_detach(m);
> +		if (!RTE_MBUF_DIRECT(m)) {
> +			if (!RTE_MBUF_HAS_EXTBUF(m) ||
> +			    !RTE_MBUF_HAS_PINNED_EXTBUF(m))
> +				rte_pktmbuf_detach(m);
> +			else if (__rte_pktmbuf_pinned_extbuf_decref(m))
> +				return NULL;

When NULL is returned here, m->refcnt has been decremented to 0.

I don't know which is correct, but I suppose m->refcnt should end up with the same value in both cases?

> +		}
> 
>  		if (m->next != NULL) {
>  			m->next = NULL;
> --
> 1.8.3.1
>