Mon, Mar 30, 2015 at 02:39:31AM CEST, jhs@mojatatu.com wrote:

Jiri,

Comments below

On 03/26/15 08:53, Jiri Pirko wrote:

quoted

This patch introduces OpenFlow-based filter. So far, the very essential
packet fields are supported (according to OpenFlow v1.4 spec).

This patch is only the first step. There is a lot of potential performance
improvements possible to implement. Also a lot of features are missing
now. They will be addressed in follow-up patches.

To the name of this classifier, I believe that "cls_openflow" is pretty
accurate. It is actually a OpenFlow classifier.

I agree with your statement above.
Small comments below:

quoted

Signed-off-by: Jiri Pirko <jiri@resnulli.us>
---

quoted

+struct of_flow_key {
+	int	indev_ifindex;
+	struct {
+		u8	src[ETH_ALEN];
+		u8	dst[ETH_ALEN];
+		__be16	type;
+	} eth;

Could you have got rid of "type" above given we always have
a "proto" field in tc that is checked at the core?

Sure it is possible. But I try to stick with the names used in OpenFlof
documentation.

quoted

+	struct {
+		u8	proto;
+	} ip;
+	union {
+		struct {
+			__be32 src;
+			__be32 dst;
+		} ipv4;
+		struct {
+			struct in6_addr src;
+			struct in6_addr dst;
+		} ipv6;
+	};
+	union {
+		struct {
+			__be16 src;
+			__be16 dst;
+		} tp;
+	};

Was there something else that was intended to go into that tp union?

Following fields are missing:

OXM_OF_SCTP_SRC
OXM_OF_SCTP_DST
OXM_OF_ICMPV4_TYPE
OXM_OF_ICMPV4_CODE

quoted

+static int of_dump(struct net *net, struct tcf_proto *tp, unsigned long fh,
+		   struct sk_buff *skb, struct tcmsg *t)
+{
+	struct cls_of_filter *f = (struct cls_of_filter *) fh;
+	struct nlattr *nest;
+	struct of_flow_key *key, *mask;
+
+	if (!f)
+		return skb->len;
+
+	t->tcm_handle = f->handle;
+
+	nest = nla_nest_start(skb, TCA_OPTIONS);
+	if (!nest)
+		goto nla_put_failure;
+
+	if (f->res.classid &&
+	    nla_put_u32(skb, TCA_BASIC_CLASSID, f->res.classid))
+		goto nla_put_failure;

I am sure you didnt meant the above;->

Ha. Will fix.

Other than that looks good. I dont mind saying
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

Thanks.

General comments:
so what happens if someone adds a new field? It sounds to me like
given it is tied to datapath match it will never be backward compatible
(i.e think old tc, new kernel vs new tc, old kernel)

Well if kernel does not understand the new field, it will simply ignore
it.

cheers,
jamal