On Thu, Apr 27, 2017 at 12:39 PM, Miroslav Lichvar <mlich...@redhat.com> wrote:
> On Thu, Apr 27, 2017 at 12:21:00PM -0400, Willem de Bruijn wrote:
>> >> > @@ -720,6 +720,7 @@ void __sock_recv_timestamp(struct msghdr *msg,
>> >> > struct sock *sk,
>> >> > empty = 0;
>> >> > if (shhwtstamps &&
>> >> > (sk->sk_tsflags & SOF_TIMESTAMPING_RAW_HARDWARE) &&
>> >> > + (empty || !skb_is_err_queue(skb)) &&
>> >> > ktime_to_timespec_cond(shhwtstamps->hwtstamp, tss.ts + 2)) {
>> >>
>> >> I find skb->tstamp == 0 easier to understand than the condition on empty.
>> >>
>> >> Indeed, this is so non-obvious that I would suggest another helper
>> >> function
>> >> skb_is_hwtx_tstamp with a concise comment about the race condition
>> >> between tx software and hardware timestamps (as in the last sentence of
>> >> the commit message).
>> >
>> > Should it include also the skb_is_err_queue() check? If it returned
>> > true for both TX and RX HW timestamps, maybe it could be called
>> > skb_has_hw_tstamp?
>>
>> For the purpose of documenting why this complex condition exists,
>> I would call the skb_is_err_queue in that helper function and make
>> it tx + hw specific.
>
> Hm, like this?
>
> if (shhwtstamps &&
> (sk->sk_tsflags & SOF_TIMESTAMPING_RAW_HARDWARE) &&
> + (skb_is_hwtx_tstamp(skb) || !skb_is_err_queue(skb)) &&
> ktime_to_timespec_cond(shhwtstamps->hwtstamp, tss.ts + 2)) {
>
> where skb_is_hwtx_tstamp() has
> return skb->tstamp == 0 && skb_is_err_queue(skb);
>
> I was just not sure about the unnecessary skb_is_err_queue() call.
Oh, good point. If the condition is
(skb_is_err_queue(skb) && !skb->tstamp) || !skb_is_err_queue(skb)
then it makes more sense to just use the simpler expression
(!skb_is_err_queue(skb)) || (!skb->tstamp)
This cannot be called skb_is_hwtx_tstamp, as this does not imply
skb_hwtstamps(skb). Perhaps instead define
/* On transmit, software and hardware timestamps are returned independently.
* Do not return hardware timestamps even if skb_hwtstamps(skb) is true if
* skb->tstamp is set
*/
static bool skb_is_swtx_tstamp(skb) {
return skb_is_err_queue(skb) && skb->tstamp;
}
and use !skb_is_swtx_tstamp(skb) in this condition. The comment is
just a quick first try, can probably be improved.
