From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from sipsolutions.net (s3.sipsolutions.net [IPv6:2a01:4f8:191:4433::2]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by lists.bufferbloat.net (Postfix) with ESMTPS id A1EC63B29E for ; Mon, 10 Sep 2018 07:12:42 -0400 (EDT) Received: by sipsolutions.net with esmtpsa (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.91) (envelope-from ) id 1fzK7U-00046A-VS; Mon, 10 Sep 2018 13:12:41 +0200 Message-ID: <1536577952.3224.58.camel@sipsolutions.net> From: Johannes Berg To: Toke =?ISO-8859-1?Q?H=F8iland-J=F8rgensen?= , linux-wireless@vger.kernel.org, make-wifi-fast@lists.bufferbloat.net Cc: Rajkumar Manoharan , Felix Fietkau Date: Mon, 10 Sep 2018 13:12:32 +0200 In-Reply-To: <87k1ntlinf.fsf@toke.dk> References: <153635803319.14170.10011969968767927187.stgit@alrua-x1> <153635897010.14170.2992498632345986102.stgit@alrua-x1> <1536566666.3224.24.camel@sipsolutions.net> <87k1ntlinf.fsf@toke.dk> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.26.6 (3.26.6-1.fc27) Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Subject: Re: [Make-wifi-fast] [PATCH RFC v3 1/4] mac80211: Add TXQ scheduling API X-BeenThere: make-wifi-fast@lists.bufferbloat.net X-Mailman-Version: 2.1.20 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 10 Sep 2018 11:12:42 -0000 On Mon, 2018-09-10 at 13:02 +0200, Toke Høiland-Jørgensen wrote: > > > Is there a case where it's allowed to call this while *not* empty? At > > least for the drivers it seems not, so perhaps when called from the > > driver it should WARN() here or so? > > Yeah, mac80211 calls this on wake_txq, where it will often be scheduled > already. And since that can happen while drivers schedule stuff, it > could also be the case that a driver re-schedules a queue that is > already scheduled. Right, I kinda gathered that but was thinking more from a driver POV as I was reviewing, makes sense. > > I'm just a bit worried that drivers will get this a bit wrong, and > > we'll never know, but things won't work properly in some weird way. > > What, subtle bugs in the data path that causes hangs in hard-to-debug > cases? Nah, that never happens ;) Good :-P Actually though, we can't put a warn on there, because the driver might take a txq, then mac80211 puts it on the list due to a new packet, and then the driver also returns it. > > > + txqi = list_first_entry(&local->active_txqs[ac], > > > + struct txq_info, > > > + schedule_order); > > > + > > > + if (txqi->schedule_round == local->schedule_round[ac]) > > > + txqi = NULL; > > > > that assigment seems unnecessary? txqi defaults to NULL. > > No, this is an 'undo' of the previous line. I.e., we get the first txqi, > check if we've already seen it this round, and if we have, unset txqi so > the function returns NULL (causing the driver to stop looping). Err, yeah, obviously. johannes