[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20150330062632.GB2045@nanopsycho.orion>
Date: Mon, 30 Mar 2015 08:26:32 +0200
From: Jiri Pirko <jiri@...nulli.us>
To: Jamal Hadi Salim <jhs@...atatu.com>
Cc: netdev@...r.kernel.org, davem@...emloft.net, tgraf@...g.ch,
jesse@...ira.com
Subject: Re: [patch net-next] tc: introduce OpenFlow classifier
Mon, Mar 30, 2015 at 02:39:31AM CEST, jhs@...atatu.com wrote:
>Jiri,
>
>Comments below
>
>On 03/26/15 08:53, Jiri Pirko wrote:
>>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:
>
>
>>Signed-off-by: Jiri Pirko <jiri@...nulli.us>
>>---
>
>>+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.
>
>
>>+ 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
>
>>+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@...atatu.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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists