Re: [PATCH net-next 1/2] net: flower: add support for matching cfm fields

[Alexander Lobakin <aleksander.lobakin@intel.com>]

From: Zahari Doychev <zahari.doychev@linux.com>
Date: Fri, 17 Feb 2023 17:19:20 +0100

> 
> [...]
> 
>>> +/**
>>> + * struct flow_dissector_key_cfm
>>> + *
>>> + */
>>
>> ???
>>
>> Without a proper kernel-doc, this makes no sense. So either remove this
>> comment or make a kernel-doc from it, describing the structure and each
>> its member (I'd go for kernel-doc :P).
>>
> 
> I will fix this.
> 
>>> +struct flow_dissector_key_cfm {
>>> +	u8	mdl:3,
>>> +		ver:5;
>>> +	u8	opcode;
>>> +};
>>> +
>>>  enum flow_dissector_key_id {
>>>  	FLOW_DISSECTOR_KEY_CONTROL, /* struct flow_dissector_key_control */
>>>  	FLOW_DISSECTOR_KEY_BASIC, /* struct flow_dissector_key_basic */
>>> @@ -329,6 +339,7 @@ enum flow_dissector_key_id {
>>>  	FLOW_DISSECTOR_KEY_NUM_OF_VLANS, /* struct flow_dissector_key_num_of_vlans */
>>>  	FLOW_DISSECTOR_KEY_PPPOE, /* struct flow_dissector_key_pppoe */
>>>  	FLOW_DISSECTOR_KEY_L2TPV3, /* struct flow_dissector_key_l2tpv3 */
>>> +	FLOW_DISSECTOR_KEY_CFM, /* struct flow_dissector_key_cfm */
>>>  
>>>  	FLOW_DISSECTOR_KEY_MAX,
>>>  };
>>> diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
>>> index 648a82f32666..d55f70ccfe3c 100644
>>> --- a/include/uapi/linux/pkt_cls.h
>>> +++ b/include/uapi/linux/pkt_cls.h
>>> @@ -594,6 +594,8 @@ enum {
>>>  
>>>  	TCA_FLOWER_KEY_L2TPV3_SID,	/* be32 */
>>>  
>>> +	TCA_FLOWER_KEY_CFM,
>>
>> Each existing definitions within this enum have a comment mentioning the
>> corresponding type (__be32, __u8 and so on), why doesn't this one?
>>
> 
> I was following the other nest option attributes which don't have
> a comment but sure I can add one or probably change the name to
> include the opts prefix.

Ah it's nested. You can put a comment with the single word "nested" there.

> 
>>> +
>>>  	__TCA_FLOWER_MAX,
>>>  };
>>>  
>>> @@ -702,6 +704,16 @@ enum {
>>>  	TCA_FLOWER_KEY_FLAGS_FRAG_IS_FIRST = (1 << 1),
>>>  };
>>>  
>>> +enum {
>>> +	TCA_FLOWER_KEY_CFM_OPT_UNSPEC,
>>> +	TCA_FLOWER_KEY_CFM_MD_LEVEL,
>>> +	TCA_FLOWER_KEY_CFM_OPCODE,
>>> +	__TCA_FLOWER_KEY_CFM_OPT_MAX,
>>> +};
>>> +
>>> +#define TCA_FLOWER_KEY_CFM_OPT_MAX \
>>> +		(__TCA_FLOWER_KEY_CFM_OPT_MAX - 1)
>>
>> This fits into one line...
>> Can't we put it into the enum itself?
>>
> 
> I can fix this but putting it into the enum makes it different from the 
> other defintions. So I am not quire sure on that.

Nothing present in the kernel automatically means it's good or approved
or you should go only that route. Write the code the way you feel it
looks the best and will see what others think.

> 
>>> +
>>>  #define TCA_FLOWER_MASK_FLAGS_RANGE	(1 << 0) /* Range-based match */

[...]

>>> +	key_cfm = skb_flow_dissector_target(flow_dissector,
>>> +					    FLOW_DISSECTOR_KEY_CFM,
>>> +					    target_container);
>>> +
>>> +	key_cfm->mdl = hdr->mdlevel_version >> CFM_MD_LEVEL_SHIFT;
>>> +	key_cfm->ver = hdr->mdlevel_version & CFM_MD_VERSION_MASK;
>>
>> I'd highly recommend using FIELD_GET() here.
>>
>> Or wait, why can't you just use one structure for both FD and the actual
>> header? You only need two fields going next to each other, so you could
>> save some cycles by just directly assigning them (I mean, just define
>> the fields you need, not the whole header since you use only first two
>> fields).
>>
> 
> I am not sure if get this completely. I understand we can reduce the
> struct size be removing the not needed fields but we still need to
> use the FIELD_GET here. Please correct me if my understanding is wrong.

If both packet header and kernel-side container will have the same
layout, you could assign the fields directly.

	key_cfm->mdlevel_version = hdr->mdlevel_version;
	key_cfm->opcode = hdr->opcode;

> 
>>> +	key_cfm->opcode = hdr->opcode;
>>> +
>>> +	return  FLOW_DISSECT_RET_OUT_GOOD;
>>> +}
[...]

Thanks,
Olek