On 31/10/2023 17:14, Ahmed Zaki wrote: > > > On 2023-10-31 08:45, Gal Pressman wrote: >> On 31/10/2023 16:40, Ahmed Zaki wrote: >>> >>> >>> On 2023-10-31 06:00, Gal Pressman wrote: >>>> On 29/10/2023 18:59, Ahmed Zaki wrote: >>>>> >>>>> >>>>> On 2023-10-29 06:48, Gal Pressman wrote: >>>>>> On 29/10/2023 14:42, Ahmed Zaki wrote: >>>>>>> >>>>>>> >>>>>>> On 2023-10-29 06:25, Gal Pressman wrote: >>>>>>>> On 21/10/2023 3:00, Ahmed Zaki wrote: >>>>>>>>> >>>>>>>>> >>>>>>>>> On 2023-10-20 17:49, Jakub Kicinski wrote: >>>>>>>>>> On Fri, 20 Oct 2023 17:14:11 -0600 Ahmed Zaki wrote: >>>>>>>>>>> I replied to that here: >>>>>>>>>>> >>>>>>>>>>> https://lore.kernel.org/all/afb4a06f-cfba-47ba-adb3-09bea7cb5f00@xxxxxxxxx/ >>>>>>>>>>> >>>>>>>>>>> I am kind of confused now so please bear with me. ethtool either >>>>>>>>>>> sends >>>>>>>>>>> "ethtool_rxfh" or "ethtool_rxnfc". AFAIK "ethtool_rxfh" is the >>>>>>>>>>> interface >>>>>>>>>>> for "ethtool -X" which is used to set the RSS algorithm. But we >>>>>>>>>>> kind of >>>>>>>>>>> agreed to go with "ethtool -U|-N" for symmetric-xor, and that >>>>>>>>>>> uses >>>>>>>>>>> "ethtool_rxnfc" (as implemented in this series). >>>>>>>>>> >>>>>>>>>> I have no strong preference. Sounds like Alex prefers to keep it >>>>>>>>>> closer >>>>>>>>>> to algo, which is "ethtool_rxfh". >>>>>>>>>> >>>>>>>>>>> Do you mean use "ethtool_rxfh" instead of "ethtool_rxnfc"? how >>>>>>>>>>> would >>>>>>>>>>> that work on the ethtool user interface? >>>>>>>>>> >>>>>>>>>> I don't know what you're asking of us. If you find the code to >>>>>>>>>> confusing >>>>>>>>>> maybe someone at Intel can help you :| >>>>>>>>> >>>>>>>>> The code is straightforward. I am confused by the requirements: >>>>>>>>> don't >>>>>>>>> add a new algorithm but use "ethtool_rxfh". >>>>>>>>> >>>>>>>>> I'll see if I can get more help, may be I am missing something. >>>>>>>>> >>>>>>>> >>>>>>>> What was the decision here? >>>>>>>> Is this going to be exposed through ethtool -N or -X? >>>>>>> >>>>>>> I am working on a new version that uses "ethtool_rxfh" to set the >>>>>>> symmetric-xor. The user will set per-device via: >>>>>>> >>>>>>> ethtool -X eth0 hfunc toeplitz symmetric-xor >>>>>>> >>>>>>> then specify the per-flow type RSS fields as usual: >>>>>>> >>>>>>> ethtool -N|-U eth0 rx-flow-hash <flow_type> s|d|f|n >>>>>>> >>>>>>> The downside is that all flow-types will have to be either >>>>>>> symmetric or >>>>>>> asymmetric. >>>>>> >>>>>> Why are we making the interface less flexible than it can be with -N? >>>>> >>>>> Alexander Duyck prefers to implement the "symmetric-xor" interface >>>>> as an >>>>> algorithm or extension (please refer to previous messages), but >>>>> ethtool >>>>> does not provide flowtype/RSS fields setting via "-X". The above >>>>> was the >>>>> best solution that we (at Intel) could think of. >>>> >>>> OK, it's a weird we're deliberately limiting our interface, given >>>> there's already hardware that supports controlling symmetric hashing >>>> per >>>> flow type. >>>> >>>> I saw you mentioned the way ice hardware implements symmetric-xor >>>> somewhere, it definitely needs to be added somewhere in our >>>> documentation to prevent confusion. >>>> mlx5 hardware also does symmetric hashing with xor, but not exactly as >>>> you described, we need the algorithm to be clear. >>> >>> Sure. I will add more ice-specific doc in: >>> Documentation/networking/device_drivers/ethernet/intel/ice.rst >> >> I was thinking of somewhere more generic, where ethtool users (not >> necessarily ice users) can refer to. >> >> Perhaps Documentation/networking/ethtool-netlink.rst? Or ethtool man >> page? > > Do you mean add vendor-specific implementation details to common docs? > Not sure if I have seen this before. Any examples? > > Or, we can add a note in ethtool doc that each vendor's implementation > is different and "Refer to your vendor's specifications for more info". It's a generic ethtool flag, its documentation shouldn't be vendor specific. The documentation should reflect the exact details about the algorithm, then other vendors can either use it, or add a new symmetric flag and document it separately.
