Abhishek Chauhan (ABC) wrote: > > > On 4/10/2024 8:42 AM, Willem de Bruijn wrote: > > Abhishek Chauhan wrote: > >> tstamp_type can be real, mono or userspace timestamp. > >> > >> This commit adds userspace timestamp and sets it if there is > >> valid transmit_time available in socket coming from userspace. > >> > >> To make the design scalable for future needs this commit bring in > >> the change to extend the tstamp_type:1 to tstamp_type:2 to support > >> userspace timestamp. > >> > >> Link: https://lore.kernel.org/netdev/bc037db4-58bb-4861-ac31-a361a93841d3@xxxxxxxxx/ > >> Signed-off-by: Abhishek Chauhan <quic_abchauha@xxxxxxxxxxx> > > With pahole, does this have an effect on sk_buff layout? > > > I think it does and it also impacts BPF testing. Hence in my cover letter i have mentioned that these > changes will impact BPF. My level of expertise is very limited to BPF hence the reason for RFC. > That being said i am actually trying to understand/learn BPF instructions to know things better. > I think we need to also change the offset SKB_MONO_DELIVERY_TIME_MASK and TC_AT_INGRESS_MASK > > > #ifdef __BIG_ENDIAN_BITFIELD > #define SKB_MONO_DELIVERY_TIME_MASK (1 << 7) //Suspecting changes here too > #define TC_AT_INGRESS_MASK (1 << 6) // and here > #else > #define SKB_MONO_DELIVERY_TIME_MASK (1 << 0) > #define TC_AT_INGRESS_MASK (1 << 1) (this might have to change to 1<<2 ) > #endif > #define SKB_BF_MONO_TC_OFFSET offsetof(struct sk_buff, __mono_tc_offset) > > Also i suspect i change in /selftests/bpf/prog_tests/ctx_rewrite.c > I am trying to figure out what this part of the code is doing. > https://lore.kernel.org/all/20230321014115.997841-4-kuba@xxxxxxxxxx/ > > Please correct me if i am wrong here. I broadly agree. We should convert all references to SKB_MONO_DELIVERY_TIME_MASK to an skb_tstamp_type equivalent. > > >> @@ -4274,7 +4280,16 @@ static inline void skb_set_delivery_time(struct sk_buff *skb, ktime_t kt, > >> enum skb_tstamp_type tstamp_type) > >> { > >> skb->tstamp = kt; > >> - skb->tstamp_type = kt && tstamp_type; > >> + > >> + if (skb->tstamp_type) > >> + return; > >> + > > > I can put a warn on here incase if both MONO and TAI are set. > OR > Rather make it simple as you have mentioned below. When might skb->tstamp_type already be non-zero when skb_set_deliver_type is called? In most cases, this is called for a fresh skb. In br_ip6_fragment, it is called with a previous value. But this is the value of skb->tstamp_type. It just clears it if kt is 0. If skb->tstamp_type != tstamp_type is not a condition that can be forced by an unprivileged user, then we can warn.
