netdev - Re: [PATCH net-next 3/7] sfc: debugfs for (nic) RX queues

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <252425b8-ba1f-4505-a7b4-9a90ddd5ead2@amd.com>
Date: Fri, 14 Feb 2025 09:39:05 -0800
From: "Nelson, Shannon" <shannon.nelson@....com>
To: Edward Cree <ecree.xilinx@...il.com>, edward.cree@....com,
 linux-net-drivers@....com, davem@...emloft.net, kuba@...nel.org,
 pabeni@...hat.com, edumazet@...gle.com
Cc: netdev@...r.kernel.org, habetsm.xilinx@...il.com,
 Jonathan Cooper <jonathan.s.cooper@....com>
Subject: Re: [PATCH net-next 3/7] sfc: debugfs for (nic) RX queues

On 2/14/2025 7:51 AM, Edward Cree wrote:
> 
> On 15/12/2023—
> wait, has it really been more than a year?  Yikes.

Time flies when we're having all this fun?

> 
> On 15/12/2023 00:05, Nelson, Shannon wrote:
>> On 12/11/2023 9:18 AM, edward.cree@....com wrote:
>>> +       if (snprintf(name, sizeof(name), "rx-%d", efx_rx_queue_index(rx_queue))
>>
>> Adding leading 0's here can be helpful for directory entry sorting
> 
> True, but it's not clear how many to use — the hardware supports over
>   1000 in principle, and in practice it's normal to have one per core
>   (and more than that on TX) which can get over 100 on powerful systems.
> Yet on something like an 8-core box having queues 000 to 007 just looks
>   silly imho.  I don't plan to change this line in v2.

I suppose you could do something tricky with %0*d and log10(num_queues), 
but that seems to be a bit over the top.  No change, no problem.


> 
>>> +           >= sizeof(name))
>>> +               return -ENAMETOOLONG;
>>> +       rx_queue->debug_dir = debugfs_create_dir(name,
>>> +                                                rx_queue->efx->debug_queues_dir);
>>> +       if (!rx_queue->debug_dir)
>>> +               return -ENOMEM;
>>> +
>>> +       /* Create files */
>>> +       efx_init_debugfs_rx_queue_files(rx_queue);
>>> +
>>> +       /* Create symlink to channel */
>>> +       if (snprintf(target, sizeof(target), "../../channels/%d",
>>> +                    channel->channel) >= sizeof(target))
>>> +               return -ENAMETOOLONG;
>>> +       if (!debugfs_create_symlink("channel", rx_queue->debug_dir, target))
>>> +               return -ENOMEM;
>>
>> If these fail, should you clean up the earlier create_dir()?
> 
> No; these errors mean "we didn't do everything we wanted to", not
>   "it's all broken", and the files/dir previously created are still
>   useful.  The caller treats errors as non-fatal.
> See also the kdoc comment on efx_init_debugfs_rx_queue:
>>> + * The directory must be cleaned up using efx_fini_debugfs_rx_queue(),
>>> + * even if this function returns an error.
> 
> I can't think of a suitable comment on these return statements to
>   clarify this, but suggestions are welcome.

Maybe something simple like
  "We don't clean up the files on errors here as they are still useful"

Cheers,
sln