<div dir="auto"><div><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Sep 21, 2023, 15:18 Tummala, Sivaprasad <<a href="mailto:Sivaprasad.Tummala@amd.com">Sivaprasad.Tummala@amd.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">[AMD Official Use Only - General]<br>
<br>
> -----Original Message-----<br>
> From: David Marchand <<a href="mailto:david.marchand@redhat.com" target="_blank" rel="noreferrer">david.marchand@redhat.com</a>><br>
> Sent: Wednesday, September 20, 2023 1:05 PM<br>
> To: Stanisław Kardach <<a href="mailto:kda@semihalf.com" target="_blank" rel="noreferrer">kda@semihalf.com</a>>; Tummala, Sivaprasad<br>
> <<a href="mailto:Sivaprasad.Tummala@amd.com" target="_blank" rel="noreferrer">Sivaprasad.Tummala@amd.com</a>><br>
> Cc: Ruifeng Wang <<a href="mailto:ruifeng.wang@arm.com" target="_blank" rel="noreferrer">ruifeng.wang@arm.com</a>>; Min Zhou <<a href="mailto:zhoumin@loongson.cn" target="_blank" rel="noreferrer">zhoumin@loongson.cn</a>>;<br>
> David Christensen <<a href="mailto:drc@linux.vnet.ibm.com" target="_blank" rel="noreferrer">drc@linux.vnet.ibm.com</a>>; Bruce Richardson<br>
> <<a href="mailto:bruce.richardson@intel.com" target="_blank" rel="noreferrer">bruce.richardson@intel.com</a>>; Konstantin Ananyev<br>
> <<a href="mailto:konstantin.v.ananyev@yandex.ru" target="_blank" rel="noreferrer">konstantin.v.ananyev@yandex.ru</a>>; dev <<a href="mailto:dev@dpdk.org" target="_blank" rel="noreferrer">dev@dpdk.org</a>>; Yigit, Ferruh<br>
> <<a href="mailto:Ferruh.Yigit@amd.com" target="_blank" rel="noreferrer">Ferruh.Yigit@amd.com</a>>; Thomas Monjalon <<a href="mailto:thomas@monjalon.net" target="_blank" rel="noreferrer">thomas@monjalon.net</a>><br>
> Subject: Re: [PATCH v2 2/2] eal: remove NUMFLAGS enumeration<br>
><br>
> Caution: This message originated from an External Source. Use proper caution<br>
> when opening attachments, clicking links, or responding.<br>
><br>
><br>
> On Wed, Sep 20, 2023 at 8:01 AM Stanisław Kardach <<a href="mailto:kda@semihalf.com" target="_blank" rel="noreferrer">kda@semihalf.com</a>> wrote:<br>
> ><br>
> > On Tue, Sep 19, 2023 at 4:47 PM David Marchand<br>
> <<a href="mailto:david.marchand@redhat.com" target="_blank" rel="noreferrer">david.marchand@redhat.com</a>> wrote:<br>
> > <snip><br>
> > > > Also I see you're still removing the RTE_CPUFLAG_NUMFLAGS (what I call a<br>
> last element canary). Why? If you're concerned with ABI, then we're talking about<br>
> an application linking dynamically with DPDK or talking via some RPC channel with<br>
> another DPDK application. So clashing with this definition does not come into<br>
> question. One should rather use rte_cpu_get_flag_enabled().<br>
> > > > Also if you want to introduce new features, one would add them yo the<br>
> rte_cpuflags headers, unless you'd like to not add those and keep an<br>
> undocumented list "above" the last defined element.<br>
> > > > Could you explain a bit more Your use-case?<br>
> > ><br>
> > > Hey Stanislaw,<br>
> > ><br>
> > > Talking generically, one problem with such pattern (having a LAST,<br>
> > > or MAX enum) is when an array sized with such a symbol is exposed.<br>
> > > As I mentionned in the past, this can have unwanted effects:<br>
> > > <a href="https://patchwork.dpdk.org/project/dpdk/patch/20230919140430.3251493" rel="noreferrer noreferrer" target="_blank">https://patchwork.dpdk.org/project/dpdk/patch/20230919140430.3251493</a><br>
> > > -<a href="http://1-david.marchand@redhat.com/" rel="noreferrer noreferrer" target="_blank">1-david.marchand@redhat.com/</a><br>
><br>
> Argh... who broke copy/paste in my browser ?!<br>
> Wrt to MAX and arrays, I wanted to point at:<br>
> <a href="http://inbox.dpdk.org/dev/CAJFAV8xs5CVdE2xwRtaxk5vE_PiQMV5LY5tKStk3R1gOuR" rel="noreferrer noreferrer" target="_blank">http://inbox.dpdk.org/dev/CAJFAV8xs5CVdE2xwRtaxk5vE_PiQMV5LY5tKStk3R1gOuR</a><br>
> <a href="http://TsUw@mail.gmail.com/" rel="noreferrer noreferrer" target="_blank">TsUw@mail.gmail.com/</a><br>
><br>
> > I agree, though I'd argue "LAST" and "MAX" semantics are a bit different. "LAST"<br>
> delimits the known enumeration territory while "MAX" is more of a `constepxr`<br>
> value type.<br>
> > ><br>
> > > Another issue is when an existing enum meaning changes: from the<br>
> > > application pov, the (old) MAX value is incorrect, but for the<br>
> > > library pov, a new meaning has been associated.<br>
> > > This may trigger bugs in the application when calling a function<br>
> > > that returns such an enum which never return this MAX value in the past.<br>
> > ><br>
> > > For at least those two reasons, removing those canary elements is<br>
> > > being done in DPDK.<br>
> > ><br>
> > > This specific removal has been announced:<br>
> > > <a href="https://patchwork.dpdk.org/project/dpdk/patch/20230919140430.3251493" rel="noreferrer noreferrer" target="_blank">https://patchwork.dpdk.org/project/dpdk/patch/20230919140430.3251493</a><br>
> > > -<a href="http://1-david.marchand@redhat.com/" rel="noreferrer noreferrer" target="_blank">1-david.marchand@redhat.com/</a><br>
> > Thanks for pointing this out but did you mean to link to the patch again here?<br>
><br>
> Sorry, same here, bad copy/paste :-(.<br>
><br>
> The intended link is: <a href="https://git.dpdk.org/dpdk/commit/?id=5da7c13521" rel="noreferrer noreferrer" target="_blank">https://git.dpdk.org/dpdk/commit/?id=5da7c13521</a><br>
> The deprecation notice was badly formulated and this patch here is consistent with<br>
> it.<br>
><br>
><br>
> > ><br>
> > > 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<br>
> > > is unneeded.<br>
> > > But on the other hand, is it really an issue for an application to<br>
> > > lose this (internal) information?<br>
> > I doubt it, maybe it could be used as a sanity check for choosing proper functors<br>
> in the application. Though the initial description of the reason behind this patch was<br>
> to not break the ABI and I don't think it does that. What it does is enforces users to<br>
> use explicit cpu flag values which is a good thing. Though if so, then it should be<br>
> stated in the commit description.<br>
><br>
> I agree.<br>
> Siva, can you work on a new revision?<br>
><br>
David, Stanislaw,<br>
<br>
The original motivation of this patch was to avoid ABI breakage with the introduction of new CPU flag<br>
"RTE_CPUFLAG_MONITORX" (<a href="http://mails.dpdk.org/archives/test-report/2023-April/382489.html" rel="noreferrer noreferrer" target="_blank">http://mails.dpdk.org/archives/test-report/2023-April/382489.html</a>).<br>
<br>
Because of ABI breakage, the feature was postponed to this release.<br>
<a href="https://patchwork.dpdk.org/project/dpdk/patch/20230413115334.43172-3-sivaprasad.tummala@amd.com/" rel="noreferrer noreferrer" target="_blank">https://patchwork.dpdk.org/project/dpdk/patch/20230413115334.43172-3-sivaprasad.tummala@amd.com/</a></blockquote></div></div><div dir="auto">This test is flawed, reason being that the NUMFLAGS should not be treated as a flag value and instead as a canary but this test is not taking into account.</div><div dir="auto">Your change did not break the ABI because you have properly added the new flag at the end.</div><div dir="auto">So I would ask to change the commit description to mention that NUMFLAGS is removed to:</div><div dir="auto">1. Prevent users from treating it as a usable value or an array size.</div><div dir="auto">2. Prevent false-positive failures in the ABI test.</div><div dir="auto"><br></div><div dir="auto">Also it would be good to link to the aforementioned ABI test failure to give readers some context when inspecting the git tree.</div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
<br>
Can you please add what exactly needs to be reworked in the new version.<br>
<br>
><br>
> Thanks.<br>
><br>
> --<br>
> David Marchand<br>
<br>
</blockquote></div></div></div>