From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 580A8A0518; Thu, 30 Jul 2020 12:35:28 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 374651C023; Thu, 30 Jul 2020 12:35:28 +0200 (CEST) Received: from us-smtp-delivery-1.mimecast.com (us-smtp-1.mimecast.com [207.211.31.81]) by dpdk.org (Postfix) with ESMTP id 1EBE711A2 for ; Thu, 30 Jul 2020 12:35:26 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1596105326; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=eoP8RgAkYupPEK+NC4NTu/Pe01k3O7mVxH+plylS9WE=; b=i4XcPSxKnxS+lF6/4voWG6h6FzTEjj1PDYvnjtCL05P9HvVUyBoFT4RGBs76QOMu3eE4kt jpBmnlulSsA19f4OWVO7sKGPIqFnV80XyvkKnaLjzUcCex9LCqvhPCdsHvmbrdwKpx7MKT eBzoVORJcOU+XEIoCWfMQSm8Nq5CAh8= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-405-8pb5GnpxM8WonYLpc7RtYQ-1; Thu, 30 Jul 2020 06:35:25 -0400 X-MC-Unique: 8pb5GnpxM8WonYLpc7RtYQ-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id B7E16800597; Thu, 30 Jul 2020 10:35:23 +0000 (UTC) Received: from [10.33.36.208] (unknown [10.33.36.208]) by smtp.corp.redhat.com (Postfix) with ESMTP id 39BBC19D7B; Thu, 30 Jul 2020 10:35:18 +0000 (UTC) To: David Marchand , "Dumitrescu, Cristian" Cc: "Xu, Ting" , dev , dpdk stable , Luca Boccassi References: <20200616162705.83575-1-ting.xu@intel.com> <20200722021628.17194-1-ting.xu@intel.com> From: Kevin Traynor X-Pep-Version: 2.0 Message-ID: <9347f349-9334-5ffa-81c3-5f79253278af@redhat.com> Date: Thu, 30 Jul 2020 11:35:17 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: Content-Language: en-US X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: Re: [dpdk-dev] [dpdk-stable] [PATCH v4] lib/table: fix cache alignment issue X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 29/07/2020 14:28, David Marchand wrote: > On Wed, Jul 29, 2020 at 3:14 PM Dumitrescu, Cristian > 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 t= able create function that checks the size of the above structure is 64 byte= s, which caught this issue. >=20 > Yes, and that's my point. > It was not working. > It was not tested. >=20 >=20 > This patch asks for backport in stable branches, I will let Kevin and > Luca comment. >=20 It should be in master first, but then it's a fix so I think it can go to stable if requested and supported by the table maintainer in the event of any (future) regressions. >=20 >> >>> >>> >>> 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. >=20 > That's pahole reporting this. > Maybe the tool extrapolates this attribute based on the next_valid > field placement... I don't know. >=20 >> A packet attribute would explain this issue, i.e. why did the compiler d= ecide not to insert an expected padfing of 4 bytes right after the "next" f= ield, that would allow the field "next_valid" to be aligned to its natural = boundary of 8 bytes. >=20 > Or a 64-bit field on 32-bit has a special alignment that I am not aware o= f. >=20 >=20 >> >>> >>>> >>>> Fixes: 8aa327214c ("table: hash") >>>> Cc: stable@dpdk.org >>>> >>>> Signed-off-by: Ting Xu >>>> >>>> --- >>>> 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. >=20 >=20 > What about this comment? >=20 >=20