Skip site navigation (1)Skip section navigation (2) 
Date:      Sat, 6 Oct 2012 13:00:21 -0600
From:      PseudoCylon <moonlightakkiy@yahoo.ca>
To:        Adrian Chadd <adrian@freebsd.org>, freebsd-wireless@freebsd.org
Subject:   Re: [RFC] How the TX path works; I'd like to fix this before 10.x
Message-ID:  <CAFZ_MYKampEVfyPOXyGd9Cshsr7xpP48%2BxOyV8uU8LuzMpTdRg@mail.gmail.com>
next in thread | raw e-mail | index | archive | help 
> ------------------------------
>
> Message: 4
> Date: Fri, 5 Oct 2012 23:44:10 -0700
> From: Adrian Chadd <adrian@freebsd.org>
> Subject: [RFC] How the TX path works; I'd like to fix this before 10.x
> To: freebsd-wireless@freebsd.org
> Message-ID:
>         <CAJ-VmomS=7Udyixqqss_1261EXzEpeYBuAWqwiKEah=5V4rrWw@mail.gmail.com>
> Content-Type: text/plain; charset=ISO-8859-1
>
> Before I continue - if_transmit() is not the answer. if_transmit()
> guarantess _no_ serialisation at all. So we'd stlil have to stuff
> things into queues. And ensure only one thing is running at once.

As far as I know, if_transmit/if_start are options for drivers, not
for the serialisation.
if_transmit to use device specific queue
if_start to use if_snd queue
i.e.
http://fxr.watson.org/fxr/source/dev/e1000/if_em.c#L2941

>
> I could wrap a big VAP TX lock around ieee80211_output() and
> ieee80211_start(), ensuring they don't run over each other. But
> long-held locks need to go away and die. yes, even the ones in the
> ath(4) driver that I've introduced. They're there because of a lack of
> synchronisation and queue design. A lot of the gige/10ge drivers use
> long-held locks.. sigh.
>
> I could create a net80211 TX tasklet for each vap (or ic, _maybe_)
> which serialises the TX path. ieee80211_start() would just schedule
> the tasklet to run. That would serialise all of the parallel TX entry
> points and solve a bunch of the subtle sequence number and other TX
> state races that are occuring. That doesn't solve the ic_raw_xmit() or
> ieee80211_output() problem, as both of those can also do TX 802.11
> encapsulation and kick off parts of the state machine.

I prefer taskqueue over new lock. I have had enough of LORs already.

We just need to decide where/how to funnel all tx entries.
Currently,
pseudo devices (i.e. if_bridge) \
IP stack                                     --> ieee80211 stack  --> driver
                                                       ic_raw_xmit         /
so we need to ensure serialization (by lock or taskqueue) at 2 points,
1) between upper layer and ieee80211,
2) driver and ieee80211.
If we solve the raw_xmit problem, there will be only 1 point to take care.


An idea
Guarantee only one thread is running at any moment. So, first queued
first dequeued and tx'd without a lock.

if_start() {
        if (sc_task == NOT_RUNNING)
                wakeup(sc_txtask);
}

if_txtask() {
        for (;sc_txtask_exit != DONE;) {
                IFQ_DRV_DEQUEUE();
                if (m == NULL) {
                        sc_txtask = NOT_RUNNING;
                        tsleep();
                        sc_txtask = RUNNING;
                } else
                        tx();
        }
}

tx_callback() {
        ...
        if (sc_task == NOT_RUNNING)
                wakeup(sc_txtask);
}

iv_vap_create() {
        ...
        taskqueue_enqueue(sc_taskqueue);
}

iv_vap_delete() {
        ...
        sc_txtask_exit = DONE;
        if (sc_task == NOT_RUNNING)
                wakeup(sc_txtask);
}

>
> It doesn't solve the same issues with the drivers .Yes, even if we
> converted them to use if_transmit(). iwn(4) solves this by just having
> a big IWN_LOCK() but it releases it when doing anything stack related.
> I'm not sure if it holds it across the whole TX path through
> iwn_start(). In any case, it's a big lock. ath(4) can and does have
> multiple ath_start() instances running in multiple kernel threads,
> fighting to dequeue frames from the ifnet. This still can cause issues
> with non-aggregate sequence number and CCMP IV counter allocation.
> Sigh.
>
> God even knows what to do about USB devices in all of this.
>

Similar to other drivers, i.e. iwn(4). The difference is USB drivers
create per-USB-pipe queue in addition to if_snd queue. So that, tx
path look like
if_transmit -> queue -> if_start -> dequeue -> driver_tx -> usb_queue
-> usb_stack
It seems redundant. I'd like to change that to (for run(4))
if_transmit -> driver_tx -> usb_queue -> usb_stack


AK

Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAFZ_MYKampEVfyPOXyGd9Cshsr7xpP48%2BxOyV8uU8LuzMpTdRg>