[patch net-next v2 2/4] net/sched: Introduce sample tc action

2017-01-23 Thread Jiri Pirko
From: Yotam Gigi 

This action allows the user to sample traffic matched by tc classifier.
The sampling consists of choosing packets randomly and sampling them using
the psample module. The user can configure the psample group number, the
sampling rate and the packet's truncation (to save kernel-user traffic).

Example:
To sample ingress traffic from interface eth1, one may use the commands:

tc qdisc add dev eth1 handle : ingress

tc filter add dev eth1 parent : \
   matchall action sample rate 12 group 4

Where the first command adds an ingress qdisc and the second starts
sampling randomly with an average of one sampled packet per 12 packets on
dev eth1 to psample group 4.

Signed-off-by: Yotam Gigi 
Signed-off-by: Jiri Pirko 
Acked-by: Jamal Hadi Salim 
---
 include/net/tc_act/tc_sample.h|  50 +++
 include/uapi/linux/tc_act/Kbuild  |   1 +
 include/uapi/linux/tc_act/tc_sample.h |  26 
 net/sched/Kconfig |  12 ++
 net/sched/Makefile|   1 +
 net/sched/act_sample.c| 274 ++
 6 files changed, 364 insertions(+)
 create mode 100644 include/net/tc_act/tc_sample.h
 create mode 100644 include/uapi/linux/tc_act/tc_sample.h
 create mode 100644 net/sched/act_sample.c

diff --git a/include/net/tc_act/tc_sample.h b/include/net/tc_act/tc_sample.h
new file mode 100644
index 000..89e9305
--- /dev/null
+++ b/include/net/tc_act/tc_sample.h
@@ -0,0 +1,50 @@
+#ifndef __NET_TC_SAMPLE_H
+#define __NET_TC_SAMPLE_H
+
+#include 
+#include 
+#include 
+
+struct tcf_sample {
+   struct tc_action common;
+   u32 rate;
+   bool truncate;
+   u32 trunc_size;
+   struct psample_group __rcu *psample_group;
+   u32 psample_group_num;
+   struct list_head tcfm_list;
+   struct rcu_head rcu;
+};
+#define to_sample(a) ((struct tcf_sample *)a)
+
+static inline bool is_tcf_sample(const struct tc_action *a)
+{
+#ifdef CONFIG_NET_CLS_ACT
+   return a->ops && a->ops->type == TCA_ACT_SAMPLE;
+#else
+   return false;
+#endif
+}
+
+static inline __u32 tcf_sample_rate(const struct tc_action *a)
+{
+   return to_sample(a)->rate;
+}
+
+static inline bool tcf_sample_truncate(const struct tc_action *a)
+{
+   return to_sample(a)->truncate;
+}
+
+static inline int tcf_sample_trunc_size(const struct tc_action *a)
+{
+   return to_sample(a)->trunc_size;
+}
+
+static inline struct psample_group *
+tcf_sample_psample_group(const struct tc_action *a)
+{
+   return rcu_dereference(to_sample(a)->psample_group);
+}
+
+#endif /* __NET_TC_SAMPLE_H */
diff --git a/include/uapi/linux/tc_act/Kbuild b/include/uapi/linux/tc_act/Kbuild
index e3db740..ba62ddf 100644
--- a/include/uapi/linux/tc_act/Kbuild
+++ b/include/uapi/linux/tc_act/Kbuild
@@ -4,6 +4,7 @@ header-y += tc_defact.h
 header-y += tc_gact.h
 header-y += tc_ipt.h
 header-y += tc_mirred.h
+header-y += tc_sample.h
 header-y += tc_nat.h
 header-y += tc_pedit.h
 header-y += tc_skbedit.h
diff --git a/include/uapi/linux/tc_act/tc_sample.h 
b/include/uapi/linux/tc_act/tc_sample.h
new file mode 100644
index 000..edc9058
--- /dev/null
+++ b/include/uapi/linux/tc_act/tc_sample.h
@@ -0,0 +1,26 @@
+#ifndef __LINUX_TC_SAMPLE_H
+#define __LINUX_TC_SAMPLE_H
+
+#include 
+#include 
+#include 
+
+#define TCA_ACT_SAMPLE 26
+
+struct tc_sample {
+   tc_gen;
+};
+
+enum {
+   TCA_SAMPLE_UNSPEC,
+   TCA_SAMPLE_TM,
+   TCA_SAMPLE_PARMS,
+   TCA_SAMPLE_RATE,
+   TCA_SAMPLE_TRUNC_SIZE,
+   TCA_SAMPLE_PSAMPLE_GROUP,
+   TCA_SAMPLE_PAD,
+   __TCA_SAMPLE_MAX
+};
+#define TCA_SAMPLE_MAX (__TCA_SAMPLE_MAX - 1)
+
+#endif
diff --git a/net/sched/Kconfig b/net/sched/Kconfig
index a9aa38d..72cfa3a 100644
--- a/net/sched/Kconfig
+++ b/net/sched/Kconfig
@@ -650,6 +650,18 @@ config NET_ACT_MIRRED
  To compile this code as a module, choose M here: the
  module will be called act_mirred.
 
+config NET_ACT_SAMPLE
+tristate "Traffic Sampling"
+depends on NET_CLS_ACT
+select PSAMPLE
+---help---
+ Say Y here to allow packet sampling tc action. The packet sample
+ action consists of statistically choosing packets and sampling
+ them using the psample module.
+
+ To compile this code as a module, choose M here: the
+ module will be called act_sample.
+
 config NET_ACT_IPT
 tristate "IPtables targets"
 depends on NET_CLS_ACT && NETFILTER && IP_NF_IPTABLES
diff --git a/net/sched/Makefile b/net/sched/Makefile
index 4bdda36..7b915d2 100644
--- a/net/sched/Makefile
+++ b/net/sched/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_NET_CLS_ACT) += act_api.o
 obj-$(CONFIG_NET_ACT_POLICE)   += act_police.o
 obj-$(CONFIG_NET_ACT_GACT) += act_gact.o
 obj-$(CONFIG_NET_ACT_MIRRED)   += act_mirred.o
+obj-$(CONFIG_NET_ACT_SAMPLE)   += act_sample.o
 obj-$(CONFIG_NET_ACT_IPT)  += act_ipt.o
 obj-$(CONFIG_NET_ACT_NAT)  += act_nat.o
 obj-$(CONF

Re: [patch net-next v2 2/4] net/sched: Introduce sample tc action

2017-01-24 Thread Simon Horman
On Mon, Jan 23, 2017 at 11:07:09AM +0100, Jiri Pirko wrote:
> From: Yotam Gigi 
> 
> This action allows the user to sample traffic matched by tc classifier.
> The sampling consists of choosing packets randomly and sampling them using
> the psample module. The user can configure the psample group number, the
> sampling rate and the packet's truncation (to save kernel-user traffic).
> 
> Example:
> To sample ingress traffic from interface eth1, one may use the commands:
> 
> tc qdisc add dev eth1 handle : ingress
> 
> tc filter add dev eth1 parent : \
>  matchall action sample rate 12 group 4
> 
> Where the first command adds an ingress qdisc and the second starts
> sampling randomly with an average of one sampled packet per 12 packets on
> dev eth1 to psample group 4.
> 
> Signed-off-by: Yotam Gigi 
> Signed-off-by: Jiri Pirko 
> Acked-by: Jamal Hadi Salim 

Reviewed-by: Simon Horman 

Is the tc user-space (iproute2) code available yet?



RE: [patch net-next v2 2/4] net/sched: Introduce sample tc action

2017-01-24 Thread Yotam Gigi
>-Original Message-
>From: Simon Horman [mailto:simon.hor...@netronome.com]
>Sent: Tuesday, January 24, 2017 10:33 AM
>To: Jiri Pirko 
>Cc: netdev@vger.kernel.org; da...@davemloft.net; Yotam Gigi
>; Ido Schimmel ; Elad Raz
>; Nogah Frankel ; Or Gerlitz
>; j...@mojatatu.com; geert+rene...@glider.be;
>step...@networkplumber.org; xiyou.wangc...@gmail.com; li...@roeck-us.net;
>ro...@cumulusnetworks.com; john.fastab...@gmail.com; m...@mojatatu.com
>Subject: Re: [patch net-next v2 2/4] net/sched: Introduce sample tc action
>
>On Mon, Jan 23, 2017 at 11:07:09AM +0100, Jiri Pirko wrote:
>> From: Yotam Gigi 
>>
>> This action allows the user to sample traffic matched by tc classifier.
>> The sampling consists of choosing packets randomly and sampling them using
>> the psample module. The user can configure the psample group number, the
>> sampling rate and the packet's truncation (to save kernel-user traffic).
>>
>> Example:
>> To sample ingress traffic from interface eth1, one may use the commands:
>>
>> tc qdisc add dev eth1 handle : ingress
>>
>> tc filter add dev eth1 parent : \
>> matchall action sample rate 12 group 4
>>
>> Where the first command adds an ingress qdisc and the second starts
>> sampling randomly with an average of one sampled packet per 12 packets on
>> dev eth1 to psample group 4.
>>
>> Signed-off-by: Yotam Gigi 
>> Signed-off-by: Jiri Pirko 
>> Acked-by: Jamal Hadi Salim 
>
>Reviewed-by: Simon Horman 
>
>Is the tc user-space (iproute2) code available yet?

Yes it is. I thought about sending it the moment the kernel patches gets 
accepted.

Do you want it to send it directly to you?


Re: [patch net-next v2 2/4] net/sched: Introduce sample tc action

2017-01-24 Thread Simon Horman
On Tue, Jan 24, 2017 at 08:39:37AM +, Yotam Gigi wrote:
> >-Original Message-
> >From: Simon Horman [mailto:simon.hor...@netronome.com]
> >Sent: Tuesday, January 24, 2017 10:33 AM
> >To: Jiri Pirko 
> >Cc: netdev@vger.kernel.org; da...@davemloft.net; Yotam Gigi
> >; Ido Schimmel ; Elad Raz
> >; Nogah Frankel ; Or Gerlitz
> >; j...@mojatatu.com; geert+rene...@glider.be;
> >step...@networkplumber.org; xiyou.wangc...@gmail.com; li...@roeck-us.net;
> >ro...@cumulusnetworks.com; john.fastab...@gmail.com; m...@mojatatu.com
> >Subject: Re: [patch net-next v2 2/4] net/sched: Introduce sample tc action
> >
> >On Mon, Jan 23, 2017 at 11:07:09AM +0100, Jiri Pirko wrote:
> >> From: Yotam Gigi 
> >>
> >> This action allows the user to sample traffic matched by tc classifier.
> >> The sampling consists of choosing packets randomly and sampling them using
> >> the psample module. The user can configure the psample group number, the
> >> sampling rate and the packet's truncation (to save kernel-user traffic).
> >>
> >> Example:
> >> To sample ingress traffic from interface eth1, one may use the commands:
> >>
> >> tc qdisc add dev eth1 handle : ingress
> >>
> >> tc filter add dev eth1 parent : \
> >>   matchall action sample rate 12 group 4
> >>
> >> Where the first command adds an ingress qdisc and the second starts
> >> sampling randomly with an average of one sampled packet per 12 packets on
> >> dev eth1 to psample group 4.
> >>
> >> Signed-off-by: Yotam Gigi 
> >> Signed-off-by: Jiri Pirko 
> >> Acked-by: Jamal Hadi Salim 
> >
> >Reviewed-by: Simon Horman 
> >
> >Is the tc user-space (iproute2) code available yet?
> 
> Yes it is. I thought about sending it the moment the kernel patches gets 
> accepted.
> 
> Do you want it to send it directly to you?

I'm happy to wait for now.


Re: [patch net-next v2 2/4] net/sched: Introduce sample tc action

2017-01-25 Thread Cong Wang
On Mon, Jan 23, 2017 at 2:07 AM, Jiri Pirko  wrote:
> +
> +static int tcf_sample_init(struct net *net, struct nlattr *nla,
> +  struct nlattr *est, struct tc_action **a, int ovr,
> +  int bind)
> +{
> +   struct tc_action_net *tn = net_generic(net, sample_net_id);
> +   struct nlattr *tb[TCA_SAMPLE_MAX + 1];
> +   struct psample_group *psample_group;
> +   struct tc_sample *parm;
> +   struct tcf_sample *s;
> +   bool exists = false;
> +   int ret;
> +
> +   if (!nla)
> +   return -EINVAL;
> +   ret = nla_parse_nested(tb, TCA_SAMPLE_MAX, nla, sample_policy);
> +   if (ret < 0)
> +   return ret;
> +   if (!tb[TCA_SAMPLE_PARMS] || !tb[TCA_SAMPLE_RATE] ||
> +   !tb[TCA_SAMPLE_PSAMPLE_GROUP])
> +   return -EINVAL;
> +
> +   parm = nla_data(tb[TCA_SAMPLE_PARMS]);
> +
> +   exists = tcf_hash_check(tn, parm->index, a, bind);
> +   if (exists && bind)
> +   return 0;
> +
> +   if (!exists) {
> +   ret = tcf_hash_create(tn, parm->index, est, a,
> + &act_sample_ops, bind, false);
> +   if (ret)
> +   return ret;
> +   ret = ACT_P_CREATED;
> +   } else {
> +   tcf_hash_release(*a, bind);
> +   if (!ovr)
> +   return -EEXIST;
> +   }
> +   s = to_sample(*a);
> +
> +   ASSERT_RTNL();

Copy-n-paste from mirred aciton? This is not needed for you, mirred
needs it because of target netdevice.


> +   s->tcf_action = parm->action;
> +   s->rate = nla_get_u32(tb[TCA_SAMPLE_RATE]);
> +   s->psample_group_num = nla_get_u32(tb[TCA_SAMPLE_PSAMPLE_GROUP]);
> +   psample_group = psample_group_get(net, s->psample_group_num);
> +   if (!psample_group)
> +   return -ENOMEM;

I don't think you can just return here, needs tcf_hash_cleanup() for create
case, right?


> +   RCU_INIT_POINTER(s->psample_group, psample_group);
> +
> +   if (tb[TCA_SAMPLE_TRUNC_SIZE]) {
> +   s->truncate = true;
> +   s->trunc_size = nla_get_u32(tb[TCA_SAMPLE_TRUNC_SIZE]);
> +   }


Do you need tcf_lock here if RCU only protects ->psample_group??


> +
> +   if (ret == ACT_P_CREATED)
> +   tcf_hash_insert(tn, *a);
> +   return ret;
> +}
> +


Thanks.


RE: [patch net-next v2 2/4] net/sched: Introduce sample tc action

2017-01-26 Thread Yotam Gigi
>-Original Message-
>From: Cong Wang [mailto:xiyou.wangc...@gmail.com]
>Sent: Thursday, January 26, 2017 1:30 AM
>To: Jiri Pirko 
>Cc: Linux Kernel Network Developers ; David Miller
>; Yotam Gigi ; Ido Schimmel
>; Elad Raz ; Nogah Frankel
>; Or Gerlitz ; Jamal Hadi Salim
>; geert+rene...@glider.be; Stephen Hemminger
>; Guenter Roeck ; Roopa
>Prabhu ; John Fastabend
>; Simon Horman ;
>Roman Mashak 
>Subject: Re: [patch net-next v2 2/4] net/sched: Introduce sample tc action
>
>On Mon, Jan 23, 2017 at 2:07 AM, Jiri Pirko  wrote:
>> +
>> +static int tcf_sample_init(struct net *net, struct nlattr *nla,
>> +  struct nlattr *est, struct tc_action **a, int ovr,
>> +  int bind)
>> +{
>> +   struct tc_action_net *tn = net_generic(net, sample_net_id);
>> +   struct nlattr *tb[TCA_SAMPLE_MAX + 1];
>> +   struct psample_group *psample_group;
>> +   struct tc_sample *parm;
>> +   struct tcf_sample *s;
>> +   bool exists = false;
>> +   int ret;
>> +
>> +   if (!nla)
>> +   return -EINVAL;
>> +   ret = nla_parse_nested(tb, TCA_SAMPLE_MAX, nla, sample_policy);
>> +   if (ret < 0)
>> +   return ret;
>> +   if (!tb[TCA_SAMPLE_PARMS] || !tb[TCA_SAMPLE_RATE] ||
>> +   !tb[TCA_SAMPLE_PSAMPLE_GROUP])
>> +   return -EINVAL;
>> +
>> +   parm = nla_data(tb[TCA_SAMPLE_PARMS]);
>> +
>> +   exists = tcf_hash_check(tn, parm->index, a, bind);
>> +   if (exists && bind)
>> +   return 0;
>> +
>> +   if (!exists) {
>> +   ret = tcf_hash_create(tn, parm->index, est, a,
>> + &act_sample_ops, bind, false);
>> +   if (ret)
>> +   return ret;
>> +   ret = ACT_P_CREATED;
>> +   } else {
>> +   tcf_hash_release(*a, bind);
>> +   if (!ovr)
>> +   return -EEXIST;
>> +   }
>> +   s = to_sample(*a);
>> +
>> +   ASSERT_RTNL();
>
>Copy-n-paste from mirred aciton? This is not needed for you, mirred
>needs it because of target netdevice.

Ho, you are right. I will remove it.

>
>
>> +   s->tcf_action = parm->action;
>> +   s->rate = nla_get_u32(tb[TCA_SAMPLE_RATE]);
>> +   s->psample_group_num =
>nla_get_u32(tb[TCA_SAMPLE_PSAMPLE_GROUP]);
>> +   psample_group = psample_group_get(net, s->psample_group_num);
>> +   if (!psample_group)
>> +   return -ENOMEM;
>
>I don't think you can just return here, needs tcf_hash_cleanup() for create
>case, right?

Will fix.

>
>
>> +   RCU_INIT_POINTER(s->psample_group, psample_group);
>> +
>> +   if (tb[TCA_SAMPLE_TRUNC_SIZE]) {
>> +   s->truncate = true;
>> +   s->trunc_size = nla_get_u32(tb[TCA_SAMPLE_TRUNC_SIZE]);
>> +   }
>
>
>Do you need tcf_lock here if RCU only protects ->psample_group??
>

You are right. I need to protect in case of update.

I will send a fixup patch in the following days. Thanks!

>
>> +
>> +   if (ret == ACT_P_CREATED)
>> +   tcf_hash_insert(tn, *a);
>> +   return ret;
>> +}
>> +
>
>
>Thanks.


RE: [patch net-next v2 2/4] net/sched: Introduce sample tc action

2017-01-30 Thread Yotam Gigi
>-Original Message-
>From: Yotam Gigi
>Sent: Friday, January 27, 2017 8:09 AM
>To: 'Cong Wang' ; Jiri Pirko 
>Cc: Linux Kernel Network Developers ; David Miller
>; Ido Schimmel ; Elad Raz
>; Nogah Frankel ; Or Gerlitz
>; Jamal Hadi Salim ;
>geert+rene...@glider.be; Stephen Hemminger ;
>Guenter Roeck ; Roopa Prabhu
>; John Fastabend ;
>Simon Horman ; Roman Mashak
>
>Subject: RE: [patch net-next v2 2/4] net/sched: Introduce sample tc action
>
>>-Original Message-
>>From: Cong Wang [mailto:xiyou.wangc...@gmail.com]
>>Sent: Thursday, January 26, 2017 1:30 AM
>>To: Jiri Pirko 
>>Cc: Linux Kernel Network Developers ; David Miller
>>; Yotam Gigi ; Ido Schimmel
>>; Elad Raz ; Nogah Frankel
>>; Or Gerlitz ; Jamal Hadi Salim
>>; geert+rene...@glider.be; Stephen Hemminger
>>; Guenter Roeck ; Roopa
>>Prabhu ; John Fastabend
>>; Simon Horman
>;
>>Roman Mashak 
>>Subject: Re: [patch net-next v2 2/4] net/sched: Introduce sample tc action
>>
>>On Mon, Jan 23, 2017 at 2:07 AM, Jiri Pirko  wrote:
>>> +
>>> +static int tcf_sample_init(struct net *net, struct nlattr *nla,
>>> +  struct nlattr *est, struct tc_action **a, int 
>>> ovr,
>>> +  int bind)
>>> +{
>>> +   struct tc_action_net *tn = net_generic(net, sample_net_id);
>>> +   struct nlattr *tb[TCA_SAMPLE_MAX + 1];
>>> +   struct psample_group *psample_group;
>>> +   struct tc_sample *parm;
>>> +   struct tcf_sample *s;
>>> +   bool exists = false;
>>> +   int ret;
>>> +
>>> +   if (!nla)
>>> +   return -EINVAL;
>>> +   ret = nla_parse_nested(tb, TCA_SAMPLE_MAX, nla, sample_policy);
>>> +   if (ret < 0)
>>> +   return ret;
>>> +   if (!tb[TCA_SAMPLE_PARMS] || !tb[TCA_SAMPLE_RATE] ||
>>> +   !tb[TCA_SAMPLE_PSAMPLE_GROUP])
>>> +   return -EINVAL;
>>> +
>>> +   parm = nla_data(tb[TCA_SAMPLE_PARMS]);
>>> +
>>> +   exists = tcf_hash_check(tn, parm->index, a, bind);
>>> +   if (exists && bind)
>>> +   return 0;
>>> +
>>> +   if (!exists) {
>>> +   ret = tcf_hash_create(tn, parm->index, est, a,
>>> + &act_sample_ops, bind, false);
>>> +   if (ret)
>>> +   return ret;
>>> +   ret = ACT_P_CREATED;
>>> +   } else {
>>> +   tcf_hash_release(*a, bind);
>>> +   if (!ovr)
>>> +   return -EEXIST;
>>> +   }
>>> +   s = to_sample(*a);
>>> +
>>> +   ASSERT_RTNL();
>>
>>Copy-n-paste from mirred aciton? This is not needed for you, mirred
>>needs it because of target netdevice.
>
>Ho, you are right. I will remove it.
>
>>
>>
>>> +   s->tcf_action = parm->action;
>>> +   s->rate = nla_get_u32(tb[TCA_SAMPLE_RATE]);
>>> +   s->psample_group_num =
>>nla_get_u32(tb[TCA_SAMPLE_PSAMPLE_GROUP]);
>>> +   psample_group = psample_group_get(net, s->psample_group_num);
>>> +   if (!psample_group)
>>> +   return -ENOMEM;
>>
>>I don't think you can just return here, needs tcf_hash_cleanup() for create
>>case, right?
>
>Will fix.
>
>>
>>
>>> +   RCU_INIT_POINTER(s->psample_group, psample_group);
>>> +
>>> +   if (tb[TCA_SAMPLE_TRUNC_SIZE]) {
>>> +   s->truncate = true;
>>> +   s->trunc_size = nla_get_u32(tb[TCA_SAMPLE_TRUNC_SIZE]);
>>> +   }
>>
>>
>>Do you need tcf_lock here if RCU only protects ->psample_group??
>>
>
>You are right. I need to protect in case of update.

Cong, after some thinking I think I don't really need the tcf_lock here. I
don't really care if the truncate, trunc_size, rate and tcf_action are
consistent among themselves - the only parameter that I care about is the
psample_group pointer, and it is protected via RCU. As far as I see, there is
no need to lock here.

I do need to take the tcf_lock to protect the statistics update in the 
tcf_sample_act code, as far as I see.

Am I missing something?

>
>I will send a fixup patch in the following days. Thanks!
>
>>
>>> +
>>> +   if (ret == ACT_P_CREATED)
>>> +   tcf_hash_insert(tn, *a);
>>> +   return ret;
>>> +}
>>> +
>>
>>
>>Thanks.


Re: [patch net-next v2 2/4] net/sched: Introduce sample tc action

2017-01-30 Thread Cong Wang
On Mon, Jan 30, 2017 at 8:49 AM, Yotam Gigi  wrote:
>
> Cong, after some thinking I think I don't really need the tcf_lock here. I
> don't really care if the truncate, trunc_size, rate and tcf_action are
> consistent among themselves - the only parameter that I care about is the
> psample_group pointer, and it is protected via RCU. As far as I see, there is
> no need to lock here.

OK, I trust you, you should know the logic better than me.

>
> I do need to take the tcf_lock to protect the statistics update in the
> tcf_sample_act code, as far as I see.
>

Hm? You use percpu stats, so you don't need spinlock.