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

On Tue, Sep 6, 2016 at 1:49 PM, Jamal Hadi Salim [off-list ref] wrote:

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

quoted

From: Amir Vadai <redacted>

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 ffff: \
    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 :-)
.

quoted

+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(&t->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(&params->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 <jhs@mojatatu.com>

cheers,
jamal