ilog2 overkill in ieee80211_get_hdrlen?

ilog2 overkill in ieee80211_get_hdrlen?

[bruno randolf]

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

Re: ilog2 overkill in ieee80211_get_hdrlen?

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 606 bytes --]


> 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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: ilog2 overkill in ieee80211_get_hdrlen?

[bruno randolf]

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