Re: [RFC PATCH 7/9] GSO: Support partial segmentation offload

On Sun, Mar 27, 2016 at 10:36 PM, Jesse Gross [off-list ref] wrote:

On Fri, Mar 18, 2016 at 4:25 PM, Alexander Duyck [off-list ref] wrote:

quoted

diff --git a/net/core/dev.c b/net/core/dev.c
index edb7179bc051..666cf427898b 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c

@@ -2711,6 +2711,19 @@ struct sk_buff *__skb_gso_segment(struct sk_buff *skb,

[...]

quoted

+       /* Only report GSO partial support if it will enable us to
+        * support segmentation on this frame without needing additional
+        * work.
+        */
+       if (features & NETIF_F_GSO_PARTIAL) {
+               netdev_features_t partial_features;
+               struct net_device *dev = skb->dev;
+
+               partial_features = dev->features & dev->gso_partial_features;
+               if (!skb_gso_ok(skb, features | partial_features))
+                       features &= ~NETIF_F_GSO_PARTIAL;

I think we need to add NETIF_F_GSO_ROBUST into the skb_gso_ok() check
- otherwise packets coming from VMs fail this test and we lose GSO
partial. It's totally safe to expose this feature, since we'll compute
gso_segs anyways.

Good point.  I will update that before submitting for net-next.

quoted

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index f044f970f1a6..bdcba77e164c 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c

@@ -3281,6 +3291,15 @@ perform_csum_check:
         */
        segs->prev = tail;

+       /* Update GSO info on first skb in partial sequence. */
+       if (partial_segs) {
+               skb_shinfo(segs)->gso_size = mss / partial_segs;

One small thing: this gso_size is the same as the original MSS, right?
It seems like we could trivially stick it in a local variable and
avoid the extra division.

Nice catch.  I was a bit too in the mindset of having to use the same
variable throughout.

quoted

+               skb_shinfo(segs)->gso_segs = partial_segs;
+               skb_shinfo(segs)->gso_type = skb_shinfo(head_skb)->gso_type |
+                                            SKB_GSO_PARTIAL;

Since we're computing the gso_segs ourselves, it might be nice to
strip out SKB_GSO_DODGY when we set the type.

I will have that fixed for the version I submit for net-next.

I just wanted to say that this is really nice work - I was expecting
it to turn out to be really messy and unmaintainable but this is very
clean. Thanks!

Thanks.

- Alex