Re: [PATCH net-next V2 3/6] net/mlx5: hw counters: Replace IDR+lists with xarray

[Simon Horman <horms@kernel.org>]

On Fri, Oct 04, 2024 at 09:32:11AM +0000, Cosmin Ratiu wrote:
> On Fri, 2024-10-04 at 09:58 +0100, Simon Horman wrote:
> > On Tue, Oct 01, 2024 at 01:37:06PM +0300, Tariq Toukan wrote:
> > > From: Cosmin Ratiu <cratiu@nvidia.com>
> > 
> > ...
> > 
> > > +/* Synchronization notes
> > > + *
> > > + * Access to counter array:
> > > + * - create - mlx5_fc_create() (user context)
> > > + *   - inserts the counter into the xarray.
> > > + *
> > > + * - destroy - mlx5_fc_destroy() (user context)
> > > + *   - erases the counter from the xarray and releases it.
> > > + *
> > > + * - query mlx5_fc_query(), mlx5_fc_query_cached{,_raw}() (user context)
> > > + *   - user should not access a counter after destroy.
> > > + *
> > > + * - bulk query (single thread workqueue context)
> > > + *   - create: query relies on 'lastuse' to avoid updating counters added
> > > + *             around the same time as the current bulk cmd.
> > > + *   - destroy: destroyed counters will not be accessed, even if they are
> > > + *              destroyed during a bulk query command.
> > > + */
> > > +static void mlx5_fc_stats_query_all_counters(struct mlx5_core_dev *dev)
> > >  {
> > >  	struct mlx5_fc_stats *fc_stats = dev->priv.fc_stats;
> > > -	bool query_more_counters = (first->id <= last_id);
> > > -	int cur_bulk_len = fc_stats->bulk_query_len;
> > > +	u32 bulk_len = fc_stats->bulk_query_len;
> > > +	XA_STATE(xas, &fc_stats->counters, 0);
> > >  	u32 *data = fc_stats->bulk_query_out;
> > > -	struct mlx5_fc *counter = first;
> > > +	struct mlx5_fc *counter;
> > > +	u32 last_bulk_id = 0;
> > > +	u64 bulk_query_time;
> > >  	u32 bulk_base_id;
> > > -	int bulk_len;
> > >  	int err;
> > >  
> > > -	while (query_more_counters) {
> > > -		/* first id must be aligned to 4 when using bulk query */
> > > -		bulk_base_id = counter->id & ~0x3;
> > > -
> > > -		/* number of counters to query inc. the last counter */
> > > -		bulk_len = min_t(int, cur_bulk_len,
> > > -				 ALIGN(last_id - bulk_base_id + 1, 4));
> > > -
> > > -		err = mlx5_cmd_fc_bulk_query(dev, bulk_base_id, bulk_len,
> > > -					     data);
> > > -		if (err) {
> > > -			mlx5_core_err(dev, "Error doing bulk query: %d\n", err);
> > > -			return;
> > > -		}
> > > -		query_more_counters = false;
> > > -
> > > -		list_for_each_entry_from(counter, &fc_stats->counters, list) {
> > > -			int counter_index = counter->id - bulk_base_id;
> > > -			struct mlx5_fc_cache *cache = &counter->cache;
> > > -
> > > -			if (counter->id >= bulk_base_id + bulk_len) {
> > > -				query_more_counters = true;
> > > -				break;
> > > +	xas_lock(&xas);
> > > +	xas_for_each(&xas, counter, U32_MAX) {
> > > +		if (xas_retry(&xas, counter))
> > > +			continue;
> > > +		if (unlikely(counter->id >= last_bulk_id)) {
> > > +			/* Start new bulk query. */
> > > +			/* First id must be aligned to 4 when using bulk query. */
> > > +			bulk_base_id = counter->id & ~0x3;
> > > +			last_bulk_id = bulk_base_id + bulk_len;
> > > +			/* The lock is released while querying the hw and reacquired after. */
> > > +			xas_unlock(&xas);
> > > +			/* The same id needs to be processed again in the next loop iteration. */
> > > +			xas_reset(&xas);
> > > +			bulk_query_time = jiffies;
> > > +			err = mlx5_cmd_fc_bulk_query(dev, bulk_base_id, bulk_len, data);
> > > +			if (err) {
> > > +				mlx5_core_err(dev, "Error doing bulk query: %d\n", err);
> > > +				return;
> > >  			}
> > > -
> > > -			update_counter_cache(counter_index, data, cache);
> > > +			xas_lock(&xas);
> > > +			continue;
> > >  		}
> > > +		/* Do not update counters added after bulk query was started. */
> > 
> > Hi Cosmin and Tariq,
> > 
> > I'm sorry if it is obvious, but I'm wondering if you could explain further
> > the relationship between the if block above, where bulk_query_time (and
> > bulk_base_id) is initialised and if block below, which is conditional on
> > bulk_query_time.
> > 
> > > +		if (time_after64(bulk_query_time, counter->cache.lastuse))
> > > +			update_counter_cache(counter->id - bulk_base_id, data,
> > > +					     &counter->cache);
> > >  	}
> > > +	xas_unlock(&xas);
> > >  }
> > 
> > ...
> 
> Hi Simon. Of course.
> 
> The first if (with 'unlikely') is the one that starts a bulk query.
> The second if is the one that updates a counter's cached value with the
> output from the bulk query. Bulks are usually ~32K counters, if I
> remember correctly. In any case, a large number.
> 
> The first if sets up the bulk query params and executes it without the
> lock held. During that time, counters could be added/removed. We don't
> want to update counter values for counters added between when the bulk
> query was executed and when the lock was reacquired. bulk_query_time
> with jiffy granularity is used for that purpose. When a counter is
> added, its 'cache.lastuse' is initialized to jiffies. Only counters
> with ids between [bulk_base_id, last_bulk_id) added strictly before the
> jiffy when bulk_query_time was set will be updated because the hw might
> not have set newer counter values in the bulk result and values might
> be garbage.
> 
> I also have this blurb in the commit description, but it is probably
> lost in the wall of text:
> "
> Counters could be added/deleted while the HW is queried. This is safe,
> as the HW API simply returns unknown values for counters not in HW, but
> those values won't be accessed. Only counters present in xarray before
> bulk query will actually read queried cache values.
> "

Thanks, I did see that, but for some reason I didn't relate
it to the question I asked.

> 
> There's also a comment bit in the "Synchronization notes" section:
>  * - bulk query (single thread workqueue context)
>  *   - create: query relies on 'lastuse' to avoid updating counters
> added
>  *             around the same time as the current bulk cmd.

But this one I had missed.

> 
> Hope this clears things out, let us know if you'd like something
> improved.

Yes, thank you. It is clear now :)