Re: [PATCH net-next v12 08/12] virtio_net: Use existing classifier if possible

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Nov 24, 2025 at 04:31:54PM -0600, Dan Jurgens wrote:
> On 11/24/25 4:04 PM, Michael S. Tsirkin wrote:
> > On Wed, Nov 19, 2025 at 01:15:19PM -0600, Daniel Jurgens wrote:
> >> Classifiers can be used by more than one rule. If there is an existing
> >> classifier, use it instead of creating a new one. If duplicate
> >> classifiers are created it would artifically limit the number of rules
> >> to the classifier limit, which is likely less than the rules limit.
> >>
> >> Signed-off-by: Daniel Jurgens <danielj@nvidia.com>
> >> Reviewed-by: Parav Pandit <parav@nvidia.com>
> >> Reviewed-by: Shahar Shitrit <shshitrit@nvidia.com>
> >> Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> >> ---
> >> v4:
> >>     - Fixed typo in commit message
> >>     - for (int -> for (
> >>
> >> v8:
> >>     - Removed unused num_classifiers. Jason Wang
> >>
> >> v12:
> >>     - Clarified comment about destroy_classifier freeing. MST
> >>     - Renamed the classifier field of virtnet_classifier to obj. MST
> >>     - Explained why in commit message. MST
> >> ---
> >> ---
> >>  drivers/net/virtio_net.c | 51 ++++++++++++++++++++++++++--------------
> >>  1 file changed, 34 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> >> index 7600e2383a72..5e49cd78904f 100644
> >> --- a/drivers/net/virtio_net.c
> >> +++ b/drivers/net/virtio_net.c
> >> @@ -32,6 +32,7 @@
> >>  #include <uapi/linux/virtio_pci.h>
> >>  #include <uapi/linux/virtio_net_ff.h>
> >>  #include <linux/xarray.h>
> >> +#include <linux/refcount.h>
> >>  
> >>  static int napi_weight = NAPI_POLL_WEIGHT;
> >>  module_param(napi_weight, int, 0444);
> >> @@ -302,7 +303,6 @@ struct virtnet_ff {
> >>  	struct virtio_net_ff_cap_mask_data *ff_mask;
> >>  	struct virtio_net_ff_actions *ff_actions;
> >>  	struct xarray classifiers;
> >> -	int num_classifiers;
> >>  	struct virtnet_ethtool_ff ethtool;
> >>  };
> >>  
> >> @@ -5816,12 +5816,13 @@ struct virtnet_ethtool_rule {
> >>  /* The classifier struct must be the last field in this struct */
> >>  struct virtnet_classifier {
> >>  	size_t size;
> >> +	refcount_t refcount;
> >>  	u32 id;
> >> -	struct virtio_net_resource_obj_ff_classifier classifier;
> >> +	struct virtio_net_resource_obj_ff_classifier obj;
> >>  };
> >>  
> >>  static_assert(sizeof(struct virtnet_classifier) ==
> >> -	      ALIGN(offsetofend(struct virtnet_classifier, classifier),
> >> +	      ALIGN(offsetofend(struct virtnet_classifier, obj),
> >>  		    __alignof__(struct virtnet_classifier)),
> >>  	      "virtnet_classifier: classifier must be the last member");
> >>  
> >> @@ -5909,11 +5910,24 @@ static bool validate_mask(const struct virtnet_ff *ff,
> >>  	return false;
> >>  }
> >>  
> >> -static int setup_classifier(struct virtnet_ff *ff, struct virtnet_classifier *c)
> >> +static int setup_classifier(struct virtnet_ff *ff,
> >> +			    struct virtnet_classifier **c)
> >>  {
> >> +	struct virtnet_classifier *tmp;
> >> +	unsigned long i;
> >>  	int err;
> >>  
> >> -	err = xa_alloc(&ff->classifiers, &c->id, c,
> >> +	xa_for_each(&ff->classifiers, i, tmp) {
> >> +		if ((*c)->size == tmp->size &&
> >> +		    !memcmp(&tmp->obj, &(*c)->obj, tmp->size)) {
> >> +			refcount_inc(&tmp->refcount);
> >> +			kfree(*c);
> >> +			*c = tmp;
> >> +			goto out;
> >> +		}
> >> +	}
> >> +
> >> +	err = xa_alloc(&ff->classifiers, &(*c)->id, *c,
> >>  		       XA_LIMIT(0, le32_to_cpu(ff->ff_caps->classifiers_limit) - 1),
> >>  		       GFP_KERNEL);
> >>  	if (err)
> >> @@ -5921,29 +5935,30 @@ static int setup_classifier(struct virtnet_ff *ff, struct virtnet_classifier *c)
> >>  
> >>  	err = virtio_admin_obj_create(ff->vdev,
> >>  				      VIRTIO_NET_RESOURCE_OBJ_FF_CLASSIFIER,
> >> -				      c->id,
> >> +				      (*c)->id,
> >>  				      VIRTIO_ADMIN_GROUP_TYPE_SELF,
> >>  				      0,
> >> -				      &c->classifier,
> >> -				      c->size);
> >> +				      &(*c)->obj,
> >> +				      (*c)->size);
> >>  	if (err)
> >>  		goto err_xarray;
> >>  
> >> +	refcount_set(&(*c)->refcount, 1);
> >> +out:
> >>  	return 0;
> >>  
> >>  err_xarray:
> >> -	xa_erase(&ff->classifiers, c->id);
> >> +	xa_erase(&ff->classifiers, (*c)->id);
> >>  
> >>  	return err;
> >>  }
> >>  
> >> -static void destroy_classifier(struct virtnet_ff *ff,
> >> -			       u32 classifier_id)
> >> +static void try_destroy_classifier(struct virtnet_ff *ff, u32 classifier_id)
> >>  {
> >>  	struct virtnet_classifier *c;
> >>  
> >>  	c = xa_load(&ff->classifiers, classifier_id);
> >> -	if (c) {
> >> +	if (c && refcount_dec_and_test(&c->refcount)) {
> >>  		virtio_admin_obj_destroy(ff->vdev,
> >>  					 VIRTIO_NET_RESOURCE_OBJ_FF_CLASSIFIER,
> >>  					 c->id,
> >> @@ -5967,7 +5982,7 @@ static void destroy_ethtool_rule(struct virtnet_ff *ff,
> >>  				 0);
> >>  
> >>  	xa_erase(&ff->ethtool.rules, eth_rule->flow_spec.location);
> >> -	destroy_classifier(ff, eth_rule->classifier_id);
> >> +	try_destroy_classifier(ff, eth_rule->classifier_id);
> >>  	kfree(eth_rule);
> >>  }
> >>  
> >> @@ -6139,7 +6154,7 @@ static int build_and_insert(struct virtnet_ff *ff,
> >>  	}
> >>  
> >>  	c->size = classifier_size;
> >> -	classifier = &c->classifier;
> >> +	classifier = &c->obj;
> >>  	classifier->count = num_hdrs;
> >>  	selector = (void *)&classifier->selectors[0];
> >>  
> >> @@ -6149,14 +6164,16 @@ static int build_and_insert(struct virtnet_ff *ff,
> >>  	if (err)
> >>  		goto err_key;
> >>  
> >> -	err = setup_classifier(ff, c);
> >> +	err = setup_classifier(ff, &c);
> >>  	if (err)
> >>  		goto err_classifier;
> >>  
> >>  	err = insert_rule(ff, eth_rule, c->id, key, key_size);
> >>  	if (err) {
> >> -		/* destroy_classifier will free the classifier */
> >> -		destroy_classifier(ff, c->id);
> >> +		/* destroy_classifier release the reference on the classifier
> > 
> > 
> > try_destroy_classifier ? and I think you mean *will* release and free.
> > 
> > and what is "the reference"
> 
> I see the comment is munged. But classifiers are reference counted,
> try_destroy_classifier will release the reference. And free if the
> refcount is now 0.
> 
> See setup_classifier above.


ah I got it.
you mean refcount - the reference count - not "the reference".
And in the context of refcount_t there's decrement and
increment, not "release". acquire/release in fact refer to memory ordering.

> > 
> >> +		 * and free it if needed.
> >> +		 */
> >> +		try_destroy_classifier(ff, c->id);
> >>  		goto err_key;
> >>  	}
> >>  
> >> -- 
> >> 2.50.1
> >