24/02/2024 11:42, Morten Brørup:
> From: Tyler Retzlaff [mailto:[email protected]]
> > RTE_MARKER typedefs are a GCC extension unsupported by MSVC. This series
> > hides the markers when building with MSVC and updates libraries and
> > drivers to access compatibly typed pointers to the rte_mbuf struct
> > offsets.
> >
> > This series, does the following.
> >
> > Introduces a new macro __rte_marker(type, name) which is used to
> > conditionally expand RTE_MARKER fields empty when building with GCC.
>
> Important typo: GCC -> MSVC.
>
> Correct me if I'm wrong: When building with MSVC, the RTE_MARKER fields don't
> exist at all.
Yes it should trigger a compiler error when using an old marker with MSVC.
So no need of this wrapper __rte_marker().
> > Updates existing inline functions accessing cacheline{0,1} markers in
> > the rte_mbuf struct to stop using the markers and instead uses the mbuf
> > fields directly.
> >
> > Introduces 2 new inline functions to allow drivers to access rearm_data
> > and rx_descriptor_fields1 descriptors without using the RTE_MARKER
> > fields.
> >
> > Updates all drivers to use the new inline rte_mbuf struct accessors for
> > rearm_data and rx_descriptor_fields1.
>
> Accessor functions like these are BS for structures that are part of the
> public API.
What is BS?
> Nobody will use the accessor functions! When developing an application (or
> lib or driver), the developer will look at the structure and access the
> relevant fields directly; why would the developer start looking elsewhere for
> accessor functions?
Yes that's why we need to reference the accessors inside the mbuf structure.
Also we should check new code (with checkpatch) for not using markers anymore.
> Another alternative would be to remove the rearm_data and
> rx_descriptor_fields1 fields from the structure, and in the drivers address
> the first field of the group, and preferably add some static_asserts to check
> the sequence of the fields they cover. I don't like this alternative, but I'm
> putting it out there for discussion/inspiration.
> I prefer the union+struct approach to visibly group the fields together.
Unions make the mbuf struct more complicate just for compatibility.
> Overall, I dislike approach taken in this version of the patch series.
> On the surface, it has minimal changes to the mbuf structure.
> But underneath, some of the fields (the markers) may or may not exist,
> depending on compiler, and this fact is not obvious when looking at the
> structure. I think this will degrade future MSVC compatibility for both
> applications, libraries and PMDs.
With appropriate checks, we won't use markers anymore.
> As Thomas stressed, we should take special care of the mbuf structure!
> It has to be modified for MSVC compatibility, so we have to find the best
> compromise.
> Personally, I prefer the previous approach over this version.
>
> Maybe we need to compromise on API compatibility to make this a beautiful
> modification.
>
> @Thomas, looking at the mbuf and eal patches, what do you think about this
> version of the series?
I prefer this series with following changes:
- no __rte_marker wrapper
- make sure cache line padding is effective without markers
- no direct access of fields for cache line prefetch
- comments in mbuf
- checkpatch
