netdev - Re: [RFC] HW TX Rate Limiting Driver API

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: <1380ba9e71d500628994b0a1a7cbb108b4bf9492.camel@redhat.com>
Date: Tue, 23 Apr 2024 19:25:37 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: Simon Horman <horms@...nel.org>, netdev@...r.kernel.org, Jiri Pirko
 <jiri@...nulli.us>, Madhu Chittim <madhu.chittim@...el.com>, Sridhar
 Samudrala <sridhar.samudrala@...el.com>
Subject: Re: [RFC] HW TX Rate Limiting Driver API

On Mon, 2024-04-22 at 11:06 -0700, Jakub Kicinski wrote:
> On Fri, 19 Apr 2024 13:53:53 +0200 Paolo Abeni wrote:
> > > They don't have to be nodes. They can appear as parent or child of 
> > > a real node, but they don't themselves carry any configuration.
> > > 
> > > IOW you can represent them as a special encoding of the ID field,
> > > rather than a real node.  
> > 
> > I'm sorry for the latency, I got distracted elsewhere. 
> > 
> > It's not clear the benefit of introducing this 'attach points' concept.
> > 
> > With the current proposal, configuring a queue shaper would be:
> > 
> > info.bw_min = ...
> > dev->shaper_ops->set(dev, SHAPER_LOOKUP_BY_QUEUE, queue_id, &info, &ack);
> > 
> > and restoring the default could be either:
> > 
> > info.bw_min = 0;
> > dev->shaper_ops->set(dev, SHAPER_LOOKUP_BY_QUEUE, queue_id, &info, &ack);
> 
> And presumably also bw_max = 0 also means "delete" or will it be bw_max
> = ~0 ?

I would say just bw_min = 0, all others fields are ignored in such
case. But not very relevant since...
> 
> > or:
> > 
> > dev->shaper_ops->delete(dev, SHAPER_LOOKUP_BY_QUEUE, queue_id, &info, &ack);
> 
> Which confusingly will not actually delete the node, subsequent get()
> will still return it.
> 
> > With the 'attach points' I guess it will be something alike the
> > following (am not defining a different node type here just to keep the
> > example short):
> > 
> > # configure a queue shaper
> > struct shaper_info attach_info;
> > dev->shaper_ops->get(dev, SHAPER_LOOKUP_BY_QUEUE, queue_id, &attach_info, &ack);
> > info.parent_id = attach_info.id;
> > info.bw_min = ...
> > new_node_id = dev->shaper_ops->add(dev, &info, &ack);
> > 
> > # restore defaults:
> > dev->shaper_ops->delete(dev, SHAPER_LOOKUP_BY_TREE_ID, new_node_id, &info, &ack);
> > 
> > likely some additional operation would be needed to traverse/fetch
> > directly the actual shaper present at the attach points???
> 
> Whether type + ID (here SHAPER_LOOKUP_BY_QUEUE, queue_id) identifies
> the node sufficiently to avoid the get is orthogonal. Your ->set
> example assumes you don't have to do a get first to find exact
> (synthetic) node ID. The same can be true for an ->add, if you prefer.

... my understanding is that you have strong preference over the
'attach points' variant.

I think in the end is mostly a matter of clearly define
expectation/behavior and initial status. 

Assuming the initial tree is empty, and the kernel tracks all changes
vs the default (empty tree) configuration, I guess it's probably better
change slightly the ->add():

int (*add)(struct net_device *dev, 
	   enum shaper_lookup_mode lookup_mode, // for the parent
	   u32 parent_id			// relative to lookup_mode
	   const struct shaper_info *shaper,    // 'id' and 'parent_id' fields
						// are unused
	   struct netlink_ext_ack *extack);

so we can easily add/configure a queue shapers with a single op:

	struct shaper_info attach_info;
	info.bw_min = ...
	dev->shaper_ops->add(dev, SHAPER_LOOKUP_BY_QUEUE, queue_id, &info, &ack);

And possibly even the ->move() op:

	int (*move)(struct net_device *dev,
		    u32 id, // shapers to be moved, id is SHAPER_LOOKUP_BY_TREE_ID
		    enum shaper_lookup_mode lookup_mode, // for the new parent
		    u32 new_parent_id, 			 // relative to 'lookup_mode'
		    struct netlink_ext_ack *extack);

Since it does not make sense to move around the attach point, the
kernel knows the id of the created/modified nodes, and it should be
useful be able to move the shaper directly under an arbitrary attach
point.

Finally, what about renaming the whole SHAPER_LOOKUP_* values to
SHAPER_LOOKUP_TX_* so we can later easily extends this to RX? 

> I do find it odd that we have objects in multiple places of 
> the hierarchy when there is no configuration intended. Especially
> that
> the HW may actually not support such configuration (say there is
> always
> a DRR before the egress, now we insert a shaping stage there).

Regardless of the model we chose (with 'attach points' or without), if
the H/W does not support a given configuration, the relative op must
fail. No shapers will be created matching an unsupported configuration.

Thanks,

Paolo