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

[Jakub Kicinski <kuba@kernel.org>]

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.

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

> 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.
Plus IIRC you already have the EQ configuration via params.