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 3DF1AA0093; Tue, 10 May 2022 13:22:30 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 2967F406B4; Tue, 10 May 2022 13:22:30 +0200 (CEST) Received: from mail-lj1-f176.google.com (mail-lj1-f176.google.com [209.85.208.176]) by mails.dpdk.org (Postfix) with ESMTP id 05D694069D for ; Tue, 10 May 2022 13:22:28 +0200 (CEST) Received: by mail-lj1-f176.google.com with SMTP id 4so20423867ljw.11 for ; Tue, 10 May 2022 04:22:28 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=semihalf-com.20210112.gappssmtp.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=Weu2VoqqLet450Q/Xn9kaEEoQX0LPHUkUrJY02ClDls=; b=p1PChF0nnmC7fig1WPosGLs/SlxGH4RYTF4gkl8X6NHUhkh2/0o7sbapf+mUt1NDft VCl6AG/1Yye2sDg565nSgfo3b0Ip+0AL7trunuVnWHtPnqpYuA1CAo/eAPzEBw1duABn isjdenXxLMHRh8EREnDWcfPKDNhZfjMGYFq7tbqa9kyjHpyUNt+HsBBnjJbWNgBzWcnP T9zNDakne/RJycGUot5SUvr+ijociQk4E5NaQ3rLGCaS0GZyWHHyfwsEZcLDvEXEio5B +de2jPjSYaqWLVj6r9HQGS5YK3uTgNpMwpxxOCFP6Ebf205Kt0Bc4dMbMuT9kNs4NGb8 8hXg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=Weu2VoqqLet450Q/Xn9kaEEoQX0LPHUkUrJY02ClDls=; b=MyNQktViL52O7qSMaPeCCseFSH7adbrwm+bRih6LovNUy8BtjtPbY5LAQPIKCcraks rquLZD8YPYy74WduGKBuHYQHcZEn+65Mwr82S1o5uDLJsNLNktZVXu6OGLrP8lIzvG9k 5vP5m3wSmOYWITaufOd7LELz95KPLojbiYURhihjy2yJHLNLfBA/p1YyiDEzZV2bt7A/ ANqjokIDi+1tHEN2jtUnpxlRSLryh9woEfpi13CueKHB2sBEgYEJGar9QQAVLeNOxt3v /bDwnN5jawUnqehsmGYHoAt51By7jkObS0q5Yqa4mEdNVg6bpYqq45ufwLQUtHH+nr1N u/FA== X-Gm-Message-State: AOAM533bae1pk4RlUFtUW8/ToinccHRhzwP0wFYtyIZpGqdrOGJX4reT tzdyotmlbx5WkRPFMoKQuwrRJAkMQX7u60+oH6pf/w== X-Google-Smtp-Source: ABdhPJw04U8g1FyJlqiuXGvx8VTyaQFaLDCXcAd0Z7xRXAmv6uMb8fU5gBBRUJy/XB8/QMZgCTgTmj7dHUz2EdyZiyU= X-Received: by 2002:a05:651c:1506:b0:250:6459:d6d4 with SMTP id e6-20020a05651c150600b002506459d6d4mr14052073ljf.271.1652181748445; Tue, 10 May 2022 04:22:28 -0700 (PDT) MIME-Version: 1.0 References: <20220505173003.3242618-1-kda@semihalf.com> In-Reply-To: From: =?UTF-8?Q?Stanis=C5=82aw_Kardach?= Date: Tue, 10 May 2022 13:21:52 +0200 Message-ID: Subject: Re: [PATCH 00/11] Introduce support for RISC-V architecture To: David Marchand Cc: dev , Frank Zhao , Sam Grove , Marcin Wojtas , upstream@semihalf.com, Thomas Monjalon , Stephen Hemminger Content-Type: multipart/alternative; boundary="0000000000003884a605dea68af8" 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 --0000000000003884a605dea68af8 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Mon, May 9, 2022 at 4:31 PM David Marchand wrote: > On Mon, May 9, 2022 at 2:24 PM Stanis=C5=82aw Kardach = wrote: > >> Testing all riscv configs in test-meson-buils.sh seems too much to me. > >> Is there a real value to test both current targets? > > > > It's for sanity and compilation coverage testing. I.e. SiFive variant > has a specific build config which does not require extra barriers when > reading time and cycle registers for rte_rdtsc_precise(). I want to make > sure that if anyone changes some code based on configuration flags, it ge= ts > at least compile-checked. > > I believe similar thing is done for Aarch64 builds. > > As far as I experienced, building all those aarch64 combinations never > revealed any specific platform compilation issue. > It only consumes cpu, disk and our (maintainers) time. > I proposed to Thomas to shrink aarch64 builds list not so long ago :-). > > The best would be for SiFive to provide a system for the CI to do > those checks on their variant. > > > >> About the new "Sponsored-by" tag, it should not raise warnings in the > >> CI if we agree on its addition. > > > > I'll modify it in V2 to be in form of: > > Sponsored by: StarFive Technology > > ... > > Signed-off-by: ... > > This was suggested by Stephen Hemminger as having a precedent in Linux > kernel. Interestingly enough first use of this tag in kernel source was > this year in January. > > I don't have an opinion on the spelling. > > At the moment, the checks raise a warning: > http://mails.dpdk.org/archives/test-report/2022-May/278580.html > > My point is that for this new tag, either checkpatch.pl in kernel > handles it (which I don't think it is the case) or we need to disable > the signature check in checkpatch.pl and something is added in dpdk > checkpatches.sh to accept all known tags. > BAD_SIGN_OFF handles more than just tag names (in total there's 10 cases checked). I'm not sure replicating this to checkpatches.sh is worth the maintenance. Alternatively I could ignore BAD_SIGN_OFF on initial checkpatch.pl run and then run it again with just the BAD_SIGN_OFF type and filter out the result= . In that case, what would be the acceptable content of Sponsored-by tag? For line: Sponsored-by: StarFive Technology Current checkpatch.pl generates (used --terse for brevity): 0001-eal-add-initial-support-for-RISC-V-architecture.patch:55: WARNING:BAD_SIGN_OFF: Non-standard signature: Sponsored-by: 0001-eal-add-initial-support-for-RISC-V-architecture.patch:55: ERROR:BAD_SIGN_OFF: Unrecognized email address: 'StarFive Technology' Using "Sponsored by:" does not trigger checks above (still feels like a hack). > > >> In general, please avoid letting arch specific headers leak > >> internal/non rte_ prefixed helpers out of them. > >> For example, I noticed a RV64_CSRR macro that can be undefined after > usage. > > > > Thanks for noticing. I'l fix this one in V2. > > There are 2 other symbols that leak but on purpose (out of a better > idea): vect_load_128() and vect_and(). Both are used in l3fwd_em to > simulate vector operations. Other platforms reference their intrinsics > straight in the l3fwd_em.c. As I don't have support for vector ops and I > wanted to indicate that xmm_t should be an isolated API, I've put both in > rte_vect.h. That said I'm not happy with this solution and am open to > suggestions on how to solve it neatly. > > I'll try to have a look in the next revision. > > > >> > >> > >> Patch 3 is huge, not sure it is easy to split, did you consider doing > so? > > > > It seems to me the nature of a new EAL implementation, I have to includ= e > all symbols, otherwise DPDK won't compile. > > Alternatively I could have a huge initial patch with empty stubs that > would be filled in later commits. Downside of this approach is that it's > hard to verify each commit separately as tests will fail until all > implementation is there, so the division is only visual. > > If you are sure there is nothing that can be separated, let's keep it > whole. > > > > -- > David Marchand > > --0000000000003884a605dea68af8 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
On Mon, May 9, 2022 at 4:31 PM David Marc= hand <david.marchand@redhat= .com> wrote:
On Mon, May 9, 2022 at 2:24 PM Stanis=C5= =82aw Kardach <kda= @semihalf.com> wrote:
>> Testing all riscv configs in test-meson-buils.sh seems too much to= me.
>> Is there a real value to test both current targets?
>
> It's for sanity and compilation coverage testing. I.e. SiFive vari= ant has a specific build config which does not require extra barriers when = reading time and cycle registers for rte_rdtsc_precise(). I want to make su= re that if anyone changes some code based on configuration flags, it gets a= t least compile-checked.
> I believe similar thing is done for Aarch64 builds.

As far as I experienced, building all those aarch64 combinations never
revealed any specific platform compilation issue.
It only consumes cpu, disk and our (maintainers) time.
I proposed to Thomas to shrink aarch64 builds list not so long ago :-).

The best would be for SiFive to provide a system for the CI to do
those checks on their variant.


>> About the new "Sponsored-by" tag, it should not raise wa= rnings in the
>> CI if we agree on its addition.
>
> I'll modify it in V2 to be in form of:
>=C2=A0 =C2=A0Sponsored by: StarFive Technology
>=C2=A0 =C2=A0...
>=C2=A0 =C2=A0Signed-off-by: ...
> This was suggested by Stephen Hemminger as having a precedent in Linux= kernel. Interestingly enough first use of this tag in kernel source was th= is year in January.

I don't have an opinion on the spelling.

At the moment, the checks raise a warning:
http://mails.dpdk.org/archives/test-r= eport/2022-May/278580.html

My point is that for this new tag, either checkpatch.pl in kernel
handles it (which I don't think it is the case) or we need to disable the signature check in checkpatch.pl and something is added in dpdk
checkpatches.sh to accept all known tags.
BAD_SIGN_OFF= handles more than just tag names (in total there's 10 cases checked). = I'm not sure replicating this to checkpatches.sh is worth the maintenan= ce.
Alternatively I could ignore BAD_SIGN_OFF on initial checkpatch.pl run and then run it again with = just the BAD_SIGN_OFF type and filter out the result.
In that cas= e, what would be the acceptable content of Sponsored-by tag? For line:
=C2=A0=C2=A0Sponsored-by: StarFive Technology
Current checkpatch.pl generates (used --terse for b= revity):
=C2=A0 0001-eal-add-initial-support-for-RISC-V-architect= ure.patch:55: WARNING:BAD_SIGN_OFF: Non-standard signature: Sponsored-by:=C2=A0 0001-eal-add-initial-support-for-RISC-V-architecture.patch:55: ERR= OR:BAD_SIGN_OFF: Unrecognized email address: 'StarFive Technology'<= br>

Using "Sponsored by:" does not trigg= er checks above (still feels like a hack).

>> In general, please avoid letting arch specific headers leak
>> internal/non rte_ prefixed helpers out of them.
>> For example, I noticed a RV64_CSRR macro that can be undefined aft= er usage.
>
> Thanks for noticing. I'l fix this one in V2.
> There are 2 other symbols that leak but on purpose (out of a better id= ea): vect_load_128() and vect_and(). Both are used in l3fwd_em to simulate = vector operations. Other platforms reference their intrinsics straight in t= he l3fwd_em.c. As I don't have support for vector ops and I wanted to i= ndicate that xmm_t should be an isolated API, I've put both in rte_vect= .h. That said I'm not happy with this solution and am open to suggestio= ns on how to solve it neatly.

I'll try to have a look in the next revision.


>>
>>
>> Patch 3 is huge, not sure it is easy to split, did you consider do= ing so?
>
> It seems to me the nature of a new EAL implementation, I have to inclu= de all symbols, otherwise DPDK won't compile.
> Alternatively I could have a huge initial patch with empty stubs that = would be filled in later commits. Downside of this approach is that it'= s hard to verify each commit separately as tests will fail until all implem= entation is there, so the division is only visual.

If you are sure there is nothing that can be separated, let's keep it w= hole.



--
David Marchand

--0000000000003884a605dea68af8--