From: "Dumitrescu, Cristian" <cristian.dumitrescu@intel.com> To: David Marchand <david.marchand@redhat.com>, "Xu, Ting" <ting.xu@intel.com> Cc: dev <dev@dpdk.org>, dpdk stable <stable@dpdk.org> Subject: Re: [dpdk-dev] [PATCH v4] lib/table: fix cache alignment issue Date: Wed, 29 Jul 2020 13:13:57 +0000 Message-ID: <BYAPR11MB29352D566CB43BB6D5752361EB700@BYAPR11MB2935.namprd11.prod.outlook.com> (raw) In-Reply-To: <CAJFAV8zZRQEjVGp07NzuraHuA9YWWK_6VZZ5hN69w2GQuFeTeQ@mail.gmail.com> > -----Original Message----- > From: David Marchand <david.marchand@redhat.com> > Sent: Wednesday, July 29, 2020 1:01 PM > To: Xu, Ting <ting.xu@intel.com>; Dumitrescu, Cristian > <cristian.dumitrescu@intel.com> > Cc: dev <dev@dpdk.org>; dpdk stable <stable@dpdk.org> > Subject: Re: [dpdk-dev] [PATCH v4] lib/table: fix cache alignment issue > > On Wed, Jul 22, 2020 at 4:13 AM Ting Xu <ting.xu@intel.com> wrote: > > > > When create softnic hash table with 16 keys, it failed on 32-bit > > environment, because the pointer field in structure rte_bucket_4_16 > > is only 32 bits. Add a padding field in 32-bit environment to keep > > the structure to a multiple of 64 bytes. Apply this to 8-byte and > > 32-byte key hash function as well. > > Please correct me if I am wrong, but it simply means this part of the > table library never worked for 32-bit. > It seems more adding 32-bit support rather than a fix and then I > wonder if it has its place in rc3. > Functionally. the code works, but performance is affected. The only thing that prevents the code from working is the check in the table create function that checks the size of the above structure is 64 bytes, which caught this issue. > > > Now, looking at the details: > > For 64-bit on my x86, we have: > > struct rte_bucket_4_8 { > uint64_t signature; /* 0 8 */ > uint64_t lru_list; /* 8 8 */ > struct rte_bucket_4_8 * next; /* 16 8 */ > uint64_t next_valid; /* 24 8 */ > uint64_t key[4]; /* 32 32 */ > /* --- cacheline 1 boundary (64 bytes) --- */ > uint8_t data[]; /* 64 0 */ > > /* size: 64, cachelines: 1, members: 6 */ > }; > > > For 32-bit, we have: > > struct rte_bucket_4_8 { > uint64_t signature; /* 0 8 */ > uint64_t lru_list; /* 8 8 */ > struct rte_bucket_4_8 * next; /* 16 4 */ > uint64_t next_valid; /* 20 8 */ > uint64_t key[4]; /* 28 32 */ > uint8_t data[]; /* 60 0 */ > > /* size: 60, cachelines: 1, members: 6 */ > /* last cacheline: 60 bytes */ > } __attribute__((__packed__)); > > ^^ it is interesting that a packed attribute ends up here. > I saw no such attribute in the library code. > Compiler black magic at work I guess... > Where do you see the packet attribute? I don't see it in the code. A packet attribute would explain this issue, i.e. why did the compiler decide not to insert an expected padfing of 4 bytes right after the "next" field, that would allow the field "next_valid" to be aligned to its natural boundary of 8 bytes. > > > > > Fixes: 8aa327214c ("table: hash") > > Cc: stable@dpdk.org > > > > Signed-off-by: Ting Xu <ting.xu@intel.com> > > > > --- > > v3->v4: Change design based on comment > > v2->v3: Rebase > > v1->v2: Correct patch time > > --- > > lib/librte_table/rte_table_hash_key16.c | 17 +++++++++++++++++ > > lib/librte_table/rte_table_hash_key32.c | 17 +++++++++++++++++ > > lib/librte_table/rte_table_hash_key8.c | 16 ++++++++++++++++ > > 3 files changed, 50 insertions(+) > > > > diff --git a/lib/librte_table/rte_table_hash_key16.c > b/lib/librte_table/rte_table_hash_key16.c > > index 2cca1c924..c4384b114 100644 > > --- a/lib/librte_table/rte_table_hash_key16.c > > +++ b/lib/librte_table/rte_table_hash_key16.c > > @@ -33,6 +33,7 @@ > > > > #endif > > > > +#ifdef RTE_ARCH_64 > > struct rte_bucket_4_16 { > > /* Cache line 0 */ > > uint64_t signature[4 + 1]; > > @@ -46,6 +47,22 @@ struct rte_bucket_4_16 { > > /* Cache line 2 */ > > uint8_t data[0]; > > }; > > +#else > > +struct rte_bucket_4_16 { > > + /* Cache line 0 */ > > + uint64_t signature[4 + 1]; > > + uint64_t lru_list; > > + struct rte_bucket_4_16 *next; > > + uint32_t pad; > > + uint64_t next_valid; > > + > > + /* Cache line 1 */ > > + uint64_t key[4][2]; > > + > > + /* Cache line 2 */ > > + uint8_t data[0]; > > +}; > > +#endif > > The change could simply be: > > @@ -38,6 +38,9 @@ struct rte_bucket_4_16 { > uint64_t signature[4 + 1]; > uint64_t lru_list; > struct rte_bucket_4_16 *next; > +#ifndef RTE_ARCH_64 > + uint32_t pad; > +#endif > uint64_t next_valid; > > /* Cache line 1 */ > > It avoids duplicating the whole structure definition (we could miss > updating one side of the #ifdef later). > Idem for the other "8" and "32" structures. > > > -- > David Marchand
next prev parent reply other threads:[~2020-07-29 13:14 UTC|newest] Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-06-16 16:27 [dpdk-dev] [PATCH v1] " Ting Xu 2020-06-17 5:43 ` [dpdk-dev] [PATCH v2] " Ting Xu 2020-07-02 8:06 ` Zhou, JunX W 2020-07-09 1:48 ` [dpdk-dev] [PATCH v3] " Ting Xu 2020-07-20 14:37 ` Dumitrescu, Cristian 2020-07-21 5:15 ` Xu, Ting 2020-07-21 21:16 ` Dumitrescu, Cristian 2020-07-22 2:16 ` Xu, Ting 2020-07-22 2:16 ` [dpdk-dev] [PATCH v4] " Ting Xu 2020-07-22 8:26 ` Dumitrescu, Cristian 2020-07-22 8:30 ` Xu, Ting 2020-07-22 8:49 ` Dumitrescu, Cristian 2020-07-22 8:48 ` Dumitrescu, Cristian 2020-07-29 12:01 ` David Marchand 2020-07-29 13:13 ` Dumitrescu, Cristian [this message] 2020-07-29 13:28 ` [dpdk-dev] [dpdk-stable] " David Marchand 2020-07-29 13:54 ` Dumitrescu, Cristian 2020-07-29 13:59 ` David Marchand 2020-07-29 14:53 ` Dumitrescu, Cristian 2020-07-30 6:57 ` Xu, Ting 2020-07-30 10:35 ` Kevin Traynor 2020-09-09 6:18 ` Xu, Ting 2020-09-15 8:03 ` David Marchand 2020-10-14 8:26 ` Xu, Ting 2020-10-14 13:53 ` [dpdk-dev] " David Marchand
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=BYAPR11MB29352D566CB43BB6D5752361EB700@BYAPR11MB2935.namprd11.prod.outlook.com \ --to=cristian.dumitrescu@intel.com \ --cc=david.marchand@redhat.com \ --cc=dev@dpdk.org \ --cc=stable@dpdk.org \ --cc=ting.xu@intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
DPDK patches and discussions This inbox may be cloned and mirrored by anyone: git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git # If you have public-inbox 1.1+ installed, you may # initialize and index your mirror using the following commands: public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \ dev@dpdk.org public-inbox-index dev Example config snippet for mirrors. Newsgroup available over NNTP: nntp://inbox.dpdk.org/inbox.dpdk.dev AGPL code for this site: git clone https://public-inbox.org/public-inbox.git