Message-ID: <20230217161920.gs3b2fbc5k73fput@tycho>
Date:   Fri, 17 Feb 2023 17:19:20 +0100
From:   Zahari Doychev <zahari.doychev@...ux.com>
To:     Alexander Lobakin <aleksander.lobakin@...el.com>
Cc:     netdev@...r.kernel.org, jhs@...atatu.com, xiyou.wangcong@...il.com,
        jiri@...nulli.us, davem@...emloft.net, edumazet@...gle.com,
        kuba@...nel.org, pabeni@...hat.com, hmehrtens@...linear.com,
        Zahari Doychev <zdoychev@...linear.com>
Subject: Re: [PATCH net-next 1/2] net: flower: add support for matching cfm
 fields


[...]

> > +/**
> > + * 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.

> > +
> >  	__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.

> > +
> >  #define TCA_FLOWER_MASK_FLAGS_RANGE	(1 << 0) /* Range-based match */
> >  
> >  /* Match-all classifier */
> > diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> > index 25fb0bbc310f..adb23d31f199 100644
> > --- a/net/core/flow_dissector.c
> > +++ b/net/core/flow_dissector.c
> > @@ -547,6 +547,41 @@ __skb_flow_dissect_arp(const struct sk_buff *skb,
> >  	return FLOW_DISSECT_RET_OUT_GOOD;
> >  }
> >  
> > +static enum flow_dissect_ret
> > +__skb_flow_dissect_cfm(const struct sk_buff *skb,
> > +		       struct flow_dissector *flow_dissector,
> > +		       void *target_container, const void *data,
> > +		       int nhoff, int hlen)
> > +{
> > +	struct flow_dissector_key_cfm *key_cfm;
> > +	struct cfm_common_hdr {
> 
> I don't see this type used anywhere else in the code, so you can leave
> it anonymous.

noted.

> 
> > +		__u8 mdlevel_version;
> > +		__u8 opcode;
> > +		__u8 flags;
> > +		__u8 tlv_offset;
> 
> This is a purely-kernel-side structure, so use simply `u8` here for each
> of them.

I will fix it.

> 
> > +	} *hdr, _hdr;
> > +#define CFM_MD_LEVEL_SHIFT	5
> > +#define CFM_MD_VERSION_MASK	0x1f
> > +
> > +	if (!dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_CFM))
> > +		return FLOW_DISSECT_RET_OUT_GOOD;
> > +
> > +	hdr = __skb_header_pointer(skb, nhoff, sizeof(_hdr), data,
> > +				   hlen, &_hdr);
> > +	if (!hdr)
> > +		return FLOW_DISSECT_RET_OUT_BAD;
> > +
> > +	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.

> > +	key_cfm->opcode = hdr->opcode;
> > +
> > +	return  FLOW_DISSECT_RET_OUT_GOOD;
> > +}
> > +
> >  static enum flow_dissect_ret
> >  __skb_flow_dissect_gre(const struct sk_buff *skb,
> >  		       struct flow_dissector_key_control *key_control,
> > @@ -1390,6 +1425,12 @@ bool __skb_flow_dissect(const struct net *net,
> >  		break;
> >  	}
> >  
> > +	case htons(ETH_P_CFM): {
> > +		fdret = __skb_flow_dissect_cfm(skb, flow_dissector,
> > +					       target_container, data,
> > +					       nhoff, hlen);
> > +		break;
> > +	}
> >  	default:
> >  		fdret = FLOW_DISSECT_RET_OUT_BAD;
> >  		break;
> > diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
> > index 885c95191ccf..91f2268e1577 100644
> > --- a/net/sched/cls_flower.c
> > +++ b/net/sched/cls_flower.c
> > @@ -71,6 +71,7 @@ struct fl_flow_key {
> >  	struct flow_dissector_key_num_of_vlans num_of_vlans;
> >  	struct flow_dissector_key_pppoe pppoe;
> >  	struct flow_dissector_key_l2tpv3 l2tpv3;
> > +	struct flow_dissector_key_cfm cfm;
> >  } __aligned(BITS_PER_LONG / 8); /* Ensure that we can do comparisons as longs. */
> >  
> >  struct fl_flow_mask_range {
> > @@ -711,7 +712,7 @@ static const struct nla_policy fl_policy[TCA_FLOWER_MAX + 1] = {
> >  	[TCA_FLOWER_KEY_PPPOE_SID]	= { .type = NLA_U16 },
> >  	[TCA_FLOWER_KEY_PPP_PROTO]	= { .type = NLA_U16 },
> >  	[TCA_FLOWER_KEY_L2TPV3_SID]	= { .type = NLA_U32 },
> > -
> > +	[TCA_FLOWER_KEY_CFM]		= { .type = NLA_NESTED },
> >  };
> >  
> >  static const struct nla_policy
> > @@ -760,6 +761,12 @@ mpls_stack_entry_policy[TCA_FLOWER_KEY_MPLS_OPT_LSE_MAX + 1] = {
> >  	[TCA_FLOWER_KEY_MPLS_OPT_LSE_LABEL]    = { .type = NLA_U32 },
> >  };
> >  
> > +static const struct nla_policy
> > +cfm_opt_policy[TCA_FLOWER_KEY_CFM_OPT_MAX + 1] = {
> 
> Why not just use %__TCA_FLOWER_KEY_CFM_OPT_MAX here which is the same? I
> know it's not how it's been done historically, but anyway.

I was looking again at the other definitions here.

> 
> > +	[TCA_FLOWER_KEY_CFM_MD_LEVEL]		= { .type = NLA_U8 },
> > +	[TCA_FLOWER_KEY_CFM_OPCODE]		= { .type = NLA_U8 },
> > +};
> > +
> >  static void fl_set_key_val(struct nlattr **tb,
> >  			   void *val, int val_type,
> >  			   void *mask, int mask_type, int len)
> > @@ -1644,6 +1651,67 @@ static bool is_vlan_key(struct nlattr *tb, __be16 *ethertype,
> >  	return false;
> >  }
> >  
> > +#define CFM_MD_LEVEL_MASK 0x7
> 
> Can all those definitions be located in one place in some header file
> instead of being scattered across several C files? You'll need them one
> day and forget where you place them, some other developers won't know
> they are somewhere in C files and decide they're not defined in the kernel.
> 

I can try to move them to a header file. I see some attributes have
their own header with defines only. 

> > +static int fl_set_key_cfm_md_level(struct nlattr **tb,
> > +				   struct fl_flow_key *key,
> > +				   struct fl_flow_key *mask,
> > +				   struct netlink_ext_ack *extack)
> > +{
> > +	u8 level;
> > +
> > +	if (!tb[TCA_FLOWER_KEY_CFM_MD_LEVEL])
> > +		return 0;
> > +
> > +	level = nla_get_u8(tb[TCA_FLOWER_KEY_CFM_MD_LEVEL]);
> > +	if (level & ~CFM_MD_LEVEL_MASK) {
> > +		NL_SET_ERR_MSG_ATTR(extack,
> > +				    tb[TCA_FLOWER_KEY_CFM_MD_LEVEL],
> > +				    "cfm md level must be 0-7");
> > +		return -EINVAL;
> > +	}
> > +	key->cfm.mdl = level;
> > +	mask->cfm.mdl = CFM_MD_LEVEL_MASK;
> > +
> > +	return 0;
> > +}
> > +
> > +static void fl_set_key_cfm_opcode(struct nlattr **tb,
> > +				  struct fl_flow_key *key,
> > +				  struct fl_flow_key *mask,
> > +				  struct netlink_ext_ack *extack)
> > +{
> > +	if (!tb[TCA_FLOWER_KEY_CFM_OPCODE])
> > +		return;
> > +
> > +	fl_set_key_val(tb, &key->cfm.opcode,
> > +		       TCA_FLOWER_KEY_CFM_OPCODE,
> > +		       &mask->cfm.opcode,
> > +		       TCA_FLOWER_UNSPEC,
> > +		       sizeof(key->cfm.opcode));
> 
> I think at least some of these fit into the previous line, there's no
> need to break lines just to break them or have one argument per line.
> 

I will improve this.

[...]

> +	if (mask->cfm.mdl &&
> > +	    nla_put_u8(skb,
> > +		       TCA_FLOWER_KEY_CFM_MD_LEVEL,
> > +		       key->cfm.mdl)) {
> 
> Also weird linewrapping.
> 
> > +		err = -EMSGSIZE;
> > +		goto err_cfm_opts;
> > +	}
> > +
> > +	if (mask->cfm.opcode &&
> > +	    nla_put_u8(skb,
> > +		       TCA_FLOWER_KEY_CFM_OPCODE,
> > +		       key->cfm.opcode)) {
> 
> (same)
> 

I will fix both lines as well.

Many thanks
Zahari

> > +		err = -EMSGSIZE;
> > +		goto err_cfm_opts;
> > +	}
> > +
> > +	nla_nest_end(skb, opts);
> > +
> > +	return 0;
> > +
> > +err_cfm_opts:
> > +	nla_nest_cancel(skb, opts);
> > +	return err;
> > +}
> > +
> >  static int fl_dump_key_options(struct sk_buff *skb, int enc_opt_type,
> >  			       struct flow_dissector_key_enc_opts *enc_opts)
> >  {
> > @@ -3266,6 +3379,9 @@ static int fl_dump_key(struct sk_buff *skb, struct net *net,
> >  			     sizeof(key->hash.hash)))
> >  		goto nla_put_failure;
> >  
> > +	if (fl_dump_key_cfm(skb, key, mask))
> > +		goto nla_put_failure;
> > +
> >  	return 0;
> >  
> >  nla_put_failure:
> 
> Thanks,
> Olek

Powered by blists - more mailing lists