On Sat, Sep 21, 2013 at 10:38 PM, Simon Horman [off-list ref] wrote:

On Thu, Sep 19, 2013 at 12:31:51PM -0500, Jesse Gross wrote:

quoted

On Wed, Sep 18, 2013 at 5:07 PM, Simon Horman [off-list ref] wrote:

quoted

On Tue, Sep 17, 2013 at 11:38:18AM -0700, Pravin Shelar wrote:

quoted

On Mon, Sep 9, 2013 at 12:20 AM, Simon Horman [off-list ref] wrote:

quoted

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.

quoted

quoted

quoted

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@verge.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.