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 E4628425E7; Wed, 20 Sep 2023 08:00:58 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id AA3EC402DD; Wed, 20 Sep 2023 08:00:58 +0200 (CEST) Received: from mail-qv1-f54.google.com (mail-qv1-f54.google.com [209.85.219.54]) by mails.dpdk.org (Postfix) with ESMTP id C3F3040277 for ; Wed, 20 Sep 2023 08:00:56 +0200 (CEST) Received: by mail-qv1-f54.google.com with SMTP id 6a1803df08f44-65643a83758so22953126d6.0 for ; Tue, 19 Sep 2023 23:00:56 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=semihalf.com; s=google; t=1695189656; x=1695794456; darn=dpdk.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=bM+uzpAayQfP8P+Zaa3ecPtFYKqT/zLlue2eU/ys/wM=; b=qDumxztRW1qHl6mLAPtATIJaRnb0OVUfxJ+5DyeCz8bVn6TS2F6olA/zW6DqAcbF6V cY2LvKmx4tlhwP2Hm/+Xq959DD4A4xaGs3IZX1cgOOt1rPo9G7/gwdilVs94hm7Kk1of t1WQNgOtJ0EvW8NBpaW5do+qdh4/s+e+GP35tashLffXTkLAl0E5hR2thnYYWNdGWa44 qXvFecLdaIvvehojuX4vCEjklQd5nO0FuEY60QL2jEeoopODWJmVuPN4UTHoduJ71MnG QbRBB9Bc4u2RtCQof0rY6/4anMcmoUcYF3aJ+wT+UE+8TDRv6oQ6EKwgEzTMLIWybYbm AlZA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1695189656; x=1695794456; h=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=bM+uzpAayQfP8P+Zaa3ecPtFYKqT/zLlue2eU/ys/wM=; b=lM2WmMdJuLs2FpjKgR61aN6iBizk18Flj+EiB1tqbLt1U34ojCQo/F2nRi8LHyOPFs ytKdf+NfBmgZqL3aJC3GDB+E4NGvYam8+NU2in6e7pZtwDMvUwhwkL+6nlyZovNA/5Ol CIS458KidpgH7GqTA/JEuUgeBZI3IaaOar/Qhf3BmD8kiW3GpE+Wp7VdjfGbh3/MeJsx Fg+Pw/mtLG8Ai7uff/CjjFSt2m6F1rB0e1XxCji/27rk4a1HgTn7Fnpi7d+0gcyHc4dR gzgSRDDyTaGLk0Ri+NUck6d9U0669yd2nXRlUK2znbTdn0Q59lscnaalM240FhUcgTNB lvag== X-Gm-Message-State: AOJu0Yxf2vvi85w6CX856QLDvLx5T5YfYbt26bSx1bp3N1msyg1ZMNDm miVvXhOL6F4lDV7FuY31eS7aASezRIOu5ndmCYT4kg== X-Google-Smtp-Source: AGHT+IHz8/f/luOyMihGMj4CB9qbJFQZtifLXZ9NwsWuLzeSCLAaISICXlhd90rr8Vv9oO26VdRf4rYNq+loDgQN7Lg= X-Received: by 2002:a05:6214:459a:b0:656:5441:a13f with SMTP id op26-20020a056214459a00b006565441a13fmr1788457qvb.45.1695189656052; Tue, 19 Sep 2023 23:00:56 -0700 (PDT) MIME-Version: 1.0 References: <20230802211150.939121-1-sivaprasad.tummala@amd.com> <20230811060755.481572-1-sivaprasad.tummala@amd.com> <20230811060755.481572-2-sivaprasad.tummala@amd.com> In-Reply-To: From: =?UTF-8?Q?Stanis=C5=82aw_Kardach?= Date: Wed, 20 Sep 2023 08:00:41 +0200 Message-ID: Subject: Re: [PATCH v2 2/2] eal: remove NUMFLAGS enumeration To: David Marchand Cc: Sivaprasad Tummala , Ruifeng Wang , Min Zhou , David Christensen , Bruce Richardson , Konstantin Ananyev , dev , Ferruh Yigit , Thomas Monjalon Content-Type: multipart/alternative; boundary="00000000000046b8560605c419de" 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 --00000000000046b8560605c419de Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Tue, Sep 19, 2023 at 4:47=E2=80=AFPM David Marchand wrote: > > Also I see you're still removing the RTE_CPUFLAG_NUMFLAGS (what I call a last element canary). Why? If you're concerned with ABI, then we're talking about an application linking dynamically with DPDK or talking via some RPC channel with another DPDK application. So clashing with this definition does not come into question. One should rather use rte_cpu_get_flag_enabled(). > > Also if you want to introduce new features, one would add them yo the rte_cpuflags headers, unless you'd like to not add those and keep an undocumented list "above" the last defined element. > > Could you explain a bit more Your use-case? > > Hey Stanislaw, > > Talking generically, one problem with such pattern (having a LAST, or > MAX enum) is when an array sized with such a symbol is exposed. > As I mentionned in the past, this can have unwanted effects: > https://patchwork.dpdk.org/project/dpdk/patch/20230919140430.3251493-1-davi= d.marchand@redhat.com/ I agree, though I'd argue "LAST" and "MAX" semantics are a bit different. "LAST" delimits the known enumeration territory while "MAX" is more of a `constepxr` value type. > > Another issue is when an existing enum meaning changes: from the > application pov, the (old) MAX value is incorrect, but for the library > pov, a new meaning has been associated. > This may trigger bugs in the application when calling a function that > returns such an enum which never return this MAX value in the past. > > For at least those two reasons, removing those canary elements is > being done in DPDK. > > This specific removal has been announced: > https://patchwork.dpdk.org/project/dpdk/patch/20230919140430.3251493-1-davi= d.marchand@redhat.com/ Thanks for pointing this out but did you mean to link to the patch again here? > > Now, practically, when I look at the cpuflags API, I don't see us > exposed to those two issues wrt rte_cpu_flag_t, so maybe this change > is unneeded. > But on the other hand, is it really an issue for an application to > lose this (internal) information? I doubt it, maybe it could be used as a sanity check for choosing proper functors in the application. Though the initial description of the reason behind this patch was to not break the ABI and I don't think it does that. What it does is enforces users to use explicit cpu flag values which is a good thing. Though if so, then it should be stated in the commit description. > --00000000000046b8560605c419de Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
On Tue, Sep 19, 2023 at 4:47=E2=80=AFPM David Marchand &l= t;david.marchand@redhat.com> wrote:
<snip>
> > Also I see you're still removing the RTE_CPUFLAG_NUMFLAGS (wh= at I call a last element canary). Why? If you're concerned with ABI, th= en we're talking about an application linking dynamically with DPDK or = talking via some RPC channel with another DPDK application. So clashing wit= h this definition does not come into question. One should rather use rte_cp= u_get_flag_enabled().
> > Also if you want to introduce new features, one would add them yo= the rte_cpuflags headers, unless you'd like to not add those and keep = an undocumented list "above" the last defined element.
> > Could you explain a bit more Your use-case?
>
> Hey Stanislaw,
>
> Talking generically, one problem with such pattern (having a LAST, or<= br> > MAX enum) is when an array sized with such a symbol is exposed.
> As I mentionned in the past, this can have unwanted effects:
> https://patchwork.dpdk.org/project/dpdk/= patch/20230919140430.3251493-1-david.marchand@redhat.com/
I agree, though I'd argue "LAST" and "MAX" semantic= s are a bit different. "LAST" delimits the known enumeration terr= itory while "MAX" is more of a `constepxr` value type.
>
> Another issue is when an existing enum meaning changes: from the
> application pov, the (old) MAX value is incorrect, but for the library=
> pov, a new meaning has been associated.
> This may trigger bugs in the application when calling a function that<= br> > returns such an enum which never return this MAX value in the past. >
> For at least those two reasons, removing those canary elements is
> being done in DPDK.
>
> This specific removal has been announced:
> https://patchwork.dpdk.org/project/dpdk/= patch/20230919140430.3251493-1-david.marchand@redhat.com/
Thanks for pointing this out but did you mean to link to the patch again he= re?
>
> Now, practically, when I look at the cpuflags API, I don't see us<= br> > exposed to those two issues wrt rte_cpu_flag_t, so maybe this change > is unneeded.
> But on the other hand, is it really an issue for an application to
> lose this (internal) information?
I doubt it, maybe it could be use= d as a sanity check for choosing proper functors in the application. Though= the initial description of the reason behind this patch was to not break t= he ABI and I don't think it does that. What it does is enforces users t= o use explicit cpu flag values which is a good thing. Though if so, then it= should be stated in the commit description.
>
--00000000000046b8560605c419de--