Re: [PATCH 4/6] mpls: VPLS support

[David Lamparter <equinox@diac24.net>]

On Mon, Aug 21, 2017 at 05:14:46PM +0200, Amine Kherbouche wrote:
> > +#include <net/ip_tunnels.h>
> 
> some headers are not needed.

Which ones?  net/ip_tunnels.h is needed for ip_tunnel_get_stats64().

> > +static netdev_tx_t vpls_xmit(struct sk_buff *skb, struct net_device *dev)
> > +{
> > +	int err = -EINVAL, ok_count = 0;
> > +	struct vpls_priv *priv = netdev_priv(dev);
> > +	struct vpls_info *vi;
> > +	struct pcpu_sw_netstats *stats;
> > +	size_t len = skb->len;
> > +
> > +	rcu_read_lock();
> > +	vi = skb_vpls_info(skb);
> > +
> > +	skb_orphan(skb);
> > +	skb_forward_csum(skb);
> > +
> > +	if (vi) {
> > +		err = vpls_xmit_wire(skb, dev, priv, vi->pw_label);
> > +		if (err)
> > +			goto out_err;
> > +	} else {
> 
> the rcu_read_lock() is just needed for the else statement right ?

Yeah, it's leftover from an earlier version where it was used for what
is now skb_vpls_info().  Fixed in next version.

> > +int vpls_rcv(struct sk_buff *skb, struct net_device *in_dev,
> > +	     struct packet_type *pt, struct mpls_route *rt,
> > +	     struct mpls_shim_hdr *hdr, struct net_device *orig_dev)
> > +{
> > +	struct net_device *dev = rt->rt_vpls_dev;
> > +	struct mpls_entry_decoded dec;
[...]
> > +	dec = mpls_entry_decode(hdr);
> 
> we can get dec.bos from mpls stack as a function param, no need to 
> decode the mpls header again.

I didn't do that for 2 reasons:
- mpls_entry_decode is a very small inlined function
- for future OAM GAL label support, it will need to decode a 2nd label

> > +static const struct net_device_ops vpls_netdev_ops = {
> > +	.ndo_init		= vpls_dev_init,
> > +	.ndo_open		= vpls_open,
> > +	.ndo_stop		= vpls_close,
> > +	.ndo_start_xmit		= vpls_xmit,
> > +	.ndo_change_mtu		= vpls_change_mtu,
> > +	.ndo_get_stats64	= ip_tunnel_get_stats64,
> > +	.ndo_set_rx_mode	= vpls_set_multicast_list,
> > +	.ndo_set_mac_address	= eth_mac_addr,
> > +	.ndo_features_check	= passthru_features_check,
> > +};
> 
> can you add .ndo_get_stats64 function ?

It uses the same stats as ip tunnels:
+	.ndo_get_stats64	= ip_tunnel_get_stats64,

I don't think copypasting improves code quality here(???)

Cheers,


-David