From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 5AB274333B; Wed, 15 Nov 2023 23:43:44 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 01EDF402EC; Wed, 15 Nov 2023 23:43:44 +0100 (CET) Received: from mail-qk1-f178.google.com (mail-qk1-f178.google.com [209.85.222.178]) by mails.dpdk.org (Postfix) with ESMTP id 979D8402E6 for ; Wed, 15 Nov 2023 23:43:42 +0100 (CET) Received: by mail-qk1-f178.google.com with SMTP id af79cd13be357-778940531dbso7718785a.0 for ; Wed, 15 Nov 2023 14:43:42 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=semihalf.com; s=google; t=1700088222; x=1700693022; darn=dpdk.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=ywblt8XQV/XUN1CoK4jcbiwl5fvmwq5bxpK2rULBOjg=; b=GgKXqSNjwj0C1Og8lYzRB/TbN5bGHqifeR8L6LT/5UYM/zXifD79BYkQOpQAHDHBn1 IGXTL4znfZlsB3K7RRVth8cKMO41WEuFv5pNnxOj6m5EvxJp9XCE2GEKeq8PIYi7L7A+ TiYeA6tHXABjeu0hyFi1wcNhWlcRf2NweqxW7QWIKXvsuAh8kEWVLSVxcBOa9Jta3ByY OPYNSZoXOZdKk+T/MYMRYH+YHiy5+U9REF5kPpdm9CST+rxF1IG4XaWqO1wycFRzI0VV w1CRS7XHqkUZskCSMlP3Cg3IQs5iuA6vmAW72sCbKaI5zDlvA3mpG9ZrU38VHe4sjce7 QPtA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1700088222; x=1700693022; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=ywblt8XQV/XUN1CoK4jcbiwl5fvmwq5bxpK2rULBOjg=; b=hyrCzbmmVaB3k8wMBZdWCgr+z51p2/tTuJiY1nd+YZZCFdCUgypnQCSuc7aBOgwo4W P/LLmpMOqYakA2fZpGJQH7zpstNA2eUlKdwwOqp5kB5XEGNwy9BzAsVFB67eBK1N1cO2 DXhJA8z72Lb9H5TVv1ogY78vsr1WyP8LpGzBrqJXEnyHCZ/BQlpGSm60j/1XPlonz8py pGogxhmDYxGBO1r8RAHAeb2x82Dd779OnJ/S0DyzZVxSfo9DB8z+dPjiIc6kz4IhXIV4 ENQjTTZ3Y6rSKcV+tUA0khKOsBEG0dhdvBc4K6RO3BgyRTuBYgUnGXCnaf8SDdlcrw51 Piug== X-Gm-Message-State: AOJu0Yz5XcT1oFERkcKDv8vNpVoJnhpWYQB6EQwx93zuVipFifrnyIqa 7GnGBWAs6XGKNj4+48qsatYKtYQk8Y3jKD1LBPteTw== X-Google-Smtp-Source: AGHT+IHovBQSnjvOXSnRWxY4Eq+YxVNOw7XDZ4HD/v9/nv2GSQqgsBp7vLB5MZ4M/IYNm4yzv4xxaNboxnNUzwSgeso= X-Received: by 2002:a0c:f6ca:0:b0:65d:6a5:1a3f with SMTP id d10-20020a0cf6ca000000b0065d06a51a3fmr7777328qvo.43.1700088221845; Wed, 15 Nov 2023 14:43:41 -0800 (PST) MIME-Version: 1.0 References: <1700069997-4399-1-git-send-email-roretzla@linux.microsoft.com> <1700069997-4399-2-git-send-email-roretzla@linux.microsoft.com> <98CBD80474FA8B44BF855DF32C47DC35E9F02F@smartserver.smartshare.dk> <20231115210331.GA524@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> In-Reply-To: <20231115210331.GA524@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> From: =?UTF-8?Q?Stanis=C5=82aw_Kardach?= Date: Wed, 15 Nov 2023 23:43:05 +0100 Message-ID: Subject: Re: [PATCH] eal: use C11 alignas instead of GCC attribute aligned To: Tyler Retzlaff Cc: =?UTF-8?Q?Morten_Br=C3=B8rup?= , dev@dpdk.org, =?UTF-8?Q?Mattias_R=C3=B6nnblom?= , Anatoly Burakov , Bruce Richardson , David Christensen , Harry van Haaren , Konstantin Ananyev , Min Zhou , Ruifeng Wang Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On Wed, Nov 15, 2023 at 10:03=E2=80=AFPM Tyler Retzlaff wrote: > > On Wed, Nov 15, 2023 at 09:08:05PM +0100, Morten Br=C3=B8rup wrote: > > > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com] > > > Sent: Wednesday, 15 November 2023 18.40 > > > > > > Now that we have enabled C11 replace the use of __rte_cache_aligned > > > and __rte_aligned(n) with alignas(RTE_CACHE_LINE_SIZE) and > > > __rte_aligned(n) respectively. > > > > > > > [...] > > > > > typedef union rte_xmm { > > > + alignas(16) > > > xmm_t x; > > > uint8_t u8[XMM_SIZE / sizeof(uint8_t)]; > > > uint16_t u16[XMM_SIZE / sizeof(uint16_t)]; > > > uint32_t u32[XMM_SIZE / sizeof(uint32_t)]; > > > uint64_t u64[XMM_SIZE / sizeof(uint64_t)]; > > > double pd[XMM_SIZE / sizeof(double)]; > > > -} __rte_aligned(16) rte_xmm_t; > > > +} rte_xmm_t; > > > > Your patch message should mention that C11 doesn't allow alignas() bein= g applied to the declarations of struct/union types, so it is applied to th= e first field in the struct/union, which has the same effect. > > no problem, will add a note. > > > > > Someone unfamiliar with alignas() would expect: > > > > -typedef union rte_xmm { > > +typedef alignas(16) union rte_xmm { > > [...] > > -} __rte_aligned(16) rte_xmm_t; > > +} rte_xmm_t; > > > > [...] > > > > > #ifndef RTE_VECT_RISCV_H > > > #define RTE_VECT_RISCV_H > > > > > > +#include > > > #include > > > #include "generic/rte_vect.h" > > > #include "rte_common.h" > > > @@ -23,13 +24,14 @@ > > > #define XMM_MASK (XMM_SIZE - 1) > > > > > > typedef union rte_xmm { > > > + alignas(16) /* !! NOTE !! changed to 16 it looks like this was a > > > bug? */ > > > xmm_t x; > > > uint8_t u8[XMM_SIZE / sizeof(uint8_t)]; > > > uint16_t u16[XMM_SIZE / sizeof(uint16_t)]; > > > uint32_t u32[XMM_SIZE / sizeof(uint32_t)]; > > > uint64_t u64[XMM_SIZE / sizeof(uint64_t)]; > > > double pd[XMM_SIZE / sizeof(double)]; > > > -} __rte_aligned(8) rte_xmm_t; > > > +} rte_xmm_t; > > > > Yes, this looks very much like a bug. > > Even if a RISC-V CPU could handle alignment like that, it might interac= t with other software/hardware expecting type-sized alignment, i.e. 16-byte= alignment, so partially using 8-byte alignment would cause bugs. > > > > It should be a separate patch with a Fixes tag. > > i'll submit a patch/fix for this so it is available and others can > discuss if it should or shouldn't be merged for 23.11. It is definitely a bug. Good catch. Since we did not have vector extensions on our bring-up board, all xmm_t handling was essentially scalar. > > > > > We need to urgently decide if this bug should live on in DPDK 23.11, or= if the fix should be included although we are very late in the release pro= cess. > > > > Stanislaw, what do you think? > > > > Furthermore, I wonder if it can be backported to stable, and to what ex= tent backporting it would break the ABI/API. > > --=20 Best Regards, Stanis=C5=82aw Kardach