Message-ID: <CALnjE+o_bcNbU6kv2eUBaNS4tincV+BmrFyJtZZRTaMPMVv8gw@mail.gmail.com>
Date:	Mon, 23 Sep 2013 12:47:23 -0700
From:	Pravin Shelar <pshelar@...ira.com>
To:	Simon Horman <horms@...ge.net.au>
Cc:	Jesse Gross <jesse@...ira.com>,
	"dev@...nvswitch.org" <dev@...nvswitch.org>,
	netdev <netdev@...r.kernel.org>, Ravi K <rkerur@...il.com>,
	Isaku Yamahata <yamahata@...inux.co.jp>,
	Joe Stringer <joe@...d.net.nz>
Subject: Re: [PATCH v2.39 7/7] datapath: Add basic MPLS support to kernel

On Sat, Sep 21, 2013 at 10:38 PM, Simon Horman <horms@...ge.net.au> wrote:
> On Thu, Sep 19, 2013 at 12:31:51PM -0500, Jesse Gross wrote:
>> On Wed, Sep 18, 2013 at 5:07 PM, Simon Horman <horms@...ge.net.au> wrote:
>> > On Tue, Sep 17, 2013 at 11:38:18AM -0700, Pravin Shelar wrote:
>> >> On Mon, Sep 9, 2013 at 12:20 AM, Simon Horman <horms@...ge.net.au> wrote:
>> >> > diff --git a/datapath/datapath.h b/datapath/datapath.h
>> >> > index 5d50dd4..babae3b 100644
>> >> > --- a/datapath/datapath.h
>> >> > +++ b/datapath/datapath.h
>> >> > @@ -36,6 +36,10 @@
>> >> >
>> >> >  #define SAMPLE_ACTION_DEPTH 3
>> >> >
>> >> > +#if LINUX_VERSION_CODE >= KERNEL_VERSION(3,11,0)
>> >> > +#define HAVE_INNER_PROTOCOL
>> >> > +#endif
>> >> > +
>> >> >  /**
>> >> >   * struct dp_stats_percpu - per-cpu packet processing statistics for a given
>> >> >   * datapath.
>> >> > @@ -93,11 +97,16 @@ struct datapath {
>> >> >   * @pkt_key: The flow information extracted from the packet.  Must be nonnull.
>> >> >   * @tun_key: Key for the tunnel that encapsulated this packet. NULL if the
>> >> >   * packet is not being tunneled.
>> >> > + * @inner_protocol: Provides a substitute for the skb->inner_protocol field on
>> >> > + * kernels before 3.11.
>> >> >   */
>> >> >  struct ovs_skb_cb {
>> >> >         struct sw_flow          *flow;
>> >> >         struct sw_flow_key      *pkt_key;
>> >> >         struct ovs_key_ipv4_tunnel  *tun_key;
>> >> > +#ifndef HAVE_INNER_PROTOCOL
>> >> > +       __be16                  inner_protocol;
>> >> > +#endif
>> >> >  };
>> >> >  #define OVS_CB(skb) ((struct ovs_skb_cb *)(skb)->cb)
>> >> >
>> >> Can you move this to compat struct ovs_gso_cb {}
>> >
>> > I think that you are correct and inner_protocol needs
>> > to be in struct ovs_gso_cb so that it can
>> > be accessed via skb_network_protocol() from
>> > rpl___skb_gso_segment().
>> >
>> > However I think it may also need to be present in struct ovs_cb
>> > so that it can be set correctly.
>> >
>> > Currently it is set unconditionally
>> > in ovs_execute_actions() and Jesse has suggested setting
>> > it conditionally in pop_mpls() (which is called by do_execute_actions()).
>> > But regardless it seems to me that the field would need to be available
>> > in struct ovs_cb.
>>
>> Since the helper functions are also in the compat code, I think they
>> should have access to ovs_gso_cb.
>
> Pravin also believes that is the case so I have moved inner_protocol
> to struct ovs_gso_cb as he requested.
>
>> >> I think we can simplify code by pushing vlan and then segmenting skb,
>> >> the way we do it for MPLS.
>> >
>> > Are you thinking of something like the following which applies
>> > prior to the MPLS code.
>>
>> This is basically what I was thinking about. We might actually be able
>> to move all of this to compat code by having a replacement for
>> dev_queue_xmit() similar to what we have for ip_local_out() in the
>> tunnel code.
>
> I would appreciate Pravin's opinion on this but it seems to me
> that it should be possible.
>
> The following applies on top of my previous proposed change
> in this thread - simplification of segmentation - and before
> the MPLS patch-set. Is this along the lines of what you were
> thinking of?
>
>
> From: Simon Horman <horms@...ge.net.au>
>
> datapath: Move segmentation compatibility code into a compatibility function
>
> *** Do not apply: for informational purposes only
>
> Move segmentation compatibility code out of netdev_send and into
> rpl_dev_queue_xmit(), a compatibility function used in place
> of dev_queue_xmit() as necessary.
>
> As suggested by Jesse Gross.
>
> Some minor though verbose implementation notes:
>
> * This rpl_dev_queue_xmit() endeavours to return a valid error code or
>   zero on success as per dev_queue_xmit(). The exception is that when
>   dev_queue_xmit() is called in a loop only the status of the last call is
>   taken into account, thus ignoring any errors returned by previous calls.
>   This is derived from the previous calls to dev_queue_xmit() in a loop
>   where netdev_send() ignores the return value of dev_queue_xmit()
>   entirely.
>
> * netdev_send() continues to ignore the value of dev_queue_xmit().
>   So the discussion of the return value of rpl_dev_queue_xmit()
>   above is has no bearing on run-time behaviour.
>
> * The return value of netdev_send() may differ from the previous
>   implementation in the case where segmentation is performed before
>   calling the real dev_queue_xmit(). This is because previously in
>   this case netdev_send() would return the combined length of the
>   skbs resulting from segmentation. Whereas the current code
>   always returns the length of the original skb.

This approach looks good to me.

>
> Compile tested only.
>
This patch does not work since vport-netdev does not include compat
gso header. after including gso.h it gives me compiler error.
Can you post combined patch with fixes?

It is better to move rpl_dev_queue_xmit definition from gso.h to
linux/netdevice.h.

Thanks.
--
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