Date:	Thu, 4 Apr 2013 09:28:09 +0900
From:	Simon Horman <horms@...ge.net.au>
To:	Jesse Gross <jesse@...ira.com>
Cc:	"dev@...nvswitch.org" <dev@...nvswitch.org>,
	netdev <netdev@...r.kernel.org>,
	Jarno Rajahalme <jarno.rajahalme@....com>,
	Pravin B Shelar <pshelar@...ira.com>
Subject: Re: [PATCH v2] MPLS: Add limited GSO support

On Wed, Apr 03, 2013 at 04:51:38PM -0700, Jesse Gross wrote:
> On Wed, Apr 3, 2013 at 2:11 AM, Simon Horman <horms@...ge.net.au> wrote:
> > In the case where a non-MPLS packet is recieved and an MPLS stack is
> > added it may well be the case that the original skb is GSO but the
> > NIC used for transmit does not support GSO of MPLS packets.
> >
> > The aim of this code is to provide GSO in software for MPLS packets
> > whose skbs are GSO.
> >
> > When an implementation adds an MPLS stack to a non-MPLS packet it should do
> > the following to skb metadata:
> >
> > * Set skb_mac_header(skb)->protocol to the new MPLS ethertype.
> >   That is, either ETH_P_MPLS_UC or ETH_P_MPLS_MC.
> >
> > * Leave skb->protocol as the old non-MPLS ethertype.
> >
> > * Set skb->encapsulation = 1.
> >
> >   This may not strictly be necessary as I believe that checking
> >   skb_mac_header(skb)->protocol and skb->protocol should be necessary and
> >   sufficient.
> >
> >   However, it does seem to fit nicely with the current implementation of
> >   dev_hard_start_xmit() where the more expensive check of
> >   skb_mac_header(skb)->protocol may be guarded by an existing check of
> >   skb->encapsulation.
> >
> > One aspect of this patch that I am unsure about is the modification I have
> > made to skb_segment(). This seems to be necessary as checskum accelearation
> > may no longer be possible as the packet has changed to be MPLS from some
> > other packet type which may have been supported by the hardware in use.
> >
> > I will post a patch, "[PATCH v3.24] datapath: Add basic MPLS support to
> > kernel" which adds MPLS support to the kernel datapath of Open vSwtich.
> > That patch sets the above requirements in
> > datapath/actions.c:set_ethertype() and was used to exercise the MPLS GSO
> > code. The datapath patch is against the Open vSwtich tree but it is
> > intended that it be added to the Open vSwtich code present in the mainline
> > Linux kernel at some point.
> >
> > Suggested by Jesse Gross. Based heavily on "v4 GRE: Add TCP segmentation
> > offload for GRE" by Pravin B Shelar.
> >
> > Cc: Jesse Gross <jesse@...ira.com>
> > Cc: Pravin B Shelar <pshelar@...ira.com>
> > Signed-off-by: Simon Horman <horms@...ge.net.au>
> 
> MPLS is very similar to both the Ethernet header and vlans in that GSO
> only requires replication without any modification.  That means that
> if we look at the mac_len as containing all three then we can just
> copy it without any special knowledge.  I don't know that we carefully
> maintain mac_len in all places but you are already doing that in your
> MPLS patches.

At least for the cases that I am aware of I think that mac_len is
predictable. But I'm a little unsure of what you are getting at here.

> The other piece at that point is getting the inner protocol.  I'm
> worried that using skb->protocol for this will break things that try
> to use it for filtering.  It also means that depending on whether an
> MPLS packet is locally sourced or not skb->protocol may be different
> because we won't always be able to find the inner header.  I think
> this will require a careful definition of that field to make it
> consistent.

Yes, I agree. I was hoping that posting the patch to netdev would
result in some analysis of this.

Another idea I had would be to a new element to struct sk_buff to
explicitly hold the encapsulated protocol for (cases like) MPLS.
But it seems that it would be nicer to re-use the protocol element
if possible.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists