On Fri, Apr 28, 2017 at 4:54 AM, Miroslav Lichvar <mlich...@redhat.com> wrote:
> On Wed, Apr 26, 2017 at 08:00:02PM -0400, Willem de Bruijn wrote:
>> > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>> > index 81ef53f..42bff22 100644
>> > --- a/include/linux/skbuff.h
>> > +++ b/include/linux/skbuff.h
>> > @@ -3300,8 +3300,7 @@ void skb_tstamp_tx(struct sk_buff *orig_skb,
>> >
>> > static inline void sw_tx_timestamp(struct sk_buff *skb)
>> > {
>> > - if (skb_shinfo(skb)->tx_flags & SKBTX_SW_TSTAMP &&
>> > - !(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS))
>> > + if (skb_shinfo(skb)->tx_flags & SKBTX_SW_TSTAMP)
>> > skb_tstamp_tx(skb, NULL);
>> > }
>
>> > +++ b/net/core/skbuff.c
>> > @@ -3874,6 +3874,10 @@ void __skb_tstamp_tx(struct sk_buff *orig_skb,
>> > if (!sk)
>> > return;
>> >
>> > + if (!hwtstamps && !(sk->sk_tsflags & SOF_TIMESTAMPING_OPT_TX_SWHW)
>> > &&
>> > + skb_shinfo(orig_skb)->tx_flags & SKBTX_IN_PROGRESS)
>> > + return;
>> > +
>>
>> This check should only happen for software transmit timestamps, so simpler to
>> revise the check in sw_tx_timestamp above to
>>
>> if (skb_shinfo(skb)->tx_flags & SKBTX_SW_TSTAMP &&
>> - !(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS))
>> + (!(skb_shinfo(orig_skb)->tx_flags & SKBTX_IN_PROGRESS)) ||
>> + (skb->sk && skb->sk->sk_tsflags & SOF_TIMESTAMPING_OPT_TX_SWHW)
>
> I'm not sure if this can work. sk_buff.h would need to include sock.h
> in order to get the definition of struct sock. Any suggestions?
A more elegant solution would be to not set SKBTX_IN_PROGRESS
at all if SOF_TIMESTAMPING_OPT_TX_SWHW is set on the socket.
But the patch to do so is not elegant, having to update callsites in many
device drivers.
Otherwise you may indeed have to call skb_tstamp_tx for every packet
that has SKBTX_SW_TSTAMP set, as you do. We can at least move
the skb->sk != NULL check into skb_tx_timestamp in skbuff.h.
By the way, if changing this code, I think that it's time to get rid of
sw_tx_timestamp. It is only called from skb_tx_timestamp. Let's
just move the condition in there.
