On Wed, 2009-11-18 at 14:53 -0500, Luis R. Rodriguez wrote:
> > Ok so this might /slightly/ impact ath5k/ath9k performance, but maybe
> > they shouldn't be using a tasklet anyway but also use work structs...
>
> Shoudln't by current mac80211 API documentation or shouldn't as in
> it'd be good to change it? From the current docs I read hard IRQ
> context but that could be clarified.
The latter :) I really see no real reason for it to be using tasklets.
> I'm not opposed to changing ath[59]k, in fact I want this change to
> happen; in the future more of our drivers will hopefully share more
> code and this then does mean eventually doing more things in process
> context. But if something is affecting performance terribly without proper
> coordination for a specific kernel release it would be terrible for
> us and would prefer to try to avoid it. Can you test to see how much
> performance difference you see with this?
Not really, I think it'd be hard to test, I expect a really small
increase in CPU utilisation. It's only a difference between processing
it, and putting it on a list, pulling it off a list and processing it.
> > --- wireless-testing.orig/net/mac80211/main.c 2009-11-18 19:40:08.000000000 +0100
> > +++ wireless-testing/net/mac80211/main.c 2009-11-18 19:51:51.000000000 +0100
> > @@ -625,16 +623,20 @@ void ieee80211_unregister_hw(struct ieee
> >
> > cancel_work_sync(&local->reconfig_filter);
> >
> > + cancel_work_sync(&local->tx_status_work);
> > +
>
> Are there changes required during suspend as well?
Good question, but I think not since we don't touch the tasklet during
suspend right now. Now, that might be wrong too, but at least it
wouldn't change anything (actually we flush the workqueue for suspend
anyway, don't we?)
> > --- wireless-testing.orig/net/mac80211/status.c 2009-11-18 19:40:08.000000000 +0100
> > +++ wireless-testing/net/mac80211/status.c 2009-11-18 20:05:55.000000000 +0100
> > @@ -16,25 +16,31 @@
> > #include "led.h"
> >
> >
> > -void ieee80211_tx_status_irqsafe(struct ieee80211_hw *hw,
> > - struct sk_buff *skb)
> > +void ieee80211_tx_status_irqsafe(struct ieee80211_hw *hw, struct sk_buff *skb)
> > {
> > struct ieee80211_local *local = hw_to_local(hw);
> > struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
> > - int tmp;
> > + struct sk_buff_head *queue;
> > + int num;
> >
> > - skb->pkt_type = IEEE80211_TX_STATUS_MSG;
> > - skb_queue_tail(info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS ?
> > - &local->skb_queue : &local->skb_queue_unreliable, skb);
> > - tmp = skb_queue_len(&local->skb_queue) +
> > - skb_queue_len(&local->skb_queue_unreliable);
> > - while (tmp > IEEE80211_IRQSAFE_QUEUE_LIMIT &&
> > - (skb = skb_dequeue(&local->skb_queue_unreliable))) {
> > + if (info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS)
> > + queue = &local->tx_status;
> > + else
> > + queue = &local->tx_status_unreliable;
> > +
> > + skb_queue_tail(queue, skb);
>
> This sort of change could go into a seprate patch to help review
> easier.
Huh? The patch really only changes the if from being inlined into the
function call to not being there .. while changing the skb queue names
anyway.
> > + /* if we got over the limit with this frame, try to make room */
> > + if (num > IEEE80211_TX_STATUS_QUEUE_LIMIT &&
> > + (skb = skb_dequeue(&local->tx_status_unreliable))) {
> > dev_kfree_skb_irq(skb);
>
> Note before there was a while loop and now just a if branch. Why can we
> get away with freeing now just one buffer? It would be nice to see
> this explained in the commit log entry as it is not obvious to me.
Ah yes, I meant to explain that. We only use the status queues for TX
status now and not for RX too as before, so before it could actually
happen that we could free more than one off the unreliable queue, while
now it really can only be one.
Not that it actually matters, I just realised we never ever even use the
unreliable queue since we _always_ set the
IEEE80211_TX_CTL_REQ_TX_STATUS bit!
> > - tmp--;
> > I802_DEBUG_INC(local->tx_status_drop);
> > }
> > - tasklet_schedule(&local->tasklet);
> > +
> > + ieee80211_queue_work(&local->hw, &local->tx_status_work);
>
> How about just moving this last party to a helper as well while at it?
What?
> > - rcu_read_lock();
> > - list_for_each_entry_rcu(sdata, &local->interfaces, list) {
> > + mutex_lock(&local->iflist_mtx);
>
> Hm, what change here allows us to lift the rcu_read_lock, I
> was under the impression it would be the for sdata.
Well yeah, but we have the mutex now instead.
> > --- wireless-testing.orig/include/net/mac80211.h 2009-11-18 20:09:21.000000000 +0100
> > +++ wireless-testing/include/net/mac80211.h 2009-11-18 20:11:00.000000000 +0100
> > @@ -1734,31 +1737,28 @@ static inline void ieee80211_rx_ni(struc
> > * transmitted. It is permissible to not call this function for
> > * multicast frames but this can affect statistics.
> > *
> > - * This function may not be called in IRQ context. Calls to this function
> ^^^
>
> > - * for a single hardware must be synchronized against each other. Calls
> > - * to this function and ieee80211_tx_status_irqsafe() may not be mixed
> > - * for a single hardware.
> > + * This function may not be called in hard or soft IRQ context. Calls
> ^^^^ ^^ ^^^^ ^^^
?
johannes
Attachment:
signature.asc
Description: This is a digitally signed message part
