Hi,
Sorry for my late reply.
Johannes Berg wrote:
On Fri, 2012-03-23 at 15:03 +0100, Michał Kazior wrote:
So I took a closer look at this now.
I think we should probably apply the first two patches now, and the
third with changes. After that, I'm not so sure.
You mean the following, right?
mac80211: prepare ieee80211_mandatory_rates to per-vif
mac80211: prepare ieee80211_sta_get_rates to per-vif
[tofix] mac80211: prepare ieee80211_frame_duration to per-vif
Should I resend them all, or just the 3rd one once I fix it?
You should probably send them as [PATCH].
Okay.
What if we create a new structure that would be a channel
stream/reservation? Driver could then report multiple such objects. Each
such would have a tx queue and it's own locking. Let's assume something
like:
struct ieee80211_channel_stream {
struct list_head vif; // list of vifs we're attached to
struct ieee80211_channel *ch;
struct sk_buff_head pending;
int stop_reasons[IEEE80211_MAX_QUEUES];
// .. other stuff
u8 *drv_priv[0] __attribute__((__aligned__((void *))));
};
struct ieee80211_vif {
struct ieee80211_channel_stream *channel_stream;
// .. rest of struct
};
* We could then reject set_channel() based on number of different
oper_channels and channel reservations
* We could handle off-channel through a channel reservation by either
using an unused one (which would result in hardware offloaded
off-channel) or we would lock&flush it, switch to off-channel, do
work, and get back
I'm not sure we can easily use such a "stream" for off-channel
operation. In particular, that "stream" might not support multiple
queues.
Does it need to? It could as well just map all queues to a single hw
queue with this design. I'm not sure if I get your point here.
Is it okay for us to not care if the driver supports real queues for
ACs? Should we care if a driver maps all ACs from a channel context (or
vif for that matter) onto a single hw queue? I'm wondering whether that
would simplify code paths?
We should however consider that a device may not support certain
interface combinations. One thing that comes immediately to my mind is
the following: suppose we support STA+STA on different channels, AP+AP
on the same channel, but do not support AP+AP on different channels:
1. create wlan0, set channel=1, and start AP
2. create wlan1, set channel=1, and start AP
3. do set_channel(wlan1, 3)<-- oops
This would probably be fixed easily with:
1. forbid changing channels on a running interface (i'm aware that's
the same case for mac addr)
2. thus we would require: ifdown + setchannel + ifup
But that's just one case. Are there any others? This needs investigation.
So my interface combinations advertising already supports
"num_different_channels" per combination, so that would be easy to
express. Switching the channel while the AP interface is up is probably
not desirable anyway.
You're right. I've looked at the combination structures and they seem
quite nice.
What is still missing here is the CAB queue (and possible other that may
come up in the future) you mentioned in the other thread.
Ah, I see you did see my other thread :-)
I'm not sure
whether we may use queue_mapping for our own evil purposes here. If not,
then how about this:
struct ieee80211_queue_types { // or tx_queue_types?
IEEE80211_QUEUE_VO,
IEEE80211_QUEUE_VI,
IEEE80211_QUEUE_BE,
IEEE80211_QUEUE_BK,
IEEE80211_QUEUE_CAB,
IEEE80211_QUEUE_LAST
};
[...]
This looks kind of ugly, but oh well. This way the driver will be able
to setup mapping between mac80211 and internal device queues. It should
also be possible to map one hardware queue to multiple
ieee80211_channel_stream structures (queues is a list of pointers) so
multiple vifs on the same channel is okay.
I don't think this makes a lot of sense, tbh. This isn't a separate AC,
I'd rather keep them separated.
You mean the CAB queue separated from ACs?
What are your thoughts on this? Is this terribly wrong or is it the way
to go? Have I missed something?
I think that this is all a bit overkill :)
I'm looking at the code right now, and I think we should treat ACs,
queues and channels separately. If we combine queue sets& channels into
one, I think we'll find drivers that don't work correctly with this.
Some devices for instance might have a set of queues for each interface,
even if they're on the same channel. I don't want to lose flexibility to
handle that.
You mean a driver that can handle two vifs simultaneously but on the
same channel and have two separate queue sets for that?
Then we need to move queues to ieee80211_vif and create callbacks to
bind channels contexts with a vif (just you mentioned that in other email).
I think we'd hold the AC in skb_queue_mapping, since that's assigned
early in the virtual interface queues already, and we need it to be the
AC there for proper queue selection.
The hardware queue we can put into a separate field in tx_info, I've
made space for a u8 for this in a patch I have pending (but not sent to
the list yet.)
Ignoring the monitor mode interface, I think the simplest thing we can
do is
struct ieee80211_hw {
...
u8 offchannel_queue;
};
struct ieee80211_vif {
...
u8 hw_queue[IEEE80211_NUM_ACS];
u8 after_dtim_queue;
};
For any of these (offchannel_queue, after_dtim_queue, hw_queue[i]), the
queue number Q must be 0<= Q< hw.queues. This allows us to keep all
per-hw-queue state "flattened" in ieee80211_local's pending,
queue_stop_reasons, etc. instead of separating all of it out into
separate structures.
So you want "offchannel_queue", "hw_queue[]" and "after_dtim_queue" to
point to driver/device hw queue id?
When a queue is stopped, we do:
void queue_stop(int q)
{
for_each_interface(vif) {
if (vif->hw_queue[AC] == q) // for all 4 ACs
stop netdev queue (vif->dev, AC)
if (vif->after_dtim_queue == q)
stop_all_queues(vif->dev)
}
}
Or so ... after_dtim_queue is kinda a special case.
Oh, I see. So stopping CAB would be like "stop all the things".
But how do we then check if we need to stop given queue or not? Let's
say a driver stops q=2 which corresponds to BE on vif0 and vif1. But
then comes mac80211 aggregation and stops BE on vif0. I can only think
of a single solution to that: "u8 lock_reasons[256];" in ieee80211_local
but it seems like an overkill or is it not?
If offchannel_queue is stopped, we'll have to refuse offchannel TX from
nl80211 -- that seems *very* unlikely though.
Do you think this would work?
I think that covers the queue part -- I'll split the thread and will
talk about multi-channel in a separate email.
johannes
-- Pozdrawiam / Best regards, Michal Kazior.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
