> From: Thomas Monjalon [mailto:tho...@monjalon.net] > Sent: Saturday, 24 February 2024 12.14 > > 24/02/2024 11:42, Morten Brørup: > > From: Tyler Retzlaff [mailto:roret...@linux.microsoft.com] > > > 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 >
Thomas has provided a lot of good feedback for this version of the series, showing that the path is worth exploring further. In other words: I'm likely to be convinced. ;-)
