Re: [PATCH 3/7] net/bonding: change mbuf pool and ring allocation

From: "Sanford, Robert" <rsanford@akamai.com>
To: "Min Hu (Connor)" <humin29@huawei.com>,
	Robert Sanford <rsanford2@gmail.com>,
	"dev@dpdk.org" <dev@dpdk.org>
Cc: "chas3@att.com" <chas3@att.com>
Subject: Re: [PATCH 3/7] net/bonding: change mbuf pool and ring allocation
Date: Tue, 21 Dec 2021 15:31:12 +0000	[thread overview]
Message-ID: <0E88B690-87AD-44A6-BCFA-22F342911E03@akamai.com> (raw)
In-Reply-To: <6afa9788-b349-d2b9-c869-d258b4a6d46f@huawei.com>

Hi Connor,

On 12/20/21, 9:03 PM, "Min Hu (Connor)" <humin29@huawei.com> wrote:

> Hi, Sanford,

> > There is *NO* benefit for the consumer thread (interrupt thread
> > executing tx_machine()) to have caches on per-slave LACPDU pools.
> > The interrupt thread is a control thread, i.e., a non-EAL thread.
> > Its lcore_id is LCORE_ID_ANY, so it has no "default cache" in any
> > mempool.
> Well, sorry, I forgot that interrupt thread is non-EAL thread.

No problem. (I added a temporary rte_log statement in tx_machine
to make sure lcore_id == LCORE_ID_ANY.)

> > There is little or no benefit for active data-plane threads to have
> > caches on per-slave LACPDU pools, because on each pool, the producer
> > thread puts back, at most, one mbuf per second. There is not much
> > contention with the consumer (interrupt thread).
> > 
> > I contend that caches are not necessary for these private LACPDU
> I agree with you.

Thanks.

> > I believe there is a mistake in the ring comments (in 3 places).
> > It would be better if they replace "free" with "full":
> > "... to differentiate a *full* ring from an empty ring."
> > 
> Well, I still can not understand it. I think the ring size is N, it
> should store N items, why "N - 1" items.?
> Hope for your description, thanks.

Here is an excellent article that describes ring buffers, empty vs full, N-1, etc.
https://embedjournal.com/implementing-circular-buffer-embedded-c/#the-full-vs-empty-problem

> >> To fix the bug, how about just setting the flags "RING_F_EXACT_SZ"
> > 
> > Yes, this is a good idea. I will look for examples or test code that
> > use this flag.
> Yes, if fixed, ILGM.

I will use RING_F_EXACT_SZ flag in the next version of the patchset. I did not know about that flag.
	rte_ring_create(... N_PKTS ... RING_F_EXACT_SZ)
... is equivalent to, and looks cleaner than ...
	rte_ring_create(... rte_align32pow2(N_PKTS + 1) ... 0)

I plan to create a separate patchset to update the comments in rte_ring.h,
re RING_F_EXACT_SZ and "free" vs "full".

--
Regards,
Robert Sanford