Quoting Alexander Duyck (2021-01-06 20:54:11)
> On Wed, Jan 6, 2021 at 10:04 AM Antoine Tenart <aten...@kernel.org> wrote:
> > +/* 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.
> > 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.
>
> > - 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
