Re: [RFC PATCH net-next 3/6 v2] net/sched: cls_flower: Add ematch support

[Paul Blakey <paulb@mellanox.com>]

On 29/01/2019 20:08, Marcelo Leitner wrote:
> On Tue, Jan 29, 2019 at 10:02:03AM +0200, Paul Blakey wrote:
>> TODO: handle EEXist.
>>
>> Signed-off-by: Paul Blakey <paulb@mellanox.com>
>> ---
>>  include/uapi/linux/pkt_cls.h |  2 ++
>>  net/sched/cls_flower.c       | 22 ++++++++++++++++++----
>>  2 files changed, 20 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
>> index 121f1ef..d848d6d 100644
>> --- a/include/uapi/linux/pkt_cls.h
>> +++ b/include/uapi/linux/pkt_cls.h
>> @@ -506,6 +506,8 @@ enum {
>>  	TCA_FLOWER_KEY_CT_LABELS,
>>  	TCA_FLOWER_KEY_CT_LABELS_MASK,
>>  
>> +	TCA_FLOWER_EMATCHES,
>> +
>>  	__TCA_FLOWER_MAX,
>>  };
>>  
>> diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
>> index bf74a31..f11fda0 100644
>> --- a/net/sched/cls_flower.c
>> +++ b/net/sched/cls_flower.c
>> @@ -104,6 +104,7 @@ struct cls_fl_filter {
>>  	struct rhash_head ht_node;
>>  	struct fl_flow_key mkey;
>>  	struct tcf_exts exts;
>> +	struct tcf_ematch_tree ematches;
>>  	struct tcf_result res;
>>  	struct fl_flow_key key;
>>  	struct list_head list;
>> @@ -332,10 +333,14 @@ static int fl_classify(struct sk_buff *skb, const struct tcf_proto *tp,
>>  		fl_set_masked_key(&skb_mkey, &skb_key, mask);
>>  
>>  		f = fl_lookup(mask, &skb_mkey, &skb_key);
>> -		if (f && !tc_skip_sw(f->flags)) {
>> -			*res = f->res;
>> -			return tcf_exts_exec(skb, &f->exts, res);
>> -		}
>> +		if (!f || tc_skip_sw(f->flags))
>> +			continue;
>> +
>> +		if (!tcf_em_tree_match(skb, &f->ematches, NULL))
>> +			continue;
> 
> Considering just the recirc_id (and not the other fields supported by
> ematch), have you considered integrating recirc_id match on flow
> dissector instead?  It would avoid the matching in 2 steps here and
> benefit from the hashing.
> 

yes,
although ematch is no op if not used, I actually have to convert flower
to a rhl hashtable as we can have the flower keys but different ematches
which is a pointer (and why I have the TODO in the commit msg), then all
similar flows , different only by recirc id ematch, could be on the same
list, and this would be slow. I'm not sure how real this example is, but
I agree.

So I'll change it for next patch, unless someone thinks different.

Thanks.

>> +
>> +		*res = f->res;
>> +		return tcf_exts_exec(skb, &f->exts, res);
>>  	}
>>  	return -1;
>>  }
>> @@ -388,6 +393,7 @@ static bool fl_mask_put(struct cls_fl_head *head, struct fl_flow_mask *mask,
>>  static void __fl_destroy_filter(struct cls_fl_filter *f)
>>  {
>>  	tcf_exts_destroy(&f->exts);
>> +	tcf_em_tree_destroy(&f->ematches);
>>  	tcf_exts_put_net(&f->exts);
>>  	kfree(f);
>>  }
>> @@ -523,6 +529,7 @@ static void *fl_get(struct tcf_proto *tp, u32 handle)
>>  static const struct nla_policy fl_policy[TCA_FLOWER_MAX + 1] = {
>>  	[TCA_FLOWER_UNSPEC]		= { .type = NLA_UNSPEC },
>>  	[TCA_FLOWER_CLASSID]		= { .type = NLA_U32 },
>> +	[TCA_FLOWER_EMATCHES]		= { .type = NLA_NESTED },
>>  	[TCA_FLOWER_INDEV]		= { .type = NLA_STRING,
>>  					    .len = IFNAMSIZ },
>>  	[TCA_FLOWER_KEY_ETH_DST]	= { .len = ETH_ALEN },
>> @@ -1348,6 +1355,10 @@ static int fl_set_parms(struct net *net, struct tcf_proto *tp,
>>  	if (err < 0)
>>  		return err;
>>  
>> +	err = tcf_em_tree_validate(tp, tb[TCA_FLOWER_EMATCHES], &f->ematches);
>> +	if (err < 0)
>> +		return err;
>> +
>>  	if (tb[TCA_FLOWER_CLASSID]) {
>>  		f->res.classid = nla_get_u32(tb[TCA_FLOWER_CLASSID]);
>>  		tcf_bind_filter(tp, &f->res, base);
>> @@ -2143,6 +2154,9 @@ static int fl_dump(struct net *net, struct tcf_proto *tp, void *fh,
>>  	    nla_put_u32(skb, TCA_FLOWER_CLASSID, f->res.classid))
>>  		goto nla_put_failure;
>>  
>> +	if (tcf_em_tree_dump(skb, &f->ematches, TCA_FLOWER_EMATCHES) < 0)
>> +		goto nla_put_failure;
>> +
>>  	key = &f->key;
>>  	mask = &f->mask->key;
>>  
>> -- 
>> 1.8.3.1
>>