On Fri, Sep 2, 2016 at 12:25 AM, Steffen Klassert <steffen.klass...@secunet.com> wrote: > On Wed, Aug 31, 2016 at 10:25:59AM -0700, Alexander Duyck wrote: >> On Wed, Aug 31, 2016 at 3:41 AM, Steffen Klassert >> <steffen.klass...@secunet.com> wrote: >> > >> > - /* GSO partial only requires that we trim off any excess that >> > - * doesn't fit into an MSS sized block, so take care of that >> > - * now. >> > - */ >> > - if (sg && csum && (features & NETIF_F_GSO_PARTIAL)) { >> > - partial_segs = len / mss; >> > - if (partial_segs > 1) >> > - mss *= partial_segs; >> > - else >> > - partial_segs = 0; >> > + if (sg && csum) { >> >> Actually you could also put the && (mss != GSO_BY_FRAGS) test up here >> as well. The GSO_PARTIAL code doesn't make any sense if we are >> segmenting by frags. >> > > Ok, we can do that. > >> > + /* GSO partial only requires that we trim off any excess >> > that >> > + * doesn't fit into an MSS sized block, so take care of >> > that >> > + * now. >> > + */ >> > + if ((features & NETIF_F_GSO_PARTIAL)) { >> > + partial_segs = len / mss; >> > + if (partial_segs > 1) >> > + mss *= partial_segs; >> > + else >> > + partial_segs = 0; >> > + } else if (list_skb && (mss != GSO_BY_FRAGS) && >> > + net_gso_ok(features, >> > skb_shinfo(head_skb)->gso_type)) { >> > + struct sk_buff *iter; >> >> Actually I think we have the ordering backwards here by a little bit. >> What we should probably be doing is your test for list_skb and such >> first and then checking for GSO_PARTIAL. If I am not mistaken we >> could end up with a situation where we have buffers with a frag_list >> that need to be partially segmented. > > Hm, when I look at __skb_gso_segment(), you remove NETIF_F_GSO_PARTIAL > from features if skb_gso_ok() returns false. So you do GSO partial only > if this buffer has no fraglist or if NETIF_F_FRAGLIST is set. This > means that we either don't need or don't want to do the fraglist > splitting in the GSO partial case.
Well I think all it would take to allow it though would be to replace skb_gso_ok with net_gso_ok, but now that I am aware that we are taking steps to prevent both firing a the same time we can hold off on that for now. >> Also I think we can merge >> partial_segs and fraglist_segs into one thing. > > Given that my assumption above is true, we could check > !(features & NETIF_F_GSO_PARTIAL). Then I do my stuff > if needed and then we do what's needed in both cases. Right. That is kind of what I was thinking. I think I call out something like that in some example code below. >> >> > + >> > + /* Split the buffer at the frag_list pointer. This >> > is >> > + * based on the assumption that all buffers in the >> > chain >> > + * excluding the last containing the same amount >> > of data. >> > + */ >> > + skb_walk_frags(head_skb, iter) { >> > + if (skb_headlen(iter)) >> > + goto normal; >> > + >> > + len -= iter->len; >> > + } >> >> What you could probably do here is actually have a goto that jumps >> into the code for NETIF_F_GSO_PARTIAL. If we do that then these two >> paths are mostly merged. >> >> > + fraglist_segs = len / mss; >> > + if (fraglist_segs > 1) >> > + mss *= fraglist_segs; >> > + else >> > + fraglist_segs = 0; >> > + } >> > } >> > >> > +normal: >> > headroom = skb_headroom(head_skb); >> > pos = skb_headlen(head_skb); >> > >> >> So one bit I can think of that might be an issue if we have to do both >> this and GSO_PARTIAL is that we might end up with a last chunk that >> has to be segmented twice, once to get rid of frag_list and again to >> trim the end piece off so that we are are evenly MSS aligned. > > As said above, I currently don't think that this can happen. You are correct. I didn't realize I was using skb_gso_ok instead of net_gso_ok. So we can fork some of this off into a separate patch for later. I can probably look into it when I have time. >> >> > @@ -3298,6 +3321,19 @@ perform_csum_check: >> > SKB_GSO_CB(segs)->data_offset = skb_headroom(segs) + >> > doffset; >> > } >> > >> > + if (fraglist_segs) { >> > + struct sk_buff *iter; >> > + >> > + for (iter = segs; iter; iter = iter->next) { >> > + skb_shinfo(iter)->gso_size = >> > skb_shinfo(head_skb)->gso_size; >> > + >> > + if (iter->next) >> > + skb_shinfo(iter)->gso_segs = fraglist_segs; >> > + else >> > + skb_shinfo(iter)->gso_segs = iter->len / >> > skb_shinfo(head_skb)->gso_size; >> >> It might be faster to just set gso_segs to fraglist_segs in every >> iteration of the loop, and then outside of the loop you could update >> the tail skb. It will save the effort of a test and jump which should >> be a few cycles per iteration. Also you will probably need to fixup >> gso_size for the tail segment. If the length less than or equal to >> gso_size then you need to drop gso_size to 0, otherwise you update >> gso_segs. Also I am pretty sure you will want to use a DIV_ROUND_UP >> logic for the segments since you want to reflect the actual packet >> count. >> >> Also this is still doing the same thing as I do at the end of the loop >> in the conditional section for partial segs. What we may want to do >> is just tweak the bits I have in that block with a check that is >> something along the lines of: >> /* Update type to add partial and then remove dodgy if set */ >> type |= (features & NETIF_F_GSO_PARTIAL) / >> NETIF_F_GSO_PARTIAL * SKB_GSO_PARTIAL; >> type &= ~SKB_GSO_DODGY; >> >> That bit of code is just a fancy way of setting the SKB_GSO_PARTIAL >> bit only if the NETIF_F_GSO_PARTIAL feature bit is set. Since we can >> avoid conditional jumps that way it usually runs faster since it is >> just an AND, SHIFT, and OR and I don't have any CPU flags to worry >> about. Once you have type recorded you could just set the 4 values I >> do in your loop and then fixup the last one. > > OK, sounds reasonable. >