Re: [net-next 08/15] net/mlx5e: Add hairpin debugfs files

[Saeed Mahameed <saeed@kernel.org>]

On 11 Jan 13:03, Jakub Kicinski wrote:
>On Wed, 11 Jan 2023 12:46:08 -0800 Saeed Mahameed wrote:
>> On 11 Jan 10:34, Jakub Kicinski wrote:
>> >On Tue, 10 Jan 2023 21:30:38 -0800 Saeed Mahameed wrote:
>> >> +	debugfs_create_file("hairpin_num_queues", 0644, tc->dfs_root,
>> >> +			    &tc->hairpin_params, &fops_hairpin_queues);
>> >> +	debugfs_create_file("hairpin_queue_size", 0644, tc->dfs_root,
>> >> +			    &tc->hairpin_params, &fops_hairpin_queue_size);
>> >
>> >debugfs should be read-only, please LMK if I'm missing something,
>> >otherwise this series is getting reverted
>>
>> I remember asking you about this and you said it's ok to use write for
>> debug features, this is needed for debugging performance bottlenecks.
>
>FWIW I don't think this fits into the debug exemption. What I meant by
>debug was stuff like write to configure what traces or debug features
>of the chip are enabled. This falls into configuration, even if it's
>not expected to be tweaked by users.
>

I see.

>> hairpin + steering performance behaves differently between different
>> hardware versions and under different NIC/E-Switch configs, so it's really
>> important to have some control on some of these attributes when debugging.
>
>Can you expand on the use of this params when debugging? AFAICT these
>configure the RQ/SQ pairs (count and size) so really the only
>"debugging" you can do here is change the config and see if it fixes
>performance...

it's more of understanding the performance effects and characteristics when
combined with other steering configs depending on the HW and current
topology, i don't have exact examples, but usually the debug ends up with
optimizing other places (steering, Firmware, application at the
other end, etc .. )

Sorry i don't have much details here, Maybe Gal can chime in.. 
but what i am sure of is changing the hairpin RQ/SQ configs comes
with a risk.

>
>> Our dilemma was either to use devlink vendor params or a debug interface,
>> since we are pretty sure that our NIC hairpin implementation
>> is unique as it uses software constructs (RQs/SQs) managed internally
>> by Firmware for abstraction of a TC redirect action, thus the only place
>> for this is either devlink vendor params or debugfs, we chose debugfs since
>> we want to keep this for debug purposes on production systems.
>>
>> we also considered extending TC but again since this is unique to CX
>> architecture of the current chips, we didn't want to pollute TC.
>>
>> Also devlink resource wasn't a good match since these resources don't
>> exist until a TC redirect action is offloaded.
>>
>> Please let me know what you think and whether this is acceptable by you.
>
>I don't know of any other devices which need the hairpin setup
>so I won't push for a common API. But we *do* need to list these
>tunables somewhere because my ability to grep them out of mlx5 when
>another vendor comes with the same problem will be very limited.
>Which is one of the reasons why devlink params have to be documented.

Then let's create https://docs.kernel.org/networking/vendor_specific.rst
and record all vendor specific dump in there, including devlink and
ethtool private flags.
once we find a common behavior, it means this should move to be standard? 

>Plus IIRC you already have the EQ configuration via params.

EQ is considered standard parameter in devlink.

We currently have 2 vendor specific params and they are related to
steering pipeline/engines only.
hairpin buffer/queue sizes is only a CX limitation, and implementation
detail.

you can clearly see a pattern here, usually the steering pipeline
requires vendor specific knobs :/ ..

Will you be ok if we moved hairpin config to devlink driver specific param
? given that we will create the vendor_specific.rst for easy tracking and
grepping.

Thanks,
Saeed.