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 DF4ABA0577 for ; Tue, 7 Apr 2020 19:14:19 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id D10DD1BEE0; Tue, 7 Apr 2020 19:14:19 +0200 (CEST) Received: from us-smtp-delivery-1.mimecast.com (us-smtp-2.mimecast.com [207.211.31.81]) by dpdk.org (Postfix) with ESMTP id 65DE31BF6C for ; Tue, 7 Apr 2020 19:14:18 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1586279657; 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: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:autocrypt:autocrypt; bh=+eJBVKBdyh2Duu3cNsXfUzLyhQHp8Z5AMhVMm9LLXws=; b=IZYK7go4nsxWugzwQMv4edgEr6fkaJeKm9vvbGMa6FNVZFfqXrHRpJT65HdtrsFhUl53t+ 00MqFiW2ue8Shj0AMfgpAdMdAMyd1dVCeVpN4FIMNsryWhIjo6v2iFOLiPYruavO1A0/Rm zS4+Q3tbtjAVkkqq/95RLO/193mGI10= 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-231-71tkmfKyNNStPrbBDgn1lg-1; Tue, 07 Apr 2020 13:13:49 -0400 X-MC-Unique: 71tkmfKyNNStPrbBDgn1lg-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 24CAB1005509; Tue, 7 Apr 2020 17:13:47 +0000 (UTC) Received: from [10.33.36.51] (unknown [10.33.36.51]) by smtp.corp.redhat.com (Postfix) with ESMTP id C57B360BE1; Tue, 7 Apr 2020 17:13:38 +0000 (UTC) To: Gavin Hu , Bruce Richardson , =?UTF-8?Q?Morten_Br=c3=b8rup?= Cc: Ferruh Yigit , "dev@dpdk.org" , nd , "david.marchand@redhat.com" , "thomas@monjalon.net" , "jerinj@marvell.com" , Honnappa Nagarahalli , Ruifeng Wang , Phil Yang , Joyce Kong , "stable@dpdk.org" , Olivier MATZ , Konstantin Ananyev , Andrew Rybchenko References: <20200303162728.93744-1-gavin.hu@arm.com> <20200307155629.45021-1-gavin.hu@arm.com> <4135ab73-75d3-421a-264d-2951fc096133@intel.com> <98CBD80474FA8B44BF855DF32C47DC35C60EA3@smartserver.smartshare.dk> <98CBD80474FA8B44BF855DF32C47DC35C60EAC@smartserver.smartshare.dk> <20200311120739.GA1922@bricha3-MOBL.ger.corp.intel.com> From: Kevin Traynor Autocrypt: addr=ktraynor@redhat.com; keydata= mQINBF2J2awBEADUEPNhgNI+nJNgiTAUcw4YIgVXEoHlsNPyyzG1BEXkWXALy0Y3fNTiw6+r ltWDkF9jzL9kfkecgQ67itGfk1OaBXgSGKuw1PUpxAwX2Bi76LAR6M5OsyGM9TSVVQwARalz hMwRBIZPzPc7or6Pw7jAOJ8SQGJ1Zlp1YJCjrvpe87V1tH/LY8Wnxn/EuoseFmWILAQZAtYS tGjcrAgYn3SPMLR1B0BP5bTBY06vWQjiufH8drenfDnMJAzuBdG1mqjnTqCjULZ3Hunv4xqZ aMnkvL/K5Tj1c12Oe4930EE53LrXIBUltRg5mBudSWHnC7twjH0082HH9f963Z/2UI63SFIT iUvRvAzJYytgy7XnWLQ0+goZBADKYfolOuC0H8VgCaux8u8KFF28Dy+N6TV2KI58jTlyg1Zu l7QwykZpnOkJFiy37Gfbu3YEOzO72cP/S7/A+zvuqkxi63jyEkd+FY99vLt/HN2MUZwRmKDw UPbLkmrs8WU01/POVsqDcfvz7vu2St8hqqTiSIdQGS2zyTKB2/DvPSM3jws3udkIYSuhn+X4 QBiV6lkVZ7DSE6a065gnAauAql+b32Eymy+xnG5jCt1tR+0Cp2VZYCR9OU2gmomUKBDoX/He pSgED01CqYPNjN+TddirwmQX7ep4DtXc8FWvv2g/pq9WZFQk2QARAQABtCNLZXZpbiBUcmF5 bm9yIDxrdHJheW5vckByZWRoYXQuY29tPokCTgQTAQgAOBYhBAoiOaH51tHF7VYtEI9CINER a+yJBQJdidmsAhsDBQsJCAcCBhUKCQgLAgQWAgMBAh4BAheAAAoJEI9CINERa+yJoxIP/3VF 2TIgW4ckxhRFCvFu/606bnvCPie88ake4uWVWMAWwcMc4fKEltRWRCpkSVOwgqoMHnyHxK5r kOKzx2CLJMX5TgTMfKzPuaBDHngHLUzl2DStpBzrod0cVg5TShdmmfjY61uxRJKz+DlSkwgJ riADdVF5PPosQXTkKSGf2ombpTGpx/pue9ocjnr3x4SDpRLlnooM6Jf/3Y3Ib4jX6HPEyWuY b+owIIk9y2nRRGPQ6jbqAhsrXd9V+77UL0QuGWloMuKMZFbNg8hbu7X5aFijAbfxj4YUgojS ba7gfGZQan8h32A9KGQWrmsCBc3j2GqEPsX0r05X7cn7WL6IOPgQJ5EiQ7PlazQYVLrvZg9B n0GKK0k6895mLG0ZZ5v/qajOPF52etSmvFD1WUPb4OqaHqGA9ZtMpaKFRt7Y6rpXqKNU1xzW F5KjbTPtTb9WF3An8dciVv+AYUI7totkZYkWvQtgss8lfaX3NKUvXLVxqK0z3dQyr7rF/tYz PneTKypSksjCgaEBLSrsRmM5zKfe7tSNF/fDntfIq/029Jtcw29TcWEP57peNu6TtejewQD9 sTI+oqiXvW2D5l7LNUDYG8eMJp2oT7I0ZSBRvwcbmjH0DtN/bXCCFfCvk8Yic68F3tV1ctix wQARVKDBhT30uCxycRWojCYqTgNJJS71uQINBF2J2awBEADP57PR2IpSYBeNSrsAjeIcsahE N4SQP2C4s50S8QEWAUhqMRI7WNv5cfeef0nDvcl1IUA6oz5SokbcsbMa+mRgaNF4N5KikWTO LPYxq2YVJoXwJ+tKmNzyOLFUIfFJ4NBJZple5dTfWzD00Dbb19Mri1hy1mWMqNTPGBee1+hw Qcp6n3mmGECvajs8G5A7NyXbwL8ihN7HX9D01ucD62b4G03yKe2g/hvKgcdUVmhCldJlF27I 2fSR9tDxH9pZqRODY4rjbFZEey/vWKXqjE+DQ8AtMSEaDfFe5D+i4Aw6erWQ3Wr+DwZt1/7G dIAElGA/q90T1ENVwJX9y7fsQssawKYYdDqURHCl5JuDXI+VXUypExipUUT5SPycMmbLsx0D iKEqPPDQWKxkIDVKqj2+EhamSuJznZUwBLJKn0h4zrIWiXWUy07lRwtVuhaDXhF3GfW+5W/x wAg7Qg3w00ASsb/XTHBIhMnenKDfS7ihtQA8SacwX8ySdxb+15XPyiplM979qBQ0mhnilulm MIJzEf/JxoYR5huuj4f1PFqqrsP06Dl+YGB7dQZp3IKggS5c3/TAynARRg9N89UsDXNtp7X0 tgIPFF5k6fnHE0J5O64GYHeTqN/1aE6dAEOV9WrGzQAJxU9ipikb8jKAWXzLewRIKGmoPcRZ WdB0NmIjmQARAQABiQI2BBgBCAAgFiEECiI5ofnW0cXtVi0Qj0Ig0RFr7IkFAl2J2awCGwwA CgkQj0Ig0RFr7IkkORAAl/NbX93WK5MEoRw7/DaPTo/Lo6Pj1XMeSqGyACigHK/452UDvlEH NjNJMzYYrNIjMtEmN9VVCfjT38CSca7mpGQVwchc0mC7QSPAETLCS+UacVf/Kwxz5FfkEUUw UT7A+uyVOIgW3d9ldlRzkHA2czonSSgTQU+i2g6DM4ha+BuQb4byAXH6HQHt/Zh1J64z0ohH v6iGsCzCY/sMWF8+LEGSnzMGRCLiiwSF0vJBHbzWK68fANaF4gBV0Z/+6tQRFN7YMhj/INmk qgvHj1ZzHFNtirjMGPRxoZs51YoLQM/aBPxKrnmXThx1ufH+0L6sGmFTugiDt0XSEkC5reH7 a+VhQ1VTFFQrClA8NmDSPzFeuhru4ryaaDHO+uEB16cNHxHrQtlP/2hts2JM5lwkZRWJ5A57 h8eDEIK5be47T85NVHfuTaboNRmgg1HygVejhGUtt69u/0MVRg/roUTa0FyEbNsvz4qAecyW yWzMcVrcGJDQLC9JLKEpoyUF6gdTKaiDL2Vao4+XRIA3Y57b6MO35a3HuzAv7+i5Z0mnDEJO XxXqTOmKYpMIGexzM/PtuA0712sT1abG9tAJ17ao/B7cqMW5IkKkalemFbWfI2unns4Papvo tk9igVqyp6EJDU98z5TJioCVojwK2laDaoIjTJk9YYv3iwCsqPd5feU= Message-ID: <30507744-63d4-c24b-4cc3-de7adff871f6@redhat.com> Date: Tue, 7 Apr 2020 18:13:37 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.5.0 MIME-Version: 1.0 In-Reply-To: Content-Language: en-US X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=WINDOWS-1252 Content-Transfer-Encoding: quoted-printable Subject: Re: [dpdk-stable] [dpdk-dev] [PATCH v2] mbuf: replace zero-length marker with unnamed union 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 13/03/2020 09:22, Gavin Hu wrote: > Hi Bruce, >=20 >> -----Original Message----- >> From: Bruce Richardson >> Sent: Wednesday, March 11, 2020 8:08 PM >> To: Morten Br=F8rup >> Cc: Gavin Hu ; Ferruh Yigit ; >> dev@dpdk.org; nd ; david.marchand@redhat.com; >> thomas@monjalon.net; ktraynor@redhat.com; jerinj@marvell.com; >> Honnappa Nagarahalli ; Ruifeng Wang >> ; Phil Yang ; Joyce Kong >> ; stable@dpdk.org; Olivier MATZ >> ; Konstantin Ananyev >> ; Andrew Rybchenko >> >> Subject: Re: [dpdk-dev] [PATCH v2] mbuf: replace zero-length marker with >> unnamed union >> >> On Wed, Mar 11, 2020 at 10:04:33AM +0100, Morten Br=F8rup wrote: >>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Gavin Hu >>>> Sent: Wednesday, March 11, 2020 8:50 AM >>>> >>>> Hi Morten, >>>> >>>>> From: Morten Br=F8rup >>>>> Sent: Monday, March 9, 2020 9:31 PM >>>>> >>>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ferruh Yigit >>>>>> Sent: Monday, March 9, 2020 12:30 PM >>>>>> >>>>>> On 3/9/2020 9:45 AM, Gavin Hu wrote: >>>>>>> Hi Ferruh, >>>>>>> >>>>>>>> -----Original Message----- >>>>>>>> From: Ferruh Yigit >>>>>>>> Sent: Monday, March 9, 2020 4:55 PM >>>>>>>> >>>>>>>> On 3/7/2020 3:56 PM, Gavin Hu wrote: >>>>>>>>> Declaring zero-length arrays in other contexts, including as >>>>>> interior >>>>>>>>> members of structure objects or as non-member objects, is >>>>>> discouraged. >>>>>>>>> Accessing elements of zero-length arrays declared in such >>>> contexts >>>>>> is >>>>>>>>> undefined and may be diagnosed.[1] >>>>>>>>> >>>>>>>>> Fix by using unnamed union and struct. >>>>>>>>> >>>>>>>>> https://bugs.dpdk.org/show_bug.cgi?id=3D396 >>>>>>>>> >>>>>>>>> Bugzilla ID: 396 >>>>>>>>> >>>>>>>>> [1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html >>>>>>>>> >>>>>>>>> Fixes: 3e6181b07038 ("mbuf: use structure marker from EAL") >>>>>>>>> Cc: stable@dpdk.org >>>>>>>>> >>>>>>>>> Signed-off-by: Gavin Hu >>>>>>>>> --- >>>>>>>>> v2: >>>>>>>>> * change 'uint64_t rearm_data' to 'uint_64_t rearm_data[1]' to >>>> fix >>>>>>>>> the SFC PMD compiling error on x86. >>>>>>>>> --- >>>>>>>>> lib/librte_mbuf/rte_mbuf_core.h | 54 +++++++++++++++++++---- >> -- >>>> ---- >>>>>> ---- >>>>>>>>> 1 file changed, 32 insertions(+), 22 deletions(-) >>>>>>>>> >>>>>>>>> diff --git a/lib/librte_mbuf/rte_mbuf_core.h >>>>>>>> b/lib/librte_mbuf/rte_mbuf_core.h >>>>>>>>> index b9a59c879..34cb152e2 100644 >>>>>>>>> --- a/lib/librte_mbuf/rte_mbuf_core.h >>>>>>>>> +++ b/lib/librte_mbuf/rte_mbuf_core.h >>>>>>>>> @@ -480,31 +480,41 @@ struct rte_mbuf { >>>>>>>>> =09=09rte_iova_t buf_physaddr; /**< deprecated */ >>>>>>>>> =09} __rte_aligned(sizeof(rte_iova_t)); >>>>>>>>> >>>>>>>>> -=09/* next 8 bytes are initialised on RX descriptor rearm */ >>>>>>>>> -=09RTE_MARKER64 rearm_data; >>>>>>>>> -=09uint16_t data_off; >>>>>>>>> - >>>>>>>>> -=09/** >>>>>>>>> -=09 * Reference counter. Its size should at least equal to the >>>> size >>>>>>>>> -=09 * of port field (16 bits), to support zero-copy broadcast. >>>>>>>>> -=09 * It should only be accessed using the following >>>> functions: >>>>>>>>> -=09 * rte_mbuf_refcnt_update(), rte_mbuf_refcnt_read(), and >>>>>>>>> -=09 * rte_mbuf_refcnt_set(). The functionality of these >>>> functions >>>>>> (atomic, >>>>>>>>> -=09 * or non-atomic) is controlled by the >>>>>>>> CONFIG_RTE_MBUF_REFCNT_ATOMIC >>>>>>>>> -=09 * config option. >>>>>>>>> -=09 */ >>>>>>>>> =09RTE_STD_C11 >>>>>>>>> =09union { >>>>>>>>> -=09=09rte_atomic16_t refcnt_atomic; /**< Atomically >>>> accessed >>>>>>>> refcnt */ >>>>>>>>> -=09=09/** Non-atomically accessed refcnt */ >>>>>>>>> -=09=09uint16_t refcnt; >>>>>>>>> -=09}; >>>>>>>>> -=09uint16_t nb_segs; /**< Number of segments. */ >>>>>>>>> +=09=09/* next 8 bytes are initialised on RX descriptor >>>> rearm */ >>>>>>>>> +=09=09uint64_t rearm_data[1]; >>>>>>>> We are using zero length array as markers only and know what we >>>> are >>>>>> doing >>>>>>>> with them, >>>>>>>> what would you think disabling the warning instead of increasing >>>> the >>>>>>>> complexity >>>>>>>> in mbuf struct? >>>>>>> Okay, I will add -Wno-zero-length-bounds to the compiler >>>> toolchain >>>>>> flags. >>>>>> >>>>>> This would be my preference but I would like to get more input, can >>>> you >>>>>> please >>>>>> for more comments before changing the implementation in case there >>>> are >>>>>> some >>>>>> strong opinion on it? >>>>>> >>>>> >>>>> I have some input to this discussion. >>>>> >>>>> Let me repeat what Gavin's GCC reference states: Declaring zero- >>>> length >>>>> arrays [...] as interior members of structure objects [...] is >>>> discouraged. >>>>> >>>>> Why would we do something that the compiler documentation says is >>>>> discouraged? I think the problem (i.e. using discouraged techniques) >>>> should >>>>> be fixed, not the symptom (i.e. getting warnings about using >>>> discouraged >>>>> techniques). >>>>> >>>>> Compiler warnings are here to help, and in my experience they are >>>> actually >>>>> very helpful, although avoiding them often requires somewhat more >>>>> verbose source code. Disabling this warning not only affects this >>>> file, but >>>>> disables warnings about potential bugs in other source code too. >>>>> >>>>> Generally, disabling compiler warnings is a slippery slope. It would >>>> be >>>>> optimal if DPDK could be compiled with -Wall, and it would probably >>>> reduce >>>>> the number of released bugs too. >>>>> >>>>> With that said, sometimes the optimal solution has to give way for >>>> the >>>>> practical solution. And this is a core file, so we should thread >>>> lightly. >>>>> >>>>> >>>>> As for an alternative solution, perhaps we can get rid of the MARKERs >>>> in the >>>>> struct and #define them instead. Not as elegant as Gavin's suggested >>>> union >>>>> based solution, but it might bring inspiration... >>>>> >>>>> struct rte_mbuf { >>>>> ... >>>>> } __rte_aligned(sizeof(rte_iova_t)); >>>>> >>>>> uint16_t data_off; >>>>> ... >>>>> } >>>>> >>>>> #define rte_mbuf_rearm_data(m) ((uint64_t *)m->data_off) >>>> >>>> This does not work out, it generates new errors: >>>> /root/dpdk/build/include/rte_mbuf_core.h:485:33: error: dereferencing >>>> type-punned pointer will break strict-aliasing rules [-Werror=3Dstrict= - >>>> aliasing] >>>> 485 | #define rte_mbuf_rearm_data(m) ((uint64_t *)&m->data_off) >>>> >>> >>> OK. Then Bruce's suggestion probably won't work either. >>> >>> I found this article about strict aliasing: >> https://gist.github.com/shafik/848ae25ee209f698763cffee272a58f8 >>> >>> The article basically says that the union based method (i.e. your origi= nal >> suggestion) is valid C (but not C++) and is the common solution. >>> >>> Alternatives have now been discussed and tested, so we should all suppo= rt >> your original suggestion, which seems to be the only correct and viable = solution. >>> >>> Please go ahead with that, and then someone should update the SFC PMD >> accordingly. >>> >>> Furthermore, I think that Stephen's suggestion about getting rid of the >> markers all together is good thinking, but it would require updating a l= ot of >> PMDs accordingly. So please also consider removing other markers that ca= n be >> removed without affecting a whole bunch of other files. >>> >> >> Does it still give errors if we don't have the cast in the macro? >=20 > Yes, it gives errors elsewhere that have the cast.=20 >=20 Hi Gavin, I lost track if v2 is still a candidate for merge. fwiw, it compiles without giving the zero-length-bounds warning on my system. Kevin.