Re: [PATCH net 3/3] net-sysfs: move the xps cpus/rxqs retrieval in a common function

Quoting Alexander Duyck (2021-01-06 20:54:11)

On Wed, Jan 6, 2021 at 10:04 AM Antoine Tenart [off-list ref] wrote:

quoted

+/* Should be called with the rtnl lock held. */
+static int xps_queue_show(struct net_device *dev, unsigned long **mask,
+                         unsigned int index, bool is_rxqs_map)

Why pass dev and index instead of just the queue which already
contains both?

Right, I can do that.

I think it would make more sense to just stick to passing the queue
through along with a pointer to the xps_dev_maps value that we need to
read.

That would require to hold rcu_read_lock in the caller and I'd like to
keep it in that function.

quoted

        if (dev->num_tc) {
                /* Do not allow XPS on subordinate device directly */
                num_tc = dev->num_tc;
-               if (num_tc < 0) {
-                       ret = -EINVAL;
-                       goto err_rtnl_unlock;
-               }
+               if (num_tc < 0)
+                       return -EINVAL;

                /* If queue belongs to subordinate dev use its map */
                dev = netdev_get_tx_queue(dev, index)->sb_dev ? : dev;

                tc = netdev_txq_to_tc(dev, index);
-               if (tc < 0) {
-                       ret = -EINVAL;
-                       goto err_rtnl_unlock;
-               }
+               if (tc < 0)
+                       return -EINVAL;
        }

So if we store the num_tc and nr_ids in the dev_maps structure then we
could simplify this a bit by pulling the num_tc info out of the
dev_map and only asking the Tx queue for the tc in that case and
validating it against (tc <0 || num_tc <= tc) and returning an error
if either are true.

This would also allow us to address the fact that the rxqs feature
doesn't support the subordinate devices as you could pull out the bit
above related to the sb_dev and instead call that prior to calling
xps_queue_show so that you are operating on the correct device map.

quoted

-       mask = bitmap_zalloc(nr_cpu_ids, GFP_KERNEL);
-       if (!mask) {
-               ret = -ENOMEM;
-               goto err_rtnl_unlock;
+       rcu_read_lock();
+
+       if (is_rxqs_map) {
+               dev_maps = rcu_dereference(dev->xps_rxqs_map);
+               nr_ids = dev->num_rx_queues;
+       } else {
+               dev_maps = rcu_dereference(dev->xps_cpus_map);
+               nr_ids = nr_cpu_ids;
+               if (num_possible_cpus() > 1)
+                       possible_mask = cpumask_bits(cpu_possible_mask);
        }

I think Jakub had mentioned earlier the idea of possibly moving some
fields into the xps_cpus_map and xps_rxqs_map in order to reduce the
complexity of this so that certain values would be protected by the
RCU lock.

This might be a good time to look at encoding things like the number
of IDs and the number of TCs there in order to avoid a bunch of this
duplication. Then you could just pass a pointer to the map you want to
display and the code should be able to just dump the values.:

100% agree to all the above. That would also prevent from making out of
bound accesses when dev->num_tc is increased after dev_maps is
allocated. I do have a series ready to be send storing num_tc into the
maps, and reworking code to use it instead of dev->num_tc. The series
also adds checks to ensure the map is valid when we access it (such as
making sure dev->num_tc == map->num_tc). I however did not move nr_ids
into the map yet, but I'll look into it.

The idea is to send it as a follow up series, as this one is only moving
code around to improve maintenance and readability. Even if all the
patches were in the same series that would be a prerequisite.

Thanks!
Antoine