Re: [net v5 2/3] net: sched: add check tc_skip_classify in sch egress

On Sun, Dec 12, 2021 at 5:40 PM Tonghao Zhang [off-list ref] wrote:

On Sat, Dec 11, 2021 at 4:11 AM Daniel Borkmann [off-list ref] wrote:

quoted

On 12/10/21 8:54 PM, Tonghao Zhang wrote:

quoted

On Sat, Dec 11, 2021 at 1:46 AM Tonghao Zhang [off-list ref] wrote:

quoted

On Sat, Dec 11, 2021 at 1:37 AM Tonghao Zhang [off-list ref] wrote:

quoted

On Sat, Dec 11, 2021 at 12:43 AM John Fastabend
[off-list ref] wrote:

quoted

xiangxia.m.yue@ wrote:

quoted

From: Tonghao Zhang <redacted>

Try to resolve the issues as below:
* We look up and then check tc_skip_classify flag in net
   sched layer, even though skb don't want to be classified.
   That case may consume a lot of cpu cycles. This patch
   is useful when there are a lot of filters with different
   prio. There is ~5 prio in in production, ~1% improvement.

   Rules as below:
   $ for id in $(seq 1 5); do
   $       tc filter add ... egress prio $id ... action mirred egress redirect dev ifb0
   $ done

* bpf_redirect may be invoked in egress path. If we don't
   check the flags and then return immediately, the packets
   will loopback.

This would be the naive case right? Meaning the BPF program is
doing a redirect without any logic or is buggy?

Can you map out how this happens for me, I'm not fully sure I
understand the exact concern. Is it possible for BPF programs
that used to see packets no longer see the packet as expected?

Is this the path you are talking about?

Hi John
Tx ethx -> __dev_queue_xmit -> sch_handle_egress
->  execute BPF program on ethx with bpf_redirect(ifb0) ->
-> ifb_xmit -> ifb_ri_tasklet -> dev_queue_xmit -> __dev_queue_xmit
the packets loopbacks, that means bpf_redirect doesn't work with ifb
netdev, right ?
so in sch_handle_egress, I add the check skb_skip_tc_classify().

But why would you do that? Usage like this is just broken by design..
If you need to loop anything back to RX, just use bpf_redirect() with
BPF_F_INGRESS? What is the concrete/actual rationale for ifb here?

Hi
note that: ifb_ri_tasklet can send out the packets or receive skb
ifb_ri_tasklet
                if (!skb->from_ingress) {
                        dev_queue_xmit(skb); // bpf_redirect to ifb
and ifb invoked the dev_queue_xmit in our case.
                } else {
                        skb_pull_rcsum(skb, skb->mac_len);
                        netif_receive_skb(skb);
                }

Hi
In this thread, I try to explain this patch, and answer questions.
What should I do next? v1-v4 is "Changes Requested" in patchwork, but
v5 is "Rejected"
Should I add more commit message in this patch, and send v6 ?
1/3, 3/3 patch still need to be reviewed.

--
Best regards, Tonghao

-- 
Best regards, Tonghao