On Mon, Apr 24, 2017 at 5:00 AM, Miroslav Lichvar <mlich...@redhat.com> wrote:
> On Thu, Apr 13, 2017 at 12:16:09PM -0400, Willem de Bruijn wrote:
>> On Thu, Apr 13, 2017 at 11:18 AM, Miroslav Lichvar <mlich...@redhat.com>
>> wrote:
>> > On Thu, Apr 13, 2017 at 10:37:07AM -0400, Willem de Bruijn wrote:
>> >> Why is this L2 length needed?
>> >
>> > It's needed for incoming packets to allow converting of preamble
>> > timestamps to trailer timestamps.
>>
>> Receiving the mac length of a packet sounds like a feature independent
>> from timestamping.
>
> I agree, but so far nobody suggested another use for this information.
> Do you have any suggestions?
>
> The idea was that if it is useful only with HW timestamping, it would
> be better to save it only with the timestamp, so there is no
> performance impact in the more common case when HW timestamping is
> disabled. Am I overly cautious here?
The additional cost of a cmsg is zero for sockets that have no cmsg
enabled, due to
if (inet->cmsg_flags)
ip_cmsg_recv_offset(msg, sk, skb, sizeof(struct udphdr), off);
But you might be right that there are no uses outside the specific
timestamp requirement you have, so if you prefer to use a timestamp
option, I won't object further.
>> Either an ioctl similar to SIOCGIFMTU or, if it may
>> vary due to existince of vlan headers, a new independent cmsg at the
>> SOL_SOCKET layer.
The latter would require adding the SOL_SOCKET level cmsg processing
infra. It is simpler to just add it at the INET/INET6 levels.
> It's not just the VLAN headers. The length of the IP header may vary
> with IP options, so the offset of the UDP data in the packet cannot be
> assumed to be constant.
As well as tunnels.
> Now I'm wondering if it's actually necessary to save the original
> value of skb->mac_len + skb->len.
Computing it on recv if needed is definitely preferable to computing
on enqueue and storing in an intermediate variable.
> Would "skb->data - skb->head -
> skb->mac_header + skb->len" always work as the L2 length for received
> packets at the time when the cmsg is prepared?
(skb->data - skb->head) - skb->mac_header computes the length
of data before the mac, such as reserve? Do you mean skb->data -
skb->mac_header (or - skb_mac_offset(skb))?
> As for the original ifindex, it seems to me it does need to be saved
> to a new field since __netif_receive_skb_core() intentionally
> overwrites skb->skb_iif. What would be the best place for it, sk_buff
> or skb_shared_info?
Finding storage space on the receive path will not be easy.
One shortcut to avoid storing this information explicitly is to look up
the device from skb->napi_id.
> And would it really be acceptable to save it for all packets in
> __netif_receive_skb_core(), even when HW timestamping is disabled?
> Seeing how the code and the data structures were optimized over time,
> I have a feeling it would not be accepted.
Incurring this cost on all packets for such a rare edge case does sound
like a non-starter.
It can be called only if the netstamp_needed static key is enabled (false),
in __net_timestamp, though.
