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 289F9A052B for ; Wed, 29 Jul 2020 15:28:42 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 69A631C025; Wed, 29 Jul 2020 15:28:41 +0200 (CEST) Received: from us-smtp-1.mimecast.com (us-smtp-delivery-1.mimecast.com [207.211.31.120]) by dpdk.org (Postfix) with ESMTP id B98A010A3 for ; Wed, 29 Jul 2020 15:28:38 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1596029318; 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=j8q/Imi3gxRbZ362wbHWZCmLgGx7fzzyweYSz+gOEC4=; b=evLH2Ddku4NO/JKz29Htz7JaJ20xabJYe3Ycu4l4bJdKux7VjdFwJ/otHAAMrf/SZNWdan K4guo8Ea9duyepKvYwC2u52xaaPDLt1LYP1L0SWbHQ0RYwX8ojkNPL96imqoO33fOCRxBh E3YIX2Ap5K4aL1ynwuQyy97oQexjvx4= Received: from mail-vk1-f197.google.com (mail-vk1-f197.google.com [209.85.221.197]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-447-liMiKhgZOlmMXoNZXOo6pg-1; Wed, 29 Jul 2020 09:28:36 -0400 X-MC-Unique: liMiKhgZOlmMXoNZXOo6pg-1 Received: by mail-vk1-f197.google.com with SMTP id q206so1376228vkb.14 for ; Wed, 29 Jul 2020 06:28:36 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=j8q/Imi3gxRbZ362wbHWZCmLgGx7fzzyweYSz+gOEC4=; b=GapbI3WVox3IfqWTNhEo3q1POayV4g/xn0TxD0wCCL2+kECG/VPdM5JDR1Bv1SHOmx y93fI2Wdyx4NxZs4y9Mqz1W3GXPy/+g0Mek4944Ji4G2On7AW7YKPQ4HmHrgixpr1DXj kLppmNBKSKz8VK5DKAu/yDYY7kKAhA5HlT+nOJ6itgbKuGORCe2kyTbn8RXUUUHVmE7m Y6DNpJu+gaKzvph2SXNaGxjg7+XttxhNkz3AMKBGg+/xkNdr1Dm0jh1UJ63P1JLM4isC NiGzmdZCPKZ9vy6YFCY5xHm+xc02wOgkr4JZJ4zTnJxS7NBAaBxHbjVtO9i8D8Fj00kL RO9w== X-Gm-Message-State: AOAM531L4O/M0GBSCVDFKowD/G0LSmrTKWiK/73Rb1mLnlaHgPNLHwna LOGJKieJgHlD6cZcQ9ZbZthPM07QYGu4j1yF5JK58NB7UkW38lOysSGpaYHMepL9eYyf222Ed2s uvYlb8/mfetdwLiNsEjG5Qt0= X-Received: by 2002:a1f:acc2:: with SMTP id v185mr22963038vke.18.1596029315692; Wed, 29 Jul 2020 06:28:35 -0700 (PDT) X-Google-Smtp-Source: ABdhPJwKVVxZoWunW4Q1Rr3fKhggiPcC+gMardVFAa5LxK4K9zrHrWmxLiBegxEZMbxSf/DJ2vf+suQFBUKclAhe044= X-Received: by 2002:a1f:acc2:: with SMTP id v185mr22963006vke.18.1596029315286; Wed, 29 Jul 2020 06:28:35 -0700 (PDT) MIME-Version: 1.0 References: <20200616162705.83575-1-ting.xu@intel.com> <20200722021628.17194-1-ting.xu@intel.com> In-Reply-To: From: David Marchand Date: Wed, 29 Jul 2020 15:28:24 +0200 Message-ID: To: "Dumitrescu, Cristian" Cc: "Xu, Ting" , dev , dpdk stable , Kevin Traynor , Luca Boccassi X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Subject: Re: [dpdk-stable] [dpdk-dev] [PATCH v4] lib/table: fix cache alignment issue X-BeenThere: stable@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches for DPDK stable branches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: stable-bounces@dpdk.org Sender: "stable" 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 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. This patch asks for backport in stable branches, I will let Kevin and Luca comment. > > > > > > > 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. That's pahole reporting this. Maybe the tool extrapolates this attribute based on the next_valid field placement... I don't know. > 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. Or a 64-bit field on 32-bit has a special alignment that I am not aware of. > > > > > > > > > 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. What about this comment? -- David Marchand