2007-10-31 06:14:54

by Bruno Randolf

[permalink] [raw]
Subject: ilog2 overkill in ieee80211_get_hdrlen?

hello!

just a question...

in 'net/mac80211/util.c', in ieee80211_get_hdrlen (line 175), a function ilog2
is used to determine the number of bits to shift for
IEEE80211_STYPE_QOS_DATA. isn't that a bit of an overkill when we could just
do >> 6? i mean IEEE80211_STYPE_QOS_DATA isn't going to change after all...

/*
* The QoS Control field is two bytes and its presence is
* indicated by the IEEE80211_STYPE_QOS_DATA bit. Add 2 to
* hdrlen if that bit is set.
* This works by masking out the bit and shifting it to
* bit position 1 so the result has the value 0 or 2.
*/
hdrlen += (fc & IEEE80211_STYPE_QOS_DATA)
>> (ilog2(IEEE80211_STYPE_QOS_DATA)-1);

i don't know if there is a reason for this, but it all seems overly
complicated to me.

if (fc & IEEE80211_STYPE_QOS_DATA)
hdrlen += 2;

would be a bit more readable.

bruno


2007-10-31 12:19:14

by Johannes Berg

[permalink] [raw]
Subject: Re: ilog2 overkill in ieee80211_get_hdrlen?


> in 'net/mac80211/util.c', in ieee80211_get_hdrlen (line 175), a function ilog2
> is used to determine the number of bits to shift for
> IEEE80211_STYPE_QOS_DATA. isn't that a bit of an overkill when we could just
> do >> 6? i mean IEEE80211_STYPE_QOS_DATA isn't going to change after all...

Yeah but ilog2() evaluates to a constant here so it doesn't matter, and
I think this is more readable.

> if (fc & IEEE80211_STYPE_QOS_DATA)
> hdrlen += 2;
>
> would be a bit more readable.

It used to be that but that's a branch and the generated code is better
the other way.

johannes


Attachments:
signature.asc (828.00 B)
This is a digitally signed message part

2007-11-01 01:42:39

by Bruno Randolf

[permalink] [raw]
Subject: Re: ilog2 overkill in ieee80211_get_hdrlen?

johannes,

thanks for the explanation!

bruno

On Wednesday 31 October 2007 21:20:35 Johannes Berg wrote:
> > in 'net/mac80211/util.c', in ieee80211_get_hdrlen (line 175), a function
> > ilog2 is used to determine the number of bits to shift for
> > IEEE80211_STYPE_QOS_DATA. isn't that a bit of an overkill when we could
> > just do >> 6? i mean IEEE80211_STYPE_QOS_DATA isn't going to change after
> > all...
>
> Yeah but ilog2() evaluates to a constant here so it doesn't matter, and
> I think this is more readable.
>
> > if (fc & IEEE80211_STYPE_QOS_DATA)
> > hdrlen += 2;
> >
> > would be a bit more readable.
>
> It used to be that but that's a branch and the generated code is better
> the other way.
>
> johannes