patches for DPDK stable branches
 help / color / Atom feed
From: "Dumitrescu, Cristian" <cristian.dumitrescu@intel.com>
To: David Marchand <david.marchand@redhat.com>
Cc: "Xu, Ting" <ting.xu@intel.com>, dev <dev@dpdk.org>,
	dpdk stable <stable@dpdk.org>,
	Kevin Traynor <ktraynor@redhat.com>,
	Luca Boccassi <bluca@debian.org>
Subject: Re: [dpdk-stable] [dpdk-dev] [PATCH v4] lib/table: fix cache alignment issue
Date: Wed, 29 Jul 2020 14:53:10 +0000
Message-ID: <BYAPR11MB293542E4B50F98B59075F8ABEB700@BYAPR11MB2935.namprd11.prod.outlook.com> (raw)
In-Reply-To: <CAJFAV8ybYZuokpo3FXBHCD8mdj4OUL2aMGBrHPjVsmKv8HL9vA@mail.gmail.com>



> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Wednesday, July 29, 2020 3:00 PM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Cc: Xu, Ting <ting.xu@intel.com>; dev <dev@dpdk.org>; dpdk stable
> <stable@dpdk.org>; Kevin Traynor <ktraynor@redhat.com>; Luca Boccassi
> <bluca@debian.org>
> Subject: Re: [dpdk-stable] [dpdk-dev] [PATCH v4] lib/table: fix cache
> alignment issue
> 
> On Wed, Jul 29, 2020 at 3:54 PM Dumitrescu, Cristian
> <cristian.dumitrescu@intel.com> wrote:
> > > -----Original Message-----
> > > From: David Marchand <david.marchand@redhat.com>
> > > Sent: Wednesday, July 29, 2020 2:28 PM
> > > To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> > > Cc: Xu, Ting <ting.xu@intel.com>; dev <dev@dpdk.org>; dpdk stable
> > > <stable@dpdk.org>; Kevin Traynor <ktraynor@redhat.com>; Luca
> Boccassi
> > > <bluca@debian.org>
> > > Subject: Re: [dpdk-stable] [dpdk-dev] [PATCH v4] lib/table: fix cache
> > > alignment issue
> > >
> > > On Wed, Jul 29, 2020 at 3:14 PM Dumitrescu, Cristian
> > > <cristian.dumitrescu@intel.com> wrote:
> > > > > 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.
> > >
> > > Yes, and that's my point.
> > > It was not working.
> > > It was not tested.
> > >
> > >
> >
> > Not sure when this code was last tested on 32-bit systems, I'll let the
> validation folks comment on this, but I cannot rule out a change in compiler
> behavior either.
> >
> > This is a low complexity and low impact change, hence low risk IMO.
> 
> Risk is to be evaluated when there is a need.
> I got pinged on this, like it was the end of the times.
> 
> Then I find something that is not worth looking at, hence I am a bit irritated.
> 

I got pinged as well, and I also had to allocate time on this patch. It probably means it is important for somebody.

> And please, for the 2nd time, can you look at my comment below?
> 
Sorry, I missed it first.

> 
> > > > > > 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.
> > >
> > >
> > > What about this comment?
> 
> What about this comment?
> 

You might suspect I also thought about this option. My preference is for the option in the patch for the reasons that IMO it is easier to read and understand the reason for the difference, even though the code is slightly larger. It also leaves the 64-bit code untouched, so it is easier to remove when we finally decide at some point to drop the 32-bit support.

But I can live with the option you describe as well. Thanks for the input.

For me, it would be great if somebody on this list could indicate why the 4-byte padding was not inserted by the compiler automatically, and hence the need for this fix.

> 
> --
> David Marchand


  reply index

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-16 16:27 [dpdk-stable] [PATCH v1] " Ting Xu
2020-06-17  5:43 ` [dpdk-stable] [PATCH v2] " Ting Xu
2020-07-02  8:06   ` [dpdk-stable] [dpdk-dev] " Zhou, JunX W
2020-07-09  1:48 ` [dpdk-stable] [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-stable] [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   ` [dpdk-stable] [dpdk-dev] " David Marchand
2020-07-29 13:13     ` Dumitrescu, Cristian
2020-07-29 13:28       ` David Marchand
2020-07-29 13:54         ` Dumitrescu, Cristian
2020-07-29 13:59           ` David Marchand
2020-07-29 14:53             ` Dumitrescu, Cristian [this message]
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   ` David Marchand

Reply instructions:

You may reply publically 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=BYAPR11MB293542E4B50F98B59075F8ABEB700@BYAPR11MB2935.namprd11.prod.outlook.com \
    --to=cristian.dumitrescu@intel.com \
    --cc=bluca@debian.org \
    --cc=david.marchand@redhat.com \
    --cc=dev@dpdk.org \
    --cc=ktraynor@redhat.com \
    --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

patches for DPDK stable branches

Archives are clonable:
	git clone --mirror http://inbox.dpdk.org/stable/0 stable/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 stable stable/ http://inbox.dpdk.org/stable \
		stable@dpdk.org
	public-inbox-index stable


Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.stable


AGPL code for this site: git clone https://public-inbox.org/ public-inbox