Re: [PATCH net-next V5 4/4] net/sched: Introduce act_tunnel_key

2016-09-06 Thread Hadar Hen Zion 
On Tue, Sep 6, 2016 at 5:11 PM, Eric Dumazet  wrote:
> On Sun, 2016-09-04 at 13:55 +0300, Hadar Hen Zion wrote:
>> From: Amir Vadai 
>
> ...
>
>> +struct tcf_tunnel_key_params {
>> + struct rcu_head rcu;
>> + int tcft_action;
>> + int action;
>> + struct metadata_dst *tcft_enc_metadata;
>> +};
>> +
>> +struct tcf_tunnel_key {
>> + struct tc_action  common;
>> + struct tcf_tunnel_key_params *params;
>
> In order to please sparse you must add __rcu qualifier, as in :
>
> struct tcf_tunnel_key_params __rcu *params;

Thanks!


>
>> +};
>> +
>
> Thanks.
>
>
>

Re: [PATCH net-next V5 4/4] net/sched: Introduce act_tunnel_key

2016-09-06 Thread Eric Dumazet 
On Sun, 2016-09-04 at 13:55 +0300, Hadar Hen Zion wrote:
> From: Amir Vadai 

...

> +struct tcf_tunnel_key_params {
> + struct rcu_head rcu;
> + int tcft_action;
> + int action;
> + struct metadata_dst *tcft_enc_metadata;
> +};
> +
> +struct tcf_tunnel_key {
> + struct tc_action  common;
> + struct tcf_tunnel_key_params *params;

In order to please sparse you must add __rcu qualifier, as in :

struct tcf_tunnel_key_params __rcu *params;

> +};
> +

Thanks.

Re: [PATCH net-next V5 4/4] net/sched: Introduce act_tunnel_key

2016-09-06 Thread Jamal Hadi Salim 

On 16-09-06 07:03 AM, Hadar Hen Zion wrote:

On Tue, Sep 6, 2016 at 1:49 PM, Jamal Hadi Salim  wrote:



Please verify by running a test and send a packet or two
and verify that stats are incremented (I know it may sound silly to
ask but it is important).


Already tested that tc filter stats are working and incremented as expected :-)


I did see issue with per-cpu counters when i updated to rcu.
But the test was on a VM. So i changed skbmod to use global stats.
Maybe the kernel i was testing had issues.
Eric may have some insights.

cheers,
jamal

Re: [PATCH net-next V5 4/4] net/sched: Introduce act_tunnel_key

2016-09-06 Thread Hadar Hen Zion 
On Tue, Sep 6, 2016 at 1:49 PM, Jamal Hadi Salim  wrote:
> On 16-09-04 06:55 AM, Hadar Hen Zion wrote:
>>
>> From: Amir Vadai 
>>
>> This action could be used before redirecting packets to a shared tunnel
>> device, or when redirecting packets arriving from a such a device.
>>
>> The action will release the metadata created by the tunnel device
>> (decap), or set the metadata with the specified values for encap
>> operation.
>>
>> For example, the following flower filter will forward all ICMP packets
>> destined to 11.11.11.2 through the shared vxlan device 'vxlan0'. Before
>> redirecting, a metadata for the vxlan tunnel is created using the
>> tunnel_key action and it's arguments:
>>
>> $ filter add dev net0 protocol ip parent : \
>> flower \
>>   ip_proto 1 \
>>   dst_ip 11.11.11.2 \
>> action tunnel_key set \
>>   src_ip 11.11.0.1 \
>>   dst_ip 11.11.0.2 \
>>   id 11 \
>> action mirred egress redirect dev vxlan0
>>
>
>
> Syntax error above. Regardless:

ack, will be fixed.

> Please verify by running a test and send a packet or two
> and verify that stats are incremented (I know it may sound silly to
> ask but it is important).

Already tested that tc filter stats are working and incremented as expected :-)
.
>
>
>> +static int tunnel_key_act(struct sk_buff *skb, const struct tc_action *a,
>> + struct tcf_result *res)
>> +{
>> +   struct tcf_tunnel_key *t = to_tunnel_key(a);
>> +   struct tcf_tunnel_key_params *params;
>> +   int action;
>> +
>> +   rcu_read_lock();
>> +
>> +   params = rcu_dereference(t->params);
>> +
>> +   tcf_lastuse_update(>tcf_tm);
>> +   bstats_cpu_update(this_cpu_ptr(t->common.cpu_bstats), skb);
>> +   action = params->action;
>> +
>> +   switch (params->tcft_action) {
>> +   case TCA_TUNNEL_KEY_ACT_RELEASE:
>> +   skb_dst_drop(skb);
>> +   break;
>> +   case TCA_TUNNEL_KEY_ACT_SET:
>> +   skb_dst_drop(skb);
>> +   skb_dst_set(skb,
>> dst_clone(>tcft_enc_metadata->dst));
>> +   break;
>> +   default:
>> +   WARN_ONCE(1, "Bad tunnel_key action.\n");
>> +   break;
>
>
>
> slow path (_init()) is already checking for a bad tcft_act so it seems
> unnecessary to have the default.
> If you have to keep default would be useful to print the value as well.

ack.

>
> Other than that looks good.
> Acked-by: Jamal Hadi Salim 
>
> cheers,
> jamal

Re: [PATCH net-next V5 4/4] net/sched: Introduce act_tunnel_key

2016-09-06 Thread Jamal Hadi Salim 

On 16-09-04 06:55 AM, Hadar Hen Zion wrote:

From: Amir Vadai 

This action could be used before redirecting packets to a shared tunnel
device, or when redirecting packets arriving from a such a device.

The action will release the metadata created by the tunnel device
(decap), or set the metadata with the specified values for encap
operation.

For example, the following flower filter will forward all ICMP packets
destined to 11.11.11.2 through the shared vxlan device 'vxlan0'. Before
redirecting, a metadata for the vxlan tunnel is created using the
tunnel_key action and it's arguments:

$ filter add dev net0 protocol ip parent : \
flower \
  ip_proto 1 \
  dst_ip 11.11.11.2 \
action tunnel_key set \
  src_ip 11.11.0.1 \
  dst_ip 11.11.0.2 \
  id 11 \
action mirred egress redirect dev vxlan0




Syntax error above. Regardless:
Please verify by running a test and send a packet or two
and verify that stats are incremented (I know it may sound silly to
ask but it is important).


+static int tunnel_key_act(struct sk_buff *skb, const struct tc_action *a,
+ struct tcf_result *res)
+{
+   struct tcf_tunnel_key *t = to_tunnel_key(a);
+   struct tcf_tunnel_key_params *params;
+   int action;
+
+   rcu_read_lock();
+
+   params = rcu_dereference(t->params);
+
+   tcf_lastuse_update(>tcf_tm);
+   bstats_cpu_update(this_cpu_ptr(t->common.cpu_bstats), skb);
+   action = params->action;
+
+   switch (params->tcft_action) {
+   case TCA_TUNNEL_KEY_ACT_RELEASE:
+   skb_dst_drop(skb);
+   break;
+   case TCA_TUNNEL_KEY_ACT_SET:
+   skb_dst_drop(skb);
+   skb_dst_set(skb, dst_clone(>tcft_enc_metadata->dst));
+   break;
+   default:
+   WARN_ONCE(1, "Bad tunnel_key action.\n");
+   break;



slow path (_init()) is already checking for a bad tcft_act so it seems
unnecessary to have the default.
If you have to keep default would be useful to print the value as well.

Other than that looks good.
Acked-by: Jamal Hadi Salim 

cheers,
jamal

Re: [PATCH net-next V5 4/4] net/sched: Introduce act_tunnel_key

2016-09-05 Thread Hadar Hen Zion 
On Sun, Sep 4, 2016 at 9:19 PM, Rosen, Rami  wrote:
> Hi, Hadar,
>
>>For example, the following flower filter will forward all ICMP packets 
>>destined to 11.11.11.2 >through the shared vxlan device 'vxlan0'. Before 
>>redirecting, a metadata for the vxlan tunnel >is created using the tunnel_key 
>>action and it's arguments:
>
> Shouldn't it be "tc filter add dev ..."?

yes, I'll fix it to next version.

Thanks,
Hadar

>
>>$ filter add dev net0 protocol ip parent : \
>>flower \
>>  ip_proto 1 \
>>  dst_ip 11.11.11.2 \
>>action tunnel_key set \
>>  src_ip 11.11.0.1 \
>>  dst_ip 11.11.0.2 \
>>  id 11 \
>>action mirred egress redirect dev vxlan0
>
> Regards,
> Rami Rosen
> Intel Corporation
>

RE: [PATCH net-next V5 4/4] net/sched: Introduce act_tunnel_key

2016-09-04 Thread Rosen, Rami 
Hi, Hadar,

>For example, the following flower filter will forward all ICMP packets 
>destined to 11.11.11.2 >through the shared vxlan device 'vxlan0'. Before 
>redirecting, a metadata for the vxlan tunnel >is created using the tunnel_key 
>action and it's arguments:

Shouldn't it be "tc filter add dev ..."?

>$ filter add dev net0 protocol ip parent : \
>flower \
>  ip_proto 1 \
>  dst_ip 11.11.11.2 \
>action tunnel_key set \
>  src_ip 11.11.0.1 \
>  dst_ip 11.11.0.2 \
>  id 11 \
>action mirred egress redirect dev vxlan0

Regards,
Rami Rosen
Intel Corporation

Re: [PATCH net-next V5 4/4] net/sched: Introduce act_tunnel_key

2016-09-04 Thread kbuild test robot 
Hi Amir,

[auto build test WARNING on net-next/master]

url:
https://github.com/0day-ci/linux/commits/Hadar-Hen-Zion/net-sched-ip-tunnel-metadata-set-release-classify-by-using-TC/20160904-185825
reproduce:
# apt-get install sparse
make ARCH=x86_64 allmodconfig
make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

   include/linux/compiler.h:230:8: sparse: attribute 'no_sanitize_address': 
unknown attribute
>> net/sched/act_tunnel_key.c:38:18: sparse: incompatible types in comparison 
>> expression (different address spaces)
   net/sched/act_tunnel_key.c:169:22: sparse: incompatible types in comparison 
expression (different address spaces)
   net/sched/act_tunnel_key.c:197:18: sparse: incompatible types in comparison 
expression (different address spaces)
   net/sched/act_tunnel_key.c:248:18: sparse: incompatible types in comparison 
expression (different address spaces)

vim +38 net/sched/act_tunnel_key.c

22  #include 
23  
24  #define TUNNEL_KEY_TAB_MASK 15
25  
26  static int tunnel_key_net_id;
27  static struct tc_action_ops act_tunnel_key_ops;
28  
29  static int tunnel_key_act(struct sk_buff *skb, const struct tc_action 
*a,
30struct tcf_result *res)
31  {
32  struct tcf_tunnel_key *t = to_tunnel_key(a);
33  struct tcf_tunnel_key_params *params;
34  int action;
35  
36  rcu_read_lock();
37  
  > 38  params = rcu_dereference(t->params);
39  
40  tcf_lastuse_update(>tcf_tm);
41  bstats_cpu_update(this_cpu_ptr(t->common.cpu_bstats), skb);
42  action = params->action;
43  
44  switch (params->tcft_action) {
45  case TCA_TUNNEL_KEY_ACT_RELEASE:
46  skb_dst_drop(skb);

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation

Re: [PATCH net-next V5 4/4] net/sched: Introduce act_tunnel_key

2016-09-04 Thread Shmulik Ladkani 
On Sun,  4 Sep 2016 13:55:55 +0300, had...@mellanox.com wrote:
> From: Amir Vadai 
> 
> This action could be used before redirecting packets to a shared tunnel
> device, or when redirecting packets arriving from a such a device.
> 
> The action will release the metadata created by the tunnel device
> (decap), or set the metadata with the specified values for encap
> operation.
> 
> For example, the following flower filter will forward all ICMP packets
> destined to 11.11.11.2 through the shared vxlan device 'vxlan0'. Before
> redirecting, a metadata for the vxlan tunnel is created using the
> tunnel_key action and it's arguments:
> 
> $ filter add dev net0 protocol ip parent : \
> flower \
>   ip_proto 1 \
>   dst_ip 11.11.11.2 \
> action tunnel_key set \
>   src_ip 11.11.0.1 \
>   dst_ip 11.11.0.2 \
>   id 11 \
> action mirred egress redirect dev vxlan0
> 
> Signed-off-by: Amir Vadai 
> Signed-off-by: Hadar Hen Zion 

Reviewed-by: Shmulik Ladkani 

Thanks!

[PATCH net-next V5 4/4] net/sched: Introduce act_tunnel_key

2016-09-04 Thread Hadar Hen Zion 
From: Amir Vadai 

This action could be used before redirecting packets to a shared tunnel
device, or when redirecting packets arriving from a such a device.

The action will release the metadata created by the tunnel device
(decap), or set the metadata with the specified values for encap
operation.

For example, the following flower filter will forward all ICMP packets
destined to 11.11.11.2 through the shared vxlan device 'vxlan0'. Before
redirecting, a metadata for the vxlan tunnel is created using the
tunnel_key action and it's arguments:

$ filter add dev net0 protocol ip parent : \
flower \
  ip_proto 1 \
  dst_ip 11.11.11.2 \
action tunnel_key set \
  src_ip 11.11.0.1 \
  dst_ip 11.11.0.2 \
  id 11 \
action mirred egress redirect dev vxlan0

Signed-off-by: Amir Vadai 
Signed-off-by: Hadar Hen Zion 
---
 include/net/tc_act/tc_tunnel_key.h|  31 +++
 include/uapi/linux/tc_act/tc_tunnel_key.h |  42 
 net/sched/Kconfig |  11 +
 net/sched/Makefile|   1 +
 net/sched/act_tunnel_key.c| 348 ++
 5 files changed, 433 insertions(+)
 create mode 100644 include/net/tc_act/tc_tunnel_key.h
 create mode 100644 include/uapi/linux/tc_act/tc_tunnel_key.h
 create mode 100644 net/sched/act_tunnel_key.c

diff --git a/include/net/tc_act/tc_tunnel_key.h 
b/include/net/tc_act/tc_tunnel_key.h
new file mode 100644
index 000..7c34652
--- /dev/null
+++ b/include/net/tc_act/tc_tunnel_key.h
@@ -0,0 +1,31 @@
+/*
+ * Copyright (c) 2016, Amir Vadai 
+ * Copyright (c) 2016, Mellanox Technologies. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#ifndef __NET_TC_TUNNEL_KEY_H
+#define __NET_TC_TUNNEL_KEY_H
+
+#include 
+
+struct tcf_tunnel_key_params {
+   struct rcu_head rcu;
+   int tcft_action;
+   int action;
+   struct metadata_dst *tcft_enc_metadata;
+};
+
+struct tcf_tunnel_key {
+   struct tc_action  common;
+   struct tcf_tunnel_key_params *params;
+};
+
+#define to_tunnel_key(a) ((struct tcf_tunnel_key *)a)
+
+#endif /* __NET_TC_TUNNEL_KEY_H */
+
diff --git a/include/uapi/linux/tc_act/tc_tunnel_key.h 
b/include/uapi/linux/tc_act/tc_tunnel_key.h
new file mode 100644
index 000..f9ddf53
--- /dev/null
+++ b/include/uapi/linux/tc_act/tc_tunnel_key.h
@@ -0,0 +1,42 @@
+/*
+ * Copyright (c) 2016, Amir Vadai 
+ * Copyright (c) 2016, Mellanox Technologies. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#ifndef __LINUX_TC_TUNNEL_KEY_H
+#define __LINUX_TC_TUNNEL_KEY_H
+
+#include 
+
+#define TCA_ACT_TUNNEL_KEY 17
+
+#define TCA_TUNNEL_KEY_ACT_SET 1
+#define TCA_TUNNEL_KEY_ACT_RELEASE  2
+
+struct tc_tunnel_key {
+   tc_gen;
+   int t_action;
+};
+
+enum {
+   TCA_TUNNEL_KEY_UNSPEC,
+   TCA_TUNNEL_KEY_TM,
+   TCA_TUNNEL_KEY_PARMS,
+   TCA_TUNNEL_KEY_ENC_IPV4_SRC,/* be32 */
+   TCA_TUNNEL_KEY_ENC_IPV4_DST,/* be32 */
+   TCA_TUNNEL_KEY_ENC_IPV6_SRC,/* struct in6_addr */
+   TCA_TUNNEL_KEY_ENC_IPV6_DST,/* struct in6_addr */
+   TCA_TUNNEL_KEY_ENC_KEY_ID,  /* be64 */
+   TCA_TUNNEL_KEY_PAD,
+   __TCA_TUNNEL_KEY_MAX,
+};
+
+#define TCA_TUNNEL_KEY_MAX (__TCA_TUNNEL_KEY_MAX - 1)
+
+#endif
+
diff --git a/net/sched/Kconfig b/net/sched/Kconfig
index ccf931b..72e3426 100644
--- a/net/sched/Kconfig
+++ b/net/sched/Kconfig
@@ -761,6 +761,17 @@ config NET_ACT_IFE
  To compile this code as a module, choose M here: the
  module will be called act_ife.
 
+config NET_ACT_TUNNEL_KEY
+tristate "IP tunnel metadata manipulation"
+depends on NET_CLS_ACT
+---help---
+ Say Y here to set/release ip tunnel metadata.
+
+ If unsure, say N.
+
+ To compile this code as a module, choose M here: the
+ module will be called act_tunnel_key.
+
 config NET_IFE_SKBMARK
 tristate "Support to encoding decoding skb mark on IFE action"
 depends on NET_ACT_IFE
diff --git a/net/sched/Makefile b/net/sched/Makefile
index ae088a5..b9d046b 100644
--- a/net/sched/Makefile
+++ b/net/sched/Makefile
@@ -22,6 +22,7 @@ obj-$(CONFIG_NET_ACT_CONNMARK)+= act_connmark.o
 obj-$(CONFIG_NET_ACT_IFE)  += act_ife.o
 obj-$(CONFIG_NET_IFE_SKBMARK)  += act_meta_mark.o
 obj-$(CONFIG_NET_IFE_SKBPRIO)  += act_meta_skbprio.o