From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id A8528AD50 for ; Thu, 11 May 2023 23:20:25 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id A5E9AC433EF; Thu, 11 May 2023 23:20:24 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1683847225; bh=heb/Ae+vtqZAI/LR/adpb91zjfMyByllNRGWtN+x6Us=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=FLaFHom8eejodz/0W9plf6HpJILhurJIeBRhNxSwBfBXInmfaBQucQfvWVEJfAw30 1znJdGNjJD8USba4FGByWD5jWXaiGeBeFm2Q4P5fszb09ojRbX5wKmgitKlDlHZRmu bV7t6gjofHMYRChisOWaPXZeNTwAlAfRZgHXhGgxAFt1xAhzIj6KDJe1oazhmCcN05 nZ86GrdBnn7U8aWfvxR7ovUP7rxigDBkNFzXjMzqHGJwC/dAi4zlu6F/D/ca0Kyl+S 9O4ekKRL207Hj0mzYBbV6WCTtD0YBalKBwxkG1nJ5/u8SQqfhYqVuI7QvEeq1rudsm KdXgP24DPqbEQ== Date: Thu, 11 May 2023 16:20:23 -0700 From: Jakub Kicinski To: Peilin Ye , Jiri Pirko , Daniel Borkmann Cc: "David S. Miller" , Eric Dumazet , Paolo Abeni , Jamal Hadi Salim , Cong Wang , Peilin Ye , John Fastabend , Vlad Buslov , Pedro Tammela , Hillf Danton , netdev@vger.kernel.org, linux-kernel@vger.kernel.org, Cong Wang Subject: Re: [PATCH net 6/6] net/sched: qdisc_destroy() old ingress and clsact Qdiscs before grafting Message-ID: <20230511162023.3651970b@kernel.org> In-Reply-To: References: <20230508183324.020f3ec7@kernel.org> <20230510161559.2767b27a@kernel.org> Precedence: bulk X-Mailing-List: netdev@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit On Thu, 11 May 2023 13:40:10 -0700 Peilin Ye wrote: > On Wed, May 10, 2023 at 04:15:59PM -0700, Jakub Kicinski wrote: > > 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: > > On Wed, 10 May 2023 13:11:19 -0700 Peilin Ye wrote: > > > 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?