Re: [PATCH v2.29] datapath: Add basic MPLS support to kernel

On Wed, May 22, 2013 at 6:38 PM, Simon Horman [off-list ref] wrote:

On Tue, May 21, 2013 at 09:07:06AM -0700, Jesse Gross wrote:

quoted

On Mon, May 20, 2013 at 5:55 PM, Simon Horman [off-list ref] wrote:

quoted

On Fri, May 17, 2013 at 04:14:56PM -0700, Jesse Gross wrote:

quoted

On Fri, May 17, 2013 at 12:06 AM, Simon Horman [off-list ref] wrote:

quoted

+static int push_mpls(struct sk_buff *skb,
+                    const struct ovs_action_push_mpls *mpls)
+{
+       __be32 *new_mpls_lse;
+       int err;
+
+       err = make_writable(skb, skb->mac_len + MPLS_HLEN);
+       if (unlikely(err))
+               return err;
+
+       skb_push(skb, MPLS_HLEN);

What happens if there isn't enough headroom?

Good point. How about this?

        if (unlikely(skb->data<skb->head))
                return -EINVAL;
        skb_push(skb, MPLS_HLEN);

The amount of headroom is an internal kernel property so we can't
reject actions on the basis of it. We need to expand the headroom,
similar to __vlan_put_tag().

Thanks. I have changed the code around and it is now as follows.
I am a little unsure if the combination of make_writable() and
skb_cow_head() is sensible.

        err = make_writable(skb, skb->mac_len + MPLS_HLEN);
        if (unlikely(err))
                return err;

        if (skb_cow_head(skb, MPLS_HLEN) < 0)
                return -ENOMEM;

        skb_push(skb, MPLS_HLEN);

I would reverse the order of make_writable() and skb_cow_head() to
avoid possibly having to reallocate twice. I think you also don't need
to add MPLS_HLEN to the length in make_writable() because that
function is ensuring that existing packet data can be modified and the
MPLS label isn't there yet.