On Mon, May 9, 2022 at 4:31 PM David Marchand <david.march...@redhat.com> wrote:
> On Mon, May 9, 2022 at 2:24 PM Stanisław Kardach <k...@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 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 gets > 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 include > 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 > >
