Re: [RFC PATCH bpf-next v5 1/2] net: Rename mono_delivery_time to tstamp_type for scalabilty

Abhishek Chauhan (ABC)
2024-04-25T19:03:15Z

On 4/25/2024 7:31 AM, Willem de Bruijn wrote:
> Abhishek Chauhan wrote:
>> mono_delivery_time was added to check if skb->tstamp has delivery
>> time in mono clock base (i.e. EDT) otherwise skb->tstamp has
>> timestamp in ingress and delivery_time at egress.
>>
>> Renaming the bitfield from mono_delivery_time to tstamp_type is for
>> extensibilty for other timestamps such as userspace timestamp
>> (i.e. SO_TXTIME) set via sock opts.
>>
>> As we are renaming the mono_delivery_time to tstamp_type, it makes
>> sense to start assigning tstamp_type based on enum defined
>> in this commit.
>>
>> Earlier we used bool arg flag to check if the tstamp is mono in
>> function skb_set_delivery_time, Now the signature of the functions
>> accepts tstamp_type to distinguish between mono and real time.
>>
>> Introduce a new function to set tstamp_type based on clockid.
>>
>> In future tstamp_type:1 can be extended to support userspace timestamp
>> by increasing the bitfield.
>>
>> Link: https://lore.kernel.org/netdev/bc037db4-58bb-4861-ac31-a361a93841d3@linux.dev/
>> Signed-off-by: Abhishek Chauhan <quic_abchauha@quicinc.com>
>
>> +static inline void skb_set_tstamp_type_frm_clkid(struct sk_buff *skb,
>> + ktime_t kt, clockid_t clockid)
>> +{
>
> Please don't garble words to save a few characters: .._from_clockid.
>
Noted and apologies for using garble words here. I will correct it.
> And this is essentially skb_set_delivery_type, just taking another
> type. So skb_set_delivery_type_(by|from)_clockid.
>
> Also, instead of reimplementing the same logic with a different
> type, could implement as a conversion function that calls the main
> function. It won't save lines. But will avoid duplicate logic that
> needs to be kept in sync whenever there are future changes (fragile).
>

I thought about doing this but as you remember that some places in the network stack,
we are passing tstamp_type and some places we are passing clockid.

So in the previous patchset we decided with two variants.
1. One that assigns the tstamp_type directly
2. Other one which computes tstamp_type based on clockid

But i agree on the above comment that if we implement two different variants
then in future it requires maintenance to both functions.

I will make sure i handle both cases in one func.

> static inline void skb_set_delivery_type_by_clockid(struct sk_buff *skb,
> ktime_t kt, clockid_t clockid)
> {
> u8 tstamp_type = SKB_CLOCK_REAL;
>
> switch(clockid) {
> case CLOCK_REALTIME:
> break;
> case CLOCK_MONOTONIC:
> tstamp_type = SKB_CLOCK_MONO;
> break;
> default:
> WARN_ON_ONCE(1);
> kt = 0;
> };
>
> skb_set_delivery_type(skb, kt, tstamp_type);
> }
>
>
>> + skb->tstamp = kt;
>> +
>> + if (!kt) {
>> + skb->tstamp_type = SKB_CLOCK_REALTIME;
>> + return;
>> + }
>> +
>> + switch (clockid) {
>> + case CLOCK_REALTIME:
>> + skb->tstamp_type = SKB_CLOCK_REALTIME;
>> + break;
>> + case CLOCK_MONOTONIC:
>> + skb->tstamp_type = SKB_CLOCK_MONOTONIC;
>> + break;
>> + }
>> +}
>> +
>> static inline void skb_set_delivery_time(struct sk_buff *skb, ktime_t kt,
>> - bool mono)
>> + u8 tstamp_type)
>
> Indentation change: error?
>>> @@ -9444,7 +9444,7 @@ static struct bpf_insn *bpf_convert_tstamp_read(const struct bpf_prog *prog,
>> TC_AT_INGRESS_MASK | SKB_MONO_DELIVERY_TIME_MASK);
>> *insn++ = BPF_JMP32_IMM(BPF_JNE, tmp_reg,
>> TC_AT_INGRESS_MASK | SKB_MONO_DELIVERY_TIME_MASK, 2);
>> - /* skb->tc_at_ingress && skb->mono_delivery_time,
>> + /* skb->tc_at_ingress && skb->tstamp_type:1,
>
> Is the :1 a stale comment after we discussed how to handle the 2-bit
This is first patch which does not add tstamp_type:2 at the moment.
This series is divided into two patches
1. One patchset => Just rename (So the comment is still skb->tstamp_type:1)
2. Second patchset => add another bit (comment is changed to skb->tstamp_type:2)

> field going forward? I.e., not by ignoring the second bit.
>
>