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@xxxxxxxxx/ >> Signed-off-by: Abhishek Chauhan <quic_abchauha@xxxxxxxxxxx> > >> +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. > >
