On Fri, Jan 21, 2022 at 10:50:20AM -0800, sdf@xxxxxxxxxx wrote:
> On 01/20, Martin KaFai Lau wrote:
> > __sk_buff->mono_delivery_time:
> > This patch adds __sk_buff->mono_delivery_time to
> > read and write the mono delivery_time in skb->tstamp.
>
> > The bpf rewrite is like:
> > /* BPF_READ: __u64 a = __sk_buff->mono_delivery_time; */
> > if (skb->mono_delivery_time)
> > a = skb->tstamp;
> > else
> > a = 0;
>
> > /* BPF_WRITE: __sk_buff->mono_delivery_time = a; */
> > skb->tstamp = a;
> > skb->mono_delivery_time = !!a;
>
> > __sk_buff->tstamp:
> > The bpf rewrite is like:
> > /* BPF_READ: __u64 a = __sk_buff->tstamp; */
> > if (skb->tc_at_ingress && skb->mono_delivery_time)
> > a = 0;
> > else
> > a = skb->tstamp;
>
> > /* BPF_WRITE: __sk_buff->tstamp = a; */
> > skb->tstamp = a;
> > if (skb->tc_at_ingress || !a)
> > skb->mono_delivery_time = 0;
>
> > At egress, reading is the same as before. All skb->tstamp
> > is the delivery_time. Writing will not change the (kernel)
> > skb->mono_delivery_time also unless 0 is being written. This
> > will be the same behavior as before.
>
> > (#) At ingress, the current bpf prog can only expect the
> > (rcv) timestamp. Thus, both reading and writing are now treated as
> > operating on the (rcv) timestamp for the existing bpf prog.
>
> > During bpf load time, the verifier will learn if the
> > bpf prog has accessed the new __sk_buff->mono_delivery_time.
>
> > When reading at ingress, if the bpf prog does not access the
> > new __sk_buff->mono_delivery_time, it will be treated as the
> > existing behavior as mentioned in (#) above. If the (kernel) skb->tstamp
> > currently has a delivery_time, it will temporary be saved first and then
> > set the skb->tstamp to either the ktime_get_real() or zero. After
> > the bpf prog finished running, if the bpf prog did not change
> > the skb->tstamp, the saved delivery_time will be restored
> > back to the skb->tstamp.
>
> > When writing __sk_buff->tstamp at ingress, the
> > skb->mono_delivery_time will be cleared because of
> > the (#) mentioned above.
>
> > If the bpf prog does access the new __sk_buff->mono_delivery_time
> > at ingress, it indicates that the bpf prog is aware of this new
> > kernel support:
> > the (kernel) skb->tstamp can have the delivery_time or the
> > (rcv) timestamp at ingress. If the __sk_buff->mono_delivery_time
> > is available, the __sk_buff->tstamp will not be available and
> > it will be zero.
>
> > The bpf rewrite needs to access the skb's mono_delivery_time
> > and tc_at_ingress bit. They are moved up in sk_buff so
> > that bpf rewrite can be done at a fixed offset. tc_skip_classify
> > is moved together with tc_at_ingress. To get one bit for
> > mono_delivery_time, csum_not_inet is moved down and this bit
> > is currently used by sctp.
>
> > Signed-off-by: Martin KaFai Lau <kafai@xxxxxx>
> > ---
> > include/linux/filter.h | 31 +++++++-
> > include/linux/skbuff.h | 20 +++--
> > include/uapi/linux/bpf.h | 1 +
> > net/core/filter.c | 134 ++++++++++++++++++++++++++++++---
> > net/sched/act_bpf.c | 5 +-
> > net/sched/cls_bpf.c | 6 +-
> > tools/include/uapi/linux/bpf.h | 1 +
> > 7 files changed, 171 insertions(+), 27 deletions(-)
>
> > diff --git a/include/linux/filter.h b/include/linux/filter.h
> > index 71fa57b88bfc..5cef695d6575 100644
> > --- a/include/linux/filter.h
> > +++ b/include/linux/filter.h
> > @@ -572,7 +572,8 @@ struct bpf_prog {
> > has_callchain_buf:1, /* callchain buffer allocated? */
> > enforce_expected_attach_type:1, /* Enforce expected_attach_type
> > checking at attach time */
> > call_get_stack:1, /* Do we call bpf_get_stack() or
> > bpf_get_stackid() */
> > - call_get_func_ip:1; /* Do we call get_func_ip() */
> > + call_get_func_ip:1, /* Do we call get_func_ip() */
> > + delivery_time_access:1; /* Accessed __sk_buff->mono_delivery_time */
> > enum bpf_prog_type type; /* Type of BPF program */
> > enum bpf_attach_type expected_attach_type; /* For some prog types */
> > u32 len; /* Number of filter blocks */
> > @@ -699,6 +700,34 @@ static inline void bpf_compute_data_pointers(struct
> > sk_buff *skb)
> > cb->data_end = skb->data + skb_headlen(skb);
> > }
>
> > +static __always_inline u32 bpf_prog_run_at_ingress(const struct
> > bpf_prog *prog,
> > + struct sk_buff *skb)
> > +{
> > + ktime_t tstamp, delivery_time = 0;
> > + int filter_res;
> > +
> > + if (unlikely(skb->mono_delivery_time) && !prog->delivery_time_access) {
> > + delivery_time = skb->tstamp;
> > + skb->mono_delivery_time = 0;
> > + if (static_branch_unlikely(&netstamp_needed_key))
> > + skb->tstamp = tstamp = ktime_get_real();
> > + else
> > + skb->tstamp = tstamp = 0;
> > + }
> > +
> > + /* It is safe to push/pull even if skb_shared() */
> > + __skb_push(skb, skb->mac_len);
> > + bpf_compute_data_pointers(skb);
> > + filter_res = bpf_prog_run(prog, skb);
> > + __skb_pull(skb, skb->mac_len);
> > +
> > + /* __sk_buff->tstamp was not changed, restore the delivery_time */
> > + if (unlikely(delivery_time) && skb_tstamp(skb) == tstamp)
> > + skb_set_delivery_time(skb, delivery_time, true);
> > +
> > + return filter_res;
> > +}
> > +
> > /* Similar to bpf_compute_data_pointers(), except that save orginal
> > * data in cb->data and cb->meta_data for restore.
> > */
> > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> > index 4677bb6c7279..a14b04b86c13 100644
> > --- a/include/linux/skbuff.h
> > +++ b/include/linux/skbuff.h
> > @@ -866,22 +866,23 @@ struct sk_buff {
> > __u8 vlan_present:1; /* See PKT_VLAN_PRESENT_BIT */
> > __u8 csum_complete_sw:1;
> > __u8 csum_level:2;
> > - __u8 csum_not_inet:1;
> > __u8 dst_pending_confirm:1;
> > + __u8 mono_delivery_time:1;
> > +
> > +#ifdef CONFIG_NET_CLS_ACT
> > + __u8 tc_skip_classify:1;
> > + __u8 tc_at_ingress:1;
> > +#endif
> > #ifdef CONFIG_IPV6_NDISC_NODETYPE
> > __u8 ndisc_nodetype:2;
> > #endif
> > -
> > + __u8 csum_not_inet:1;
> > __u8 ipvs_property:1;
> > __u8 inner_protocol_type:1;
> > __u8 remcsum_offload:1;
> > #ifdef CONFIG_NET_SWITCHDEV
> > __u8 offload_fwd_mark:1;
> > __u8 offload_l3_fwd_mark:1;
> > -#endif
> > -#ifdef CONFIG_NET_CLS_ACT
> > - __u8 tc_skip_classify:1;
> > - __u8 tc_at_ingress:1;
> > #endif
> > __u8 redirected:1;
> > #ifdef CONFIG_NET_REDIRECT
> > @@ -894,7 +895,6 @@ struct sk_buff {
> > __u8 decrypted:1;
> > #endif
> > __u8 slow_gro:1;
> > - __u8 mono_delivery_time:1;
>
> > #ifdef CONFIG_NET_SCHED
> > __u16 tc_index; /* traffic control index */
> > @@ -972,10 +972,16 @@ struct sk_buff {
> > /* if you move pkt_vlan_present around you also must adapt these
> > constants */
> > #ifdef __BIG_ENDIAN_BITFIELD
> > #define PKT_VLAN_PRESENT_BIT 7
> > +#define TC_AT_INGRESS_SHIFT 0
> > +#define SKB_MONO_DELIVERY_TIME_SHIFT 2
> > #else
> > #define PKT_VLAN_PRESENT_BIT 0
> > +#define TC_AT_INGRESS_SHIFT 7
> > +#define SKB_MONO_DELIVERY_TIME_SHIFT 5
> > #endif
> > #define PKT_VLAN_PRESENT_OFFSET offsetof(struct sk_buff,
> > __pkt_vlan_present_offset)
> > +#define TC_AT_INGRESS_OFFSET offsetof(struct sk_buff,
> > __pkt_vlan_present_offset)
> > +#define SKB_MONO_DELIVERY_TIME_OFFSET offsetof(struct sk_buff,
> > __pkt_vlan_present_offset)
>
> > #ifdef __KERNEL__
> > /*
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index b0383d371b9a..83725c891f3c 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -5437,6 +5437,7 @@ struct __sk_buff {
> > __u32 gso_size;
> > __u32 :32; /* Padding, future use. */
> > __u64 hwtstamp;
> > + __u64 mono_delivery_time;
> > };
>
> > struct bpf_tunnel_key {
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index 4fc53d645a01..db17812f0f8c 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -7832,6 +7832,7 @@ static bool bpf_skb_is_valid_access(int off, int
> > size, enum bpf_access_type type
> > return false;
> > break;
> > case bpf_ctx_range(struct __sk_buff, tstamp):
> > + case bpf_ctx_range(struct __sk_buff, mono_delivery_time):
> > if (size != sizeof(__u64))
> > return false;
> > break;
> > @@ -7872,6 +7873,7 @@ static bool sk_filter_is_valid_access(int off, int
> > size,
> > case bpf_ctx_range(struct __sk_buff, tstamp):
> > case bpf_ctx_range(struct __sk_buff, wire_len):
> > case bpf_ctx_range(struct __sk_buff, hwtstamp):
> > + case bpf_ctx_range(struct __sk_buff, mono_delivery_time):
> > return false;
> > }
>
> > @@ -7911,6 +7913,7 @@ static bool cg_skb_is_valid_access(int off, int
> > size,
> > case bpf_ctx_range_till(struct __sk_buff, cb[0], cb[4]):
> > break;
> > case bpf_ctx_range(struct __sk_buff, tstamp):
> > + case bpf_ctx_range(struct __sk_buff, mono_delivery_time):
> > if (!bpf_capable())
> > return false;
> > break;
> > @@ -7943,6 +7946,7 @@ static bool lwt_is_valid_access(int off, int size,
> > case bpf_ctx_range(struct __sk_buff, tstamp):
> > case bpf_ctx_range(struct __sk_buff, wire_len):
> > case bpf_ctx_range(struct __sk_buff, hwtstamp):
> > + case bpf_ctx_range(struct __sk_buff, mono_delivery_time):
> > return false;
> > }
>
> > @@ -8169,6 +8173,7 @@ static bool tc_cls_act_is_valid_access(int off,
> > int size,
> > case bpf_ctx_range_till(struct __sk_buff, cb[0], cb[4]):
> > case bpf_ctx_range(struct __sk_buff, tstamp):
> > case bpf_ctx_range(struct __sk_buff, queue_mapping):
> > + case bpf_ctx_range(struct __sk_buff, mono_delivery_time):
> > break;
> > default:
> > return false;
> > @@ -8445,6 +8450,7 @@ static bool sk_skb_is_valid_access(int off, int
> > size,
> > case bpf_ctx_range(struct __sk_buff, tstamp):
> > case bpf_ctx_range(struct __sk_buff, wire_len):
> > case bpf_ctx_range(struct __sk_buff, hwtstamp):
> > + case bpf_ctx_range(struct __sk_buff, mono_delivery_time):
> > return false;
> > }
>
> > @@ -8603,6 +8609,114 @@ static struct bpf_insn
> > *bpf_convert_shinfo_access(const struct bpf_insn *si,
> > return insn;
> > }
>
>
> [..]
>
> > +static struct bpf_insn *bpf_convert_tstamp_read(const struct bpf_insn
> > *si,
> > + struct bpf_insn *insn)
> > +{
> > + __u8 value_reg = si->dst_reg;
> > + __u8 skb_reg = si->src_reg;
> > + __u8 tmp_reg = BPF_REG_AX;
> > +
> > +#ifdef CONFIG_NET_CLS_ACT
> > + *insn++ = BPF_LDX_MEM(BPF_B, tmp_reg, skb_reg, TC_AT_INGRESS_OFFSET);
> > + *insn++ = BPF_ALU32_IMM(BPF_AND, tmp_reg, 1 << TC_AT_INGRESS_SHIFT);
> > + *insn++ = BPF_JMP32_IMM(BPF_JEQ, tmp_reg, 0, 5);
> > + /* @ingress, read __sk_buff->tstamp as the (rcv) timestamp,
> > + * so check the skb->mono_delivery_time.
> > + */
> > + *insn++ = BPF_LDX_MEM(BPF_B, tmp_reg, skb_reg,
> > + SKB_MONO_DELIVERY_TIME_OFFSET);
> > + *insn++ = BPF_ALU32_IMM(BPF_AND, tmp_reg,
> > + 1 << SKB_MONO_DELIVERY_TIME_SHIFT);
> > + *insn++ = BPF_JMP32_IMM(BPF_JEQ, tmp_reg, 0, 2);
> > + /* skb->mono_delivery_time is set, read 0 as the (rcv) timestamp. */
> > + *insn++ = BPF_MOV64_IMM(value_reg, 0);
> > + *insn++ = BPF_JMP_A(1);
> > +#endif
> > +
> > + *insn++ = BPF_LDX_MEM(BPF_DW, value_reg, skb_reg,
> > + offsetof(struct sk_buff, tstamp));
> > + return insn;
> > +}
> > +
> > +static struct bpf_insn *bpf_convert_tstamp_write(const struct bpf_insn
> > *si,
> > + struct bpf_insn *insn)
> > +{
> > + __u8 value_reg = si->src_reg;
> > + __u8 skb_reg = si->dst_reg;
> > + __u8 tmp_reg = BPF_REG_AX;
> > +
> > + /* skb->tstamp = tstamp */
> > + *insn++ = BPF_STX_MEM(BPF_DW, skb_reg, value_reg,
> > + offsetof(struct sk_buff, tstamp));
> > +
> > +#ifdef CONFIG_NET_CLS_ACT
> > + *insn++ = BPF_LDX_MEM(BPF_B, tmp_reg, skb_reg, TC_AT_INGRESS_OFFSET);
> > + *insn++ = BPF_ALU32_IMM(BPF_AND, tmp_reg, 1 << TC_AT_INGRESS_SHIFT);
> > + *insn++ = BPF_JMP32_IMM(BPF_JNE, tmp_reg, 0, 1);
> > +#endif
> > +
> > + /* test tstamp != 0 */
> > + *insn++ = BPF_JMP_IMM(BPF_JNE, value_reg, 0, 3);
> > + /* writing __sk_buff->tstamp at ingress or writing 0,
> > + * clear the skb->mono_delivery_time.
> > + */
> > + *insn++ = BPF_LDX_MEM(BPF_B, tmp_reg, skb_reg,
> > + SKB_MONO_DELIVERY_TIME_OFFSET);
> > + *insn++ = BPF_ALU32_IMM(BPF_AND, tmp_reg,
> > + ~(1 << SKB_MONO_DELIVERY_TIME_SHIFT));
> > + *insn++ = BPF_STX_MEM(BPF_B, skb_reg, tmp_reg,
> > + SKB_MONO_DELIVERY_TIME_OFFSET);
> > +
> > + return insn;
> > +}
>
> I wonder if we'll see the regression from this. We read/write tstamp
> frequently and I'm not sure we care about the forwarding case.
>
> As a future work/follow up, do you think we can support cases like
> bpf_prog_load(prog_type=SCHED_CLS expected_attach_type=TC_EGRESS) where
> we can generate bytecode with only BPF_LDX_MEM/BPF_STX_MEM for skb->tstamp?
> (essentially a bytecode as it was prior to your patch series)
>
> Since we know that that specific program will run only at egress,
> I'm assuming we can generate simpler bytecode? (of coarse it needs more
> work on cls_bpf to enforce this new expected_attach_type constraint)
The common (if not the only useful?) use case for reading/writing
skb->tstamp should be at egress now. For this case, the patch added
test on skb->tc_at_ingress and test the writing value is non-zero. The
skb->mono_delivery_time bit should not be touched in this common
case at egress. Even with expected_attach_type=TC_EGRESS, it could save
testing the tc_at_ingress (3 bpf insns) but it still needs to test the
writing value is non-zero (1 bpf insn). Regardless, I doubt there
is any meaningful difference for these two new tests considering other
things that a typical bpf prog is doing (e.g. parsing header,
lookup map...) and also other logic in the stack's egress path.
For adding expected_attach_type=TC_EGRESS in the future for perf reason
alone... hmm... I suspect it will make it harder/confuse to use but yeah if
there is a convincing difference to justify that.
Unrelated to the perf topic but related to adding expected_attach_type,
I had considered adding an expected_attach_type but not for the
perf reason.
The consideration was to use expected_attach_type to distinguish the
__sk_buff->tstamp behavior on ingress, although I have a hard time
thinking of a reasonable use case on accessing __sk_buff->tstamp at ingress
other than printing the (rcv) timestamp out (which could also be 0).
However, I guess we have to assume we cannot break the ingress
behavior now. The dance in bpf_prog_run_at_ingress() in this patch
is to keep the (rcv) timestamp behavior for the existing tc-bpf@ingress.
My initial thought was to add expected_attach_type=TC_DELIVERY_TIME
to signal the bpf prog is expecting skb->tstamp could have the delviery_time at
ingress but then later I think learning this in prog->delivery_time_access
during verification should be as good, so dismissed the expected_attach_type
idea and save the user from remembering when to use this
new expected_attach_type.
Thanks for the review !
