On Wed, Jan 23, 2019 at 5:17 AM Maxim Mikityanskiy <maxi...@mellanox.com> wrote:
>
> > -----Original Message-----
> > From: Willem de Bruijn <willemdebruijn.ker...@gmail.com>
> > Sent: 17 January, 2019 17:16
> > To: Maxim Mikityanskiy <maxi...@mellanox.com>
> > Cc: David S. Miller <da...@davemloft.net>; Saeed Mahameed
> > <sae...@mellanox.com>; Willem de Bruijn <will...@google.com>; Jason Wang
> > <jasow...@redhat.com>; Eric Dumazet <eduma...@google.com>;
> > netdev@vger.kernel.org; Eran Ben Elisha <era...@mellanox.com>; Tariq Toukan
> > <tar...@mellanox.com>
> > Subject: Re: [PATCH 1/7] net: Don't set transport offset to invalid value
> >
> > On Thu, Jan 17, 2019 at 4:10 AM Maxim Mikityanskiy <maxi...@mellanox.com>
> > wrote:
> > >
> > > > This is a lot of code change. This would do.
> > > >
> > > > @@ -2434,8 +2434,6 @@ static inline void
> > > > skb_probe_transport_header(struct sk_buff *skb,
> > > >
> > > > if (skb_flow_dissect_flow_keys_basic(skb, &keys, NULL, 0, 0, 0,
> > 0))
> > > > skb_set_transport_header(skb, keys.control.thoff);
> > > > - else
> > > > - skb_set_transport_header(skb, offset_hint);
> > > > }
> > > >
> > > > Though leaving an unused argument is a bit ugly. For net-next, indeed
> > > > better to clean up (please mark your patchset with net or net-next,
> > > > btw)
> > >
> > > It's for net-next (I'll resend with the correct mark), so I'll stick
> > > with the current implementation.
> >
> > Absolutely, sounds good.
> >
> > > > > diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-
> > > > netback/netback.c
> > > > > index 80aae3a32c2a..b49b6e56ca47 100644
> > > > > --- a/drivers/net/xen-netback/netback.c
> > > > > +++ b/drivers/net/xen-netback/netback.c
> > > > > @@ -1105,6 +1105,7 @@ static int xenvif_tx_submit(struct xenvif_queue
> > > > *queue)
> > > > > struct xen_netif_tx_request *txp;
> > > > > u16 pending_idx;
> > > > > unsigned data_len;
> > > > > + bool th_set;
> > > > >
> > > > > pending_idx = XENVIF_TX_CB(skb)->pending_idx;
> > > > > txp = &queue->pending_tx_info[pending_idx].req;
> > > > > @@ -1169,20 +1170,22 @@ static int xenvif_tx_submit(struct
> > xenvif_queue
> > > > *queue)
> > > > > continue;
> > > > > }
> > > > >
> > > > > - skb_probe_transport_header(skb, 0);
> > > > > + th_set = skb_try_probe_transport_header(skb);
> > > >
> > > > Can use skb_transport_header_was_set(). Then at least there is no need
> > > > to change the function's return value.
> > >
> > > I suppose this comment relates to the previous one, and if we do it for
> > > net-next, it's fine to make change I made, isn't it?
> >
> > If this is the only reason for the boolean return value, using
> > skb_transport_header_was_set() is more standard (I immediately know
> > what's happening when I read it), slightly less code change and avoids
> > introducing a situation where the majority of callers ignore a return
> > value. I think it's preferable. But these merits are certainly
> > debatable, so either is fine.
>
> From my side, I wanted to avoid calling skb_transport_header_was_set
> twice, so I made skb_try_probe_transport_header return whether it
> succeeded or not. I think "try" in the function name indicates this idea
> pretty clearly. This result status is pretty useful, it just happened
> that it's not needed in many places,
Which is an indication that it's perhaps not needed.
> but the general idea is that we
> report this status, so if you say that my version is also good for you,
> I'll leave it as is. It was just a rationale for my decision.
It's fine. But please avoid the code churn in xenvif_tx_submit
with to extra indentation. This is equivalent:
- if (skb_is_gso(skb)) {
+ if (skb_is_gso(skb) && th_set) {
More fundamentally, the code has the assumption that th_set
always holds if skb_is_gso(skb). Why add the check? This is
another example that the return value is not really needed.
