[RFC] change mac80211_hwsim tx_rates to ieee80211_tx_rate

[RFC] change mac80211_hwsim tx_rates to ieee80211_tx_rate

[Benjamin Beichler]

Hi,

we are working on a sophisticated Wifi simulation framework based on
mac80211_hwsim. This includes the simulation of frame transmissions with
realistic channel access and channel conditions. Therefore we need
several information (e.g. long/short gi, usage of RTS/CTS, ...), which
are available on a per frame/rate basis from struct ieee80211_tx_rate.
But this information is thrown away by the conversation to struct
hwsim_tx_rate (eg. in mac80211_hwsim_tx_frame_nl)

Firstly I think this information is valuable and I don't believe, the 4
Byte per frame greatly influences speed. Moreover the explicit double
copy (firstly from the info->status.rates to tx_attempts, secondly by
nla_put), takes longer than simply put the whole info->status.rates into
the netlink message.

So I would propose to put the whole struct into the netlink messages,
but I think that will break up the communication to e.g. bob copelands
wmediumd and similar simulations. I would like to have our
Implementation working with mainline kernels and therefore ask how we
could achieve this easily.

Obviously, we could define another field in the hwsim messages, but as
bob copeland already stated, significantly more information within the
netlink messages could  intensify the timing overhead of hwsim.

Any suggestions?

Re: [RFC] change mac80211_hwsim tx_rates to ieee80211_tx_rate

[Johannes Berg]

> So I would propose to put the whole struct into the netlink messages,

This is a terrible idea, since internal changes to this struct would
break the userland API/ABI. hwsim seems perhaps less important than
most APIs, but there is wmediumd etc. already.

> but I think that will break up the communication to e.g. bob
> copelands
> wmediumd and similar simulations. I would like to have our
> Implementation working with mainline kernels and therefore ask how we
> could achieve this easily.
> 
> Obviously, we could define another field in the hwsim messages, but
> as bob copeland already stated, significantly more information within
> the netlink messages could  intensify the timing overhead of hwsim.

I don't think we have any other choice but add the relevant fields as
proper attributes.

johannes

Re: [RFC] change mac80211_hwsim tx_rates to ieee80211_tx_rate

[Benjamin Beichler]

>> So I would propose to put the whole struct into the netlink messages,
> This is a terrible idea, since internal changes to this struct would
> break the userland API/ABI. hwsim seems perhaps less important than
> most APIs, but there is wmediumd etc. already.
I agree with that, but there exist also other code in hwsim, which is
tightly coupled with the mac80211 API, as e.g., the usage of
IEEE80211_TX_MAX_RATES, which already broke older versions of wmediumd
or similar implementations. Maybe a review regarding such things would
be good to decouple the userspace daemon from the special kernel version.

>> but I think that will break up the communication to e.g. bob
>> copelands
>> wmediumd and similar simulations. I would like to have our
>> Implementation working with mainline kernels and therefore ask how we
>> could achieve this easily.
>>
>> Obviously, we could define another field in the hwsim messages, but
>> as bob copeland already stated, significantly more information within
>> the netlink messages could  intensify the timing overhead of hwsim.
> I don't think we have any other choice but add the relevant fields as
> proper attributes.

I'm totally fine with that. Nonetheless,  I would suggest to add the
flags to "struct hwsim_tx_rate", since the flags are also tightly
coupled to the rates and tries of a frame. To not break up things, we
could add the flags as a separate attribute in the struct and not as
part of the bitfield like in the original. This would be possible, due
to the "__packed" flag, but I'm also unsure, whether this is a really
good idea for a userspace API/ABI.


Benjamin

Re: [RFC] change mac80211_hwsim tx_rates to ieee80211_tx_rate

[Johannes Berg]

> I agree with that, but there exist also other code in hwsim, which is
> tightly coupled with the mac80211 API, as e.g., the usage of
> IEEE80211_TX_MAX_RATES, which already broke older versions of
> wmediumd or similar implementations. Maybe a review regarding such
> things would be good to decouple the userspace daemon from the
> special kernel version.

Agree. It'd be better if that were using nested attributes etc.
Although then again, to really decouple this we should make hwsim
behave towards wmediumd more like real hardware would, and have it pass
just a single rate to userspace, with only success/fail indication
coming back - if it fails, it could walk down the chain of rates
itself. Right now we let wmediumd do this, which is why we have all
these API internals exposed to it.

> > > but I think that will break up the communication to e.g. bob
> > > copelands
> > > wmediumd and similar simulations. I would like to have our
> > > Implementation working with mainline kernels and therefore ask
> > > how we
> > > could achieve this easily.
> > > 
> > > Obviously, we could define another field in the hwsim messages,
> > > but
> > > as bob copeland already stated, significantly more information
> > > within
> > > the netlink messages could  intensify the timing overhead of
> > > hwsim.
> > I don't think we have any other choice but add the relevant fields
> > as
> > proper attributes.
> 
> I'm totally fine with that. Nonetheless,  I would suggest to add the
> flags to "struct hwsim_tx_rate", since the flags are also tightly
> coupled to the rates and tries of a frame. To not break up things, we
> could add the flags as a separate attribute in the struct and not as
> part of the bitfield like in the original. This would be possible,
> due
> to the "__packed" flag, but I'm also unsure, whether this is a really
> good idea for a userspace API/ABI.

Changing the struct size itself would break ABI though?

johannes

Re: [RFC] change mac80211_hwsim tx_rates to ieee80211_tx_rate

[Benjamin Beichler]

> I agree with that, but there exist also other code in hwsim, which is
>> tightly coupled with the mac80211 API, as e.g., the usage of
>> IEEE80211_TX_MAX_RATES, which already broke older versions of
>> wmediumd or similar implementations. Maybe a review regarding such
>> things would be good to decouple the userspace daemon from the
>> special kernel version.
> Agree. It'd be better if that were using nested attributes etc.
> Although then again, to really decouple this we should make hwsim
> behave towards wmediumd more like real hardware would, and have it pass
> just a single rate to userspace, with only success/fail indication
> coming back - if it fails, it could walk down the chain of rates
> itself. Right now we let wmediumd do this, which is why we have all
> these API internals exposed to it.
Mhh, I thought also some atheros drivers implement hardware multirate
retry changes, which maps to this struct. Only one rate per frame would
introduce a extreme additional communication overhead, which will make
testing with standard wmediumd impractical. I think we need to keep such
a structure, but we should align that with other mac80211 depended drivers.
>
>>>> but I think that will break up the communication to e.g. bob
>>>> copelands
>>>> wmediumd and similar simulations. I would like to have our
>>>> Implementation working with mainline kernels and therefore ask
>>>> how we
>>>> could achieve this easily.
>>>>
>>>> Obviously, we could define another field in the hwsim messages,
>>>> but
>>>> as bob copeland already stated, significantly more information
>>>> within
>>>> the netlink messages could  intensify the timing overhead of
>>>> hwsim.
>>> I don't think we have any other choice but add the relevant fields
>>> as
>>> proper attributes.
>> I'm totally fine with that. Nonetheless,  I would suggest to add the
>> flags to "struct hwsim_tx_rate", since the flags are also tightly
>> coupled to the rates and tries of a frame. To not break up things, we
>> could add the flags as a separate attribute in the struct and not as
>> part of the bitfield like in the original. This would be possible,
>> due
>> to the "__packed" flag, but I'm also unsure, whether this is a really
>> good idea for a userspace API/ABI.
> Changing the struct size itself would break ABI though?

You are totally right ... I was a bit absent. But the point is, without
the rates and tries, the flags are useless, so I think it is better to
put them together, but I'm also fine with another attribute ;-)


Benjamin

Re: [RFC] change mac80211_hwsim tx_rates to ieee80211_tx_rate

[Johannes Berg]

> Mhh, I thought also some atheros drivers implement hardware multirate
> retry changes, which maps to this struct. Only one rate per frame
> would introduce a extreme additional communication overhead, which
> will make testing with standard wmediumd impractical. I think we need
> to keep such a structure, but we should align that with other
> mac80211 depended drivers.

Yeah, I was being imprecise. The driver interface is usually similar to
the mac80211 struct in one way or another (though it might also be a
more global table, like other drivers implement).

I was more thinking of the actual air interface. I'm not too worried
about the efficiency of this (we can push quite a few gbps over hwsim
today iirc), but it actually doesn't make sense because an accurate
simulation would require NAV/TXOP simulation, and that wouldn't be
possible with "software retry".

So I think our best bet remains to map this to new attributes - better
with properly formatted ones etc. than with the struct (keeping that
only for compatibility)

If you're worried about the overhead, we could consider converting
hwsim to use the rate_table API - see struct ieee80211_sta -> rates,
and maybe adding a signal to update that in the driver, send that to
userspace directly and work with that, rather than "serializing" the
table for every frame?

johannes