Re: [dpdk-dev] [PATCH 1/2] mbuf: fix performance/cache resource issue with 128-byte cache line targets

[Jerin Jacob <jerin.jacob@caviumnetworks.com>]

On Mon, Dec 07, 2015 at 03:21:33PM +0000, Ananyev, Konstantin wrote:

Hi Konstantin,

> Hi Jerin,
>
> > -----Original Message-----
> > From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> > Sent: Sunday, December 06, 2015 3:59 PM
> > To: dev@dpdk.org
> > Cc: thomas.monjalon@6wind.com; Richardson, Bruce; olivier.matz@6wind.com; Dumitrescu, Cristian; Ananyev, Konstantin; Jerin
> > Jacob
> > Subject: [dpdk-dev] [PATCH 1/2] mbuf: fix performance/cache resource issue with 128-byte cache line targets
> >
> > No need to split mbuf structure to two cache lines for 128-byte cache line
> > size targets as it can fit on a single 128-byte cache line.
> >
> > Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > ---
> >  app/test/test_mbuf.c                                          | 4 ++++
> >  lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h | 4 ++++
> >  lib/librte_mbuf/rte_mbuf.h                                    | 2 ++
> >  3 files changed, 10 insertions(+)
> >
> > diff --git a/app/test/test_mbuf.c b/app/test/test_mbuf.c
> > index b32bef6..5e21075 100644
> > --- a/app/test/test_mbuf.c
> > +++ b/app/test/test_mbuf.c
> > @@ -930,7 +930,11 @@ test_failing_mbuf_sanity_check(void)
> >  static int
> >  test_mbuf(void)
> >  {
> > +#if RTE_CACHE_LINE_SIZE == 64
> >  	RTE_BUILD_BUG_ON(sizeof(struct rte_mbuf) != RTE_CACHE_LINE_SIZE * 2);
> > +#elif RTE_CACHE_LINE_SIZE == 128
> > +	RTE_BUILD_BUG_ON(sizeof(struct rte_mbuf) != RTE_CACHE_LINE_SIZE);
> > +#endif
> >
> >  	/* create pktmbuf pool if it does not exist */
> >  	if (pktmbuf_pool == NULL) {
> > diff --git a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h b/lib/librte_eal/linuxapp/eal/include/exec-
> > env/rte_kni_common.h
> > index bd1cc09..e724af7 100644
> > --- a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h
> > +++ b/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h
> > @@ -121,8 +121,12 @@ struct rte_kni_mbuf {
> >  	uint32_t pkt_len;       /**< Total pkt len: sum of all segment data_len. */
> >  	uint16_t data_len;      /**< Amount of data in segment buffer. */
> >
> > +#if RTE_CACHE_LINE_SIZE == 64
> >  	/* fields on second cache line */
> >  	char pad3[8] __attribute__((__aligned__(RTE_CACHE_LINE_SIZE)));
> > +#elif RTE_CACHE_LINE_SIZE == 128
> > +	char pad3[24];
> > +#endif
> >  	void *pool;
> >  	void *next;
> >  };
> > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> > index f234ac9..0bf55e0 100644
> > --- a/lib/librte_mbuf/rte_mbuf.h
> > +++ b/lib/librte_mbuf/rte_mbuf.h
> > @@ -813,8 +813,10 @@ struct rte_mbuf {
> >
> >  	uint16_t vlan_tci_outer;  /**< Outer VLAN Tag Control Identifier (CPU order) */
> >
> > +#if RTE_CACHE_LINE_SIZE == 64
> >  	/* second cache line - fields only used in slow path or on TX */
> >  	MARKER cacheline1 __rte_cache_aligned;
> > +#endif
>
> I suppose you'll need to keep same space reserved for first 64B even on systems with 128B cache-line.
> Otherwise we can endup with different mbuf format for systems with 128B cache-line.

Just to understand, Is there any issue in mbuf format being different
across the systems. I think, we are not sending the mbuf over the wire
or sharing with different system etc. right?

Yes, I do understand the KNI dependency with mbuf.

> Another thing - now we have __rte_cache_aligned all over the places, and I don't know is to double
> sizes of all these structures is a good idea.

I thought so, the only concern I have, what if, the struct split to 64
and one cache line is shared between two core/two different structs which have
the different type of operation(most likely). One extensive write and other one
read, The write makes the line dirty start evicting and read core is
going to suffer. Any thoughts?

If its tradeoff between amount memory and performance, I think, it makes sense
to stick the performance in data plane, Hence split option may be not useful?
right?


> Again,  #if RTE_CACHE_LINE_SIZE == 64 ... all over the places looks a bit clumsy.
> Wonder can we have __rte_cache_aligned/ RTE_CACHE_LINE_SIZE architecture specific,

I think, its architecture specific now

> but introduce RTE_CACHE_MIN_LINE_SIZE(==64)/ __rte_cache_min_aligned and used it for mbuf
> (and might be other places).

Yes, it will help in this specific mbuf case and it make sense
if mbuf going to stay within 2 x 64 CL.

AND/OR

can we introduce something like below to reduce the clutter in
other places, macro name is just not correct, trying to share the view

#define rte_cacheline_diff(for_64, for_128)\
do {\
#if RTE_CACHE_LINE_SIZE == 64\
for_64;
#elif RTE_CACHE_LINE_SIZE == 128\
for_128;\
#endif

OR

Typedef struct rte_mbuf to new abstract type and define for 64 bytes and
128 byte

Jerin

> Konstantin
>
>