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. >> 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
