From owner-freebsd-wireless@FreeBSD.ORG Mon Feb 25 07:25:54 2013 Return-Path: Delivered-To: freebsd-wireless@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by hub.freebsd.org (Postfix) with ESMTP id 0403951B for ; Mon, 25 Feb 2013 07:25:54 +0000 (UTC) (envelope-from moonlightakkiy@yahoo.ca) Received: from nm25-vm3.bullet.mail.ne1.yahoo.com (nm25-vm3.bullet.mail.ne1.yahoo.com [98.138.91.155]) by mx1.freebsd.org (Postfix) with ESMTP id 9C8271E7 for ; Mon, 25 Feb 2013 07:25:53 +0000 (UTC) Received: from [98.138.226.180] by nm25.bullet.mail.ne1.yahoo.com with NNFMP; 25 Feb 2013 07:25:47 -0000 Received: from [98.138.226.124] by tm15.bullet.mail.ne1.yahoo.com with NNFMP; 25 Feb 2013 07:25:47 -0000 Received: from [127.0.0.1] by smtp203.mail.ne1.yahoo.com with NNFMP; 25 Feb 2013 07:25:47 -0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yahoo.ca; s=s1024; t=1361777147; bh=Qs+PZZeyLAGbT8gAuDkAnWrxoKTET8FEmU5LUQI4yHg=; h=X-Yahoo-Newman-Id:X-Yahoo-Newman-Property:X-YMail-OSG:X-Yahoo-SMTP:Received:Received:MIME-Version:X-Received:Received:In-Reply-To:References:Date:Message-ID:Subject:From:To:Cc:Content-Type; b=i8AXNP+Iz5ootsWusRkWlRXEYrwXY1zhU3Icq0AxZmRNgMWnu827w8ticJWn5/qgUIVMt7dd7m5KvnN+jhWTzqvZWiCcx3v5qIu0oFKnnE5buHlWwXhSGUSZ7V9POC7bQ8wrQvc0HxAgZsOEIgdzS0anKnZMIJ0DuK/HF0YZTm0= X-Yahoo-Newman-Id: 576324.56143.bm@smtp203.mail.ne1.yahoo.com X-Yahoo-Newman-Property: ymail-3 X-YMail-OSG: nq1zjAAVM1npbFMyektanrdeiiEw7n6PTLDmlSKO0DBo4FG 91VWddkENq10EtVWJb67TzTuXLKF4dXI5e8uwsoAtojOmV1KCMq6i6nAx1of qkHCF97NYk6JsjZ1pr1ceXJawGol1jHZzxgt6AAwhydc3r7LLLy4VF8WMq0c 6i44lDp_jeRVkSW3QT3tsrIBmvTAL8aZVlX4Nzg6R1WS119Tur96gOP0vC0v AQdzce7FIveN6ENqWalEnFf3eh.ACO.1BHmXUmOWJNRA1UZ.SBRzYTw1iWC6 BdCoaBHTcAriDeydf8C4LEPe.pxnWtR_z5hqXUcfUHTdwTrAqwgGe_9Yb6pj cTx1iZrllScJe3BKQc5HxNOlOXFMV_lEyURyzyvPz_WPkXdzDOnxTj.zx9Lf 6UJheR0Zi0f0b1IJdWdB0NQ0z6p2eek51hayRB2XkqbXwDwlGsXKr.LGvKjN qNzkumk4biHBm59UJKgwv X-Yahoo-SMTP: Xr6qjFWswBAEmd20sAvB4Q3keqXvXsIH9TjJ Received: from mail-pb0-f43.google.com (moonlightakkiy@209.85.160.43 with plain) by smtp203.mail.ne1.yahoo.com with SMTP; 24 Feb 2013 23:25:47 -0800 PST Received: by mail-pb0-f43.google.com with SMTP id md12so1519043pbc.2 for ; Sun, 24 Feb 2013 23:25:46 -0800 (PST) MIME-Version: 1.0 X-Received: by 10.66.139.227 with SMTP id rb3mr17355245pab.107.1361777146635; Sun, 24 Feb 2013 23:25:46 -0800 (PST) Received: by 10.68.52.170 with HTTP; Sun, 24 Feb 2013 23:25:46 -0800 (PST) In-Reply-To: References: Date: Mon, 25 Feb 2013 00:25:46 -0700 Message-ID: Subject: Re: [RFT] net80211 TX serialisation, take #4 From: PseudoCylon To: Adrian Chadd Content-Type: text/plain; charset=ISO-8859-1 Cc: freebsd-wireless@freebsd.org X-BeenThere: freebsd-wireless@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Discussions of 802.11 stack, tools device driver development." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 25 Feb 2013 07:25:54 -0000 On Sun, Feb 24, 2013 at 12:45 AM, Adrian Chadd wrote: > On 23 February 2013 22:30, PseudoCylon wrote: >>> So, this is all pretty terrible. The only sane solution for now is to >>> make my VAP TX lock an IC TX lock,and grab said IC TX lock for all >>> VAPs. That way the driver can grab the IC TX lock when it's doing >>> deferred sends and it'll be sure the lock is held when it decides to >>> grab/increment sequence numbers from ni->ni_txseqs[]. >> >> I don't think >> lock(); >> ni->ni_txseqs[]++; >> unlock(); >> can fix the problem because no one can guarantee which process/thread >> grabs the lock next. > > Yup, that's definitely a problem. > > The problem here is that packets coming into net80211 - either via > vap->input() (bpf injection) or vap->transmit() (802.3) , have to do > this: There isn't any sequence relations between bpf injection and packets from vap->transmit(). First come first enqueue should work. 80211 stack generated packets --\ bpf_injection -> ieee80211_output()--\ vap_transmit() -> vap_queue -> 80211stack --> driver_queue > > * processed in order; > * the same order as they're then pushed into the power save / age queue; > * the same order that these frames get assigned sequence numbers and > enforce state (eg considering things for ampdu, power save, etc); > * the same order that they're queued to the driver; > * the same order that they're dequeued from the driver; > * the same order that they're processed (eg crypto encaps); > * the same order that it's then passed down into the driver - either > via direct dispatch or deferred queue. > > >>> * .. and do this without tearing my hair out. >> >> The sequence will be messed up during moving packets from one queue to >> another, i.e from driver queue to hardware queue. As long as packets >> are in a queue (in a linked list) sequence won't change unless we >> explicitly write such code. So... >> >> Saving your hair option 1 >> tx() >> { >> for() { >> lock(); >> dequeue(m); /* assuming queue is in order */ >> ni_txseqs[]++ >> enqueue_working_queue(m); >> unlock(); >> ... >> process m >> ... >> lock(); >> /* >> * m may change here. >> * Whichever the thread does dequeues, m0 will be >> * the head of the queue, so sequence will be kept intact. >> * But, need to test if processing of m0 has been completed. >> */ >> dequeue_working_queue(m0); >> enqueue_driver_queue(m0); /* or hardware_queue() */ >> unlock(); >> } >> } >> This will keep sequence intact. > > Right. This is similar to my idea (or two.) > > There's a few other issues though! > > ic_raw_xmit() is called by a bunch of places to generate both raw > frames and management frames. This bypasses the vap queue and calls > the driver direct. As an example, injected EAPOL frames have CCMP IV > numbers (as they're encrypted!) but not necessarily sequence numbers. > Because the CCMP IV gets calculated int he driver at some point after > it's been queued, ANY slight race here causes some other frame to get > queued with a CCMP IV -after- the EAPOL frame, and it will get > dropped. Then you get your session dropped. :-) That's a tricky one. (I didn't think about encryption.) Queue only maintains the order of packets. It doesn't mean packets are processed in order. If packets need to be processed in order, should be done while the lock is held. I think this should do the trick. lock(); dequeue_working_queue(); /* or driver_queue */ if (IEEE80211_FC1_WEP) ieee80211_crypto_encap(); pass_to_hardware(); unlock(); > > ic_raw_xmit() is also an ic method, not a vap method. Yes, it bypasses > the vap queue. the same as bpf injection part > > There's deferred transmission going on (eg ath_start() getting called > from TX completion, as an example.) Should that be called under the > above lock() in your example? Yes. With encryption stuff, if_start or if_transmission will look like this. if_start/_transmit() { for () { lock(); dequeue_driver_queue(); enqueue_working_queue(); unlock(); ... process ... lock(); dequeue_working_queue(); if (IEEE80211_FC1_WEP) ieee80211_crypto_encap(); pass_to_hardware(); unlock(); } } > > And the TX sequence number stuff in iwn and ath, because the > aggregation code reuses the ni_txseqs[] array. So yes, the locking > wouldn't clash with the rest of net80211 (as only either the non-agg > TX path in net80211 or the TX path in ath/iwn would be using that > variable) but it's still a pain in the ass. If we do a good job on serialization -- matching actual order of packets sent out and tx_seqs in 80211 stack -- driver shouldn't need to touch ni_txseqs. It is still a big if. > > Grr. > > The ridiculously complicated, broken-down-into-bits version: > > * add a new mbuf tag to mbufs (optional) which lets us tag an mbuf > with a bpf TX params struct (ie, what gets optionally passed into the > raw xmit call); > * calls to ic_raw_xmit() become a VAP raw xmit; > * the VAP raw xmit pushes mbufs into the same ifnet queue that the VAP > frames have (ie right now the VAP ifnet queue) - ie, serialise the raw > and non-raw VAP frames before it gets processed for 802.11 state; > * .. then move the VAP transmit to your model - which is > + vap transmit and raw transmit just push frames into the vap ifnet queue; > + vap transmit then schedules a deferred task or a direct dispatch + > deferred task if we cant grab the VAP TX lock; > + the vap transmit task/dispatch routine handles the frames one at a > time - either inside a task queue or inside the VAP TX lock - thus all > of the 802.3 -> 802.11 encapsulation and state handling is correctly > serialised _and_ the order of sequence numbers and frame handling is > maintained; > + then those frames have to be queued to the driver in _exactly_ the > same order - so either the VAP TX task/process queues them into a > driver staging queue and kicks the driver side ,or it goes into a > separate task to do driver dispatch; > + .. and then it's up to the driver to dequeue each frame, handle it > and push them into the hardware in the same order. > > Now, that's totally, ridiculously complicated but it does avoid a > bunch of reentrant and long-held-lock problems. > > The ridiculously uncomplicated version: > > * put a big IC TX lock around ieee80211_start(), ieee80211_output() > and any routine that is calling ieee80211_encap() -> if_transmit() or > ieee80211_encap() -> ic_raw_xmit(); > * do the same with the ageq and powersave queue that dispatches to the > driver - grab that big IC TX lock, dequeue all the frames to the > driver, release the IC lock; > * the driver TX code must grab the net80211 IC TX lock before it does > a deferred transmit - so the driver TX (not TX completion - TX!) > always has a consistent set of locking; > * do a bunch of net80211 TX path refactoring to unify the TX path a bit; > * just give in to the long-held locks and worry about tidying this > crap up later; > * hope to dear god there aren't any lock-order issues between the > comlock and this new lock; same as the other crap that's going on; This is the most scary part (LOR). > > Opinions? :) Here is a list of ideas presented so far. - lock whole tx path - use taskqueue - use queue (or linked lists) I'd like to avoid holding a lock for long time. taskqueue may have performance issue. Using queue is the one still standing. I think it should fix the problem. AK