netdev - Re: [net-next PATCH v7 01/10] netdev-genl: spec: Extend netdev netlink spec in YAML for queue

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <3edec947-a760-4605-9334-81c40ce62670@intel.com>
Date: Wed, 15 Nov 2023 13:05:24 -0800
From: "Nambiar, Amritha" <amritha.nambiar@...el.com>
To: Jakub Kicinski <kuba@...nel.org>
CC: <netdev@...r.kernel.org>, <pabeni@...hat.com>,
	<sridhar.samudrala@...el.com>
Subject: Re: [net-next PATCH v7 01/10] netdev-genl: spec: Extend netdev
 netlink spec in YAML for queue

On 11/14/2023 8:48 PM, Jakub Kicinski wrote:
> On Mon, 13 Nov 2023 16:29:37 -0800 Amritha Nambiar wrote:
>> Add support in netlink spec(netdev.yaml) for queue information.
>> Add code generated from the spec.
>>
>> Note: The "queue-type" attribute takes values 0 and 1 for rx
>> and tx queue type respectively.
> 
> 
>> index 14511b13f305..e7bf6007d77f 100644
>> --- a/Documentation/netlink/specs/netdev.yaml
>> +++ b/Documentation/netlink/specs/netdev.yaml
>> @@ -55,6 +55,10 @@ definitions:
>>           name: hash
>>           doc:
>>             Device is capable of exposing receive packet hash via bpf_xdp_metadata_rx_hash().
>> +  -
>> +    name: queue-type
>> +    type: enum
>> +    entries: [ rx, tx ]
>>   
>>   attribute-sets:
>>     -
>> @@ -87,6 +91,31 @@ attribute-sets:
>>           type: u64
>>           enum: xdp-rx-metadata
>>   
>> +  -
>> +    name: queue
>> +    attributes:
>> +      -
>> +        name: queue-id
> 
> Hm. I guess it looks okay in the Python / JSON but the C defines
> will say NETDEV_QUEUE_QUEUE_ID or some such. Should we drop the word
> queue from all attrs in the queue set?
> 

We could drop the word "queue" from all attrs of queue-object and also 
the word "napi" from all attrs of napi-object. Only concern is, queue 
object will have NAPI-ID as "napi-id" while the napi object will have 
NAPI-ID as "id". Same values but referred to as "id" and "napi-id" 
depending on the object (although should be fine as the command names 
carry the object names).
Here's an example how this would look:

$ queue-get  --json='{"ifindex": 12, "id": 0, "type": 0}'
{'ifindex': 12, 'napi-id': 593, 'id': 0, 'type': 'rx'}

$ napi-get --json='{"id": 593}'
{'ifindex': 12, 'irq': 291, 'id': 593, 'pid': 3817}

Let me know if this is okay.

> Sorry, not sure how I missed this earlier. Some extra nits below while
> I'm requesting changes...
> 
>> +        doc: Queue index for most queue types are indexed like a C array, with
> 
> s/ for/;/ ?

Okay

> 
>> +             indexes starting at 0 and ending at queue count - 1. Queue indexes
>> +             are scoped to an interface and queue type.
>> +        type: u32
>> +      -
>> +        name: ifindex
>> +        doc: ifindex of the netdevice to which the queue belongs.
>> +        type: u32
>> +        checks:
>> +          min: 1
>> +      -
>> +        name: queue-type
>> +        doc: queue type as rx, tx
> 
> Add: ". Each queue type defines a separate ID space."

Okay.

> 
>> +        type: u32
>> +        enum: queue-type
>> +      -
>> +        name: napi-id
>> +        doc: ID of the NAPI instance which services this queue.
>> +        type: u32
>> +
>>   operations:
>>     list:
>>       -
>> @@ -120,6 +149,29 @@ operations:
>>         doc: Notification about device configuration being changed.
>>         notify: dev-get
>>         mcgrp: mgmt
>> +    -
>> +      name: queue-get
>> +      doc: Get queue information from the kernel.
>> +           Only configured queues will be reported (as opposed to all available
>> +           queues).
> 
> maybe add "hardware", so "all available hardware queues)" ?
> That may help the reader connect the dots

Okay.