Re: [PATCH net-next 3/3] net/sched: Introduce act_iptunnel

[Amir Vadai <amir@vadai.me>]

On Tue, Aug 23, 2016 at 08:37:07AM -0400, Jamal Hadi Salim wrote:
> On 16-08-22 10:38 AM, Amir Vadai wrote:
> > 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
> > iptunnel action and it's arguments:
> > 
> > $ filter add dev net0 protocol ip parent ffff: \
> >     flower \
> >       ip_proto 1 \
> >       dst_ip 11.11.11.2 \
> >     action iptunnel encap \
> >       src_ip 11.11.0.1 \
> >       dst_ip 11.11.0.2 \
> >       id 11 \
> >     action mirred egress redirect dev vxlan0
> 
> The noun "ip tunnel" is a little misleading. Unless you can use
> this for other types of tunnels (ipip, etc). If this is specific
> for just vxlan and metadata setting then some name like vxlanmeta
> or something else that signifies both metadata de/encap + vxlan would
> be helpful.
Yeh, this name is not the best...
The action is not vxlan specific, it should be good for all the
ip tunnel interfaces that use metadata for the outer headers.
I will rename it to something like mdtunnel - unless someone has a
better suggestion.

> 
> 
> > +static int tcf_iptunnel(struct sk_buff *skb, const struct tc_action *a,
> > +			struct tcf_result *res)
> 
> Can you rename this function to something more grep-able
> like tcf_iptunnel_run or tcf_iptunnel_exec?
> You already have a data structure called tcf_iptunnel
ack

> 
> > +{
> > +	struct tcf_iptunnel *t = to_iptunnel(a);
> > +	int action;
> > +
> > +	spin_lock(&t->tcf_lock);
> > +	tcf_lastuse_update(&t->tcf_tm);
> > +	bstats_update(&t->tcf_bstats, skb);
> > +	action = t->tcf_action;
> > +
> > +	switch (t->tcft_action) {
> > +	case TCA_IPTUNNEL_ACT_DECAP:
> > +		skb_dst_set_noref(skb, NULL);
> > +		break;
> 
> 
> So the real decap is going to be at the vxlan dev?
yes, the action here will just cleanup after the tunnel device will peel
off the outer headers and place it in the metadata. This metadata was
used by the classifier and now can be released.

> 
> 
> > +static struct metadata_dst *iptunnel_alloc(struct tcf_iptunnel *t,
> > +					   __be32 saddr, __be32 daddr,
> > +					   __be64 key_id)
> > +{
> > +	struct ip_tunnel_info *tun_info;
> > +	struct metadata_dst *metadata;
> > +
> > +	metadata = metadata_dst_alloc(0, GFP_KERNEL);
> > +	if (!metadata)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	tun_info = &metadata->u.tun_info;
> > +	tun_info->mode = IP_TUNNEL_INFO_TX;
> > +
> 
> More grep-ability tun_info sounds and i think is used by tun netdev.
ack

> 
> Otherwise looks good (although i still think this wouldve scaled better
> if you didnt depend on presence of vxlan dev).
now I start to have some regrets :)
But I don't see a good enough reason to duplicate code, since I can't
point my finger on a performance problem with using the existing code.

Thanks,
Amir

> 
> cheers,
> jamal