[ovs-dev] [PATCH 1/4] datapath: Add basic MPLS support to kernel

Simon Horman horms at verge.net.au
Sat Oct 27 05:29:00 UTC 2012


On Fri, Oct 26, 2012 at 05:08:31PM +0900, Isaku Yamahata wrote:
> Some inlined comments in case label == 0. I don't find any dependency on it.

Thanks Yamahata-san,

I agree with both of your comments, I should have fixed these up
when adding tracking of the stack depth (which was also suggested by you).
O will incorporate the changes you suggest below in the next version of
this patch.

> On Thu, Oct 18, 2012 at 06:02:18PM +0900, Simon Horman wrote:
> > diff --git a/datapath/datapath.c b/datapath/datapath.c
> > index a6915fb..e606ae1 100644
> > --- a/datapath/datapath.c
> > +++ b/datapath/datapath.c
> > @@ -74,6 +74,43 @@ int ovs_net_id __read_mostly;
> >  int (*ovs_dp_ioctl_hook)(struct net_device *dev, struct ifreq *rq, int cmd);
> >  EXPORT_SYMBOL(ovs_dp_ioctl_hook);
> >  
> > +void skb_cb_set_mpls_bos(struct sk_buff *skb)
> > +{
> > +	struct ethhdr *eth;
> > +	int nh_ofs;
> > +	__be16 dl_type = 0;
> > +
> > +	skb_reset_mac_header(skb);
> > +
> > +	eth = eth_hdr(skb);
> > +	nh_ofs = sizeof(struct ethhdr);
> > +	if (likely(eth->h_proto >= htons(ETH_TYPE_MIN))) {
> > +		dl_type = eth->h_proto;
> > +
> > +		while (dl_type == htons(ETH_P_8021Q) &&
> > +				skb->len >= nh_ofs + sizeof(struct vlan_hdr)) {
> > +			struct vlan_hdr *vh = (struct vlan_hdr*)(skb->data + nh_ofs);
> > +			dl_type = vh->h_vlan_encapsulated_proto;
> > +			nh_ofs += sizeof(struct vlan_hdr);
> > +		}
> > +
> > +		OVS_CB(skb)->mpls_bos = nh_ofs;
> > +	} else {
> > +		OVS_CB(skb)->mpls_bos = 0;
> > +	}
> > +}
> > +
> > +unsigned char *skb_cb_mpls_bos(const struct sk_buff *skb)
> > +{
> > +	return OVS_CB(skb)->mpls_bos ?
> > +		skb_mac_header(skb) + OVS_CB(skb)->mpls_bos : 0;
> > +}
> > +
> > +ptrdiff_t skb_cb_mpls_bos_offset(const struct sk_buff *skb)
> > +{
> > +	return OVS_CB(skb)->mpls_bos;
> > +}
> > +
> >  /**
> >   * DOC: Locking:
> >   *
> > @@ -663,6 +700,9 @@ static int validate_actions(const struct nlattr *attr,
> >  		static const u32 action_lens[OVS_ACTION_ATTR_MAX + 1] = {
> >  			[OVS_ACTION_ATTR_OUTPUT] = sizeof(u32),
> >  			[OVS_ACTION_ATTR_USERSPACE] = (u32)-1,
> > +			[OVS_ACTION_ATTR_PUSH_MPLS] = sizeof(struct ovs_action_push_mpls),
> > +			[OVS_ACTION_ATTR_POP_MPLS] = sizeof(__be16),
> > +			[OVS_ACTION_ATTR_SET_MPLS] = sizeof(__be32),
> >  			[OVS_ACTION_ATTR_PUSH_VLAN] = sizeof(struct ovs_action_push_vlan),
> >  			[OVS_ACTION_ATTR_POP_VLAN] = 0,
> >  			[OVS_ACTION_ATTR_SET] = (u32)-1,
> > @@ -691,6 +731,24 @@ static int validate_actions(const struct nlattr *attr,
> >  				return -EINVAL;
> >  			break;
> >  
> > +		case OVS_ACTION_ATTR_PUSH_MPLS: {
> > +			const struct ovs_action_push_mpls *mpls = nla_data(a);
> > +			if (mpls->mpls_ethertype != htons(ETH_P_MPLS_UC) &&
> > +			    mpls->mpls_ethertype != htons(ETH_P_MPLS_MC))
> > +				return -EINVAL;
> > +			break;
> > +		}
> > +
> > +		case OVS_ACTION_ATTR_POP_MPLS:
> > +			break;
> > +
> > +		case OVS_ACTION_ATTR_SET_MPLS: {
> > +			__be32 mpls_label = nla_get_be32(a);
> > +			if (mpls_label == htonl(0)) {
> > +				return -EINVAL;
> > +			}
> 
> Is this necessary?
> At least, this is inconsistent with case OVS_ACTION_ATTR_PUSH_MPLS.
> 
> 
> > +			break;
> > +		}
> >  
> >  		case OVS_ACTION_ATTR_POP_VLAN:
> >  			break;
> > @@ -805,6 +863,8 @@ static int ovs_packet_cmd_execute(struct sk_buff *skb, struct genl_info *info)
> >  	OVS_CB(packet)->flow = flow;
> >  	packet->priority = flow->key.phy.priority;
> >  
> > +	skb_cb_set_mpls_bos(packet);
> > +
> >  	rcu_read_lock();
> >  	dp = get_dp(sock_net(skb->sk), ovs_header->dp_ifindex);
> >  	err = -ENODEV;
> 
> 
> ... snip ...
> 
> > diff --git a/datapath/flow.h b/datapath/flow.h
> > index 02c563a..eb3fe49 100644
> > --- a/datapath/flow.h
> > +++ b/datapath/flow.h
> > @@ -53,6 +53,9 @@ struct sw_flow_key {
> >  		__be16 type;		/* Ethernet frame type. */
> >  	} eth;
> >  	struct {
> > +		__be32 top_label;	/* 0 if no MPLS, top label from stack otherwise */
> 
> I think that the validity of top_label is guaranteed by ethertype = MPLS.
> So top_label can be 0 as valid label value.
> 
> 
> > +	} mpls;
> > +	struct {
> >  		u8     proto;		/* IP protocol or lower 8 bits of ARP opcode. */
> >  		u8     tos;		/* IP ToS. */
> >  		u8     ttl;		/* IP TTL/hop limit. */
> > @@ -126,6 +129,10 @@ struct arp_eth_header {
> >  	unsigned char       ar_tip[4];		/* target IP address        */
> >  } __packed;
> >  
> > +#define ETH_TYPE_MIN 0x600
> > +
> > +#define MPLS_HLEN 4
> > +
> >  int ovs_flow_init(void);
> >  void ovs_flow_exit(void);
> >  
> 
> -- 
> yamahata
> 



More information about the dev mailing list