Re: [PATCH net 6/6] net/sched: qdisc_destroy() old ingress and clsact Qdiscs before grafting
From: Jakub Kicinski <kuba@kernel.org>
Date: 2023-05-11 23:20:35
Also in:
lkml
On Thu, 11 May 2023 13:40:10 -0700 Peilin Ye wrote:
quoted hunk ↗ jump to hunk
On Wed, May 10, 2023 at 04:15:59PM -0700, Jakub Kicinski wrote:quoted
My thinking was to make sure that dev->miniq_* pointers always point to one of the miniqs of the currently attached qdisc. Right now, on a quick look, those pointers are not initialized during initial graft, only when first filter is added, and may be cleared when filters are removed. But I don't think that's strictly required, miniq with no filters should be fine.Ah, I see, thanks for explaining, I didn't think of that. Looking at sch_handle_ingress() BTW, currently mini Qdisc stats aren't being updated (mini_qdisc_bstats_cpu_update()) if there's no filters, is this intended? Should I keep this behavior by:diff --git a/net/core/dev.c b/net/core/dev.c index 735096d42c1d..9016481377e0 100644 --- a/net/core/dev.c +++ b/net/core/dev.c@@ -5116,7 +5116,7 @@ sch_handle_ingress(struct sk_buff *skb, struct packet_type **pt_prev, int *ret, * that are not configured with an ingress qdisc will bail * out here. */ - if (!miniq) + if (!miniq || !miniq->filter_list) return skb; if (*pt_prev) {
Good question, maybe Jiri or Daniel can answer?
On Wed, May 10, 2023 at 04:15:59PM -0700, Jakub Kicinski wrote:quoted
On Wed, 10 May 2023 13:11:19 -0700 Peilin Ye wrote:quoted
Looking at the code, I think there is no guarantee that (1st) cannot happen after (2nd), although unlikely? Can RTNL-lockless RTM_NEWTFILTER handlers get preempted?Right, we need qdisc_graft(B) to update the appropriate dev pointer to point to b1. With that the ordering should not matter. Probably using the ->attach() callback?->attach() is later than dev_graft_qdisc(). We should get ready for concurrent filter requests (i.e. have dev pointer pointing to b1) before grafting (publishing) B.
I thought even for "unlocked" filter operations the start of it is under the lock, but the lock gets dropped after qdisc/block are found. I could be misremembering, I haven't looked at the code.
Also currently qdisc_graft() doesn't call ->attach() for ingress and clsact Qdiscs. But I see your point, thanks for the suggestion! I'll try ->init() and create v2.
->init() may be too early, aren't there any error points which could prevent the Qdisc from binding after ->init() was called?