netdev - Re: [PATCH iwl-net v2 3/3] ice: fix rebuilding the Tx scheduler tree for large queue counts

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] [day] [month] [year] [list]

Message-ID: <aB4U7TBtr75ouKzi@localhost.localdomain>
Date: Fri, 9 May 2025 16:45:01 +0200
From: Michal Kubiak <michal.kubiak@...el.com>
To: Przemek Kitszel <przemyslaw.kitszel@...el.com>
CC: <maciej.fijalkowski@...el.com>, <aleksander.lobakin@...el.com>,
	<dawid.osuchowski@...ux.intel.com>, <jacob.e.keller@...el.com>,
	<jbrandeburg@...udflare.com>, <netdev@...r.kernel.org>,
	<intel-wired-lan@...ts.osuosl.org>
Subject: Re: [PATCH iwl-net v2 3/3] ice: fix rebuilding the Tx scheduler tree
 for large queue counts

On Fri, May 09, 2025 at 03:07:56PM +0200, Przemek Kitszel wrote:
> 
> > +ice_sched_rm_vsi_subtree(struct ice_port_info *pi,
> > +			 struct ice_sched_node *vsi_node, u8 owner, u8 tc)
> > +{
> > +	u16 vsi_handle = vsi_node->vsi_handle;
> > +	bool all_vsi_nodes_removed = true;
> > +	int j = 0;
> > +
> > +	while (vsi_node) {
> > +		struct ice_sched_node *next_vsi_node;
> > +
> > +		if (ice_sched_is_leaf_node_present(vsi_node)) {
> > +			ice_debug(pi->hw, ICE_DBG_SCHED, "VSI has leaf nodes in TC %d\n", tc);
> > +			return -EBUSY;
> > +		}
> > +		while (j < vsi_node->num_children) {
> > +			if (vsi_node->children[j]->owner == owner) {
> > +				ice_free_sched_node(pi, vsi_node->children[j]);
> > +
> > +				/* reset the counter again since the num
> > +				 * children will be updated after node removal
> > +				 */
> > +				j = 0;
> 
> I know this code is a copy-pasta, but it looks that there is no need to
> reset the counter (whole array isn't reshuffled), just don't increase it
> 
You are right. After analyzing the `ice_free_sched_node()` function, it seems
we should just keep the value of `j`. After removing the child node, other
children nodes will be shifted towards the beginning of the array.

> IOW, would be good to check if this line (and the semi-obvious and
> half-wrong comment) could be removed.

I would prefer to change the code, since I'm touching it.

> 
> you could keep my RB tag, thank you again for the series!
>

Let's wait until early next week to gather more potential comments.
Then I will address it in v3.

This minor fix will not be directly related to the core of the series,
so the v2 can still be used to test the workings of XDP on multi-core machines.

Thanks,
Michal

> > +			} else {
> > +				j++;
> > +			}
> > +		}
> > +
> > +		next_vsi_node = ice_sched_find_next_vsi_node(vsi_node);
> > +
> > +		/* remove the VSI if it has no children */
> > +		if (!vsi_node->num_children)
> > +			ice_free_sched_node(pi, vsi_node);
> > +		else
> > +			all_vsi_nodes_removed = false;
> > +
> > +		vsi_node = next_vsi_node;
> > +	}
> > +
> > +	/* clean up aggregator related VSI info if any */
> > +	if (all_vsi_nodes_removed)
> > +		ice_sched_rm_agg_vsi_info(pi, vsi_handle);
> > +
> > +	return 0;
> > +}