[PATCH 4/7] mac80211: make ieee80211_get_hdrlen_from_skb return unsigned

[PATCH 4/7] mac80211: make ieee80211_get_hdrlen_from_skb return unsigned

[Harvey Harrison]

Many callers already expect it to.

Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
---
 include/net/mac80211.h |    2 +-
 net/mac80211/util.c    |   10 +++++-----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 53c3b5e..7ab4ff6 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -1535,7 +1535,7 @@ ieee80211_get_buffered_bc(struct ieee80211_hw *hw, struct ieee80211_vif *vif);
  *
  * @skb: the frame
  */
-int ieee80211_get_hdrlen_from_skb(const struct sk_buff *skb);
+unsigned int ieee80211_get_hdrlen_from_skb(const struct sk_buff *skb);
 
 /**
  * ieee80211_get_hdrlen - get header length from frame control
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index fade001..60fec1c 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -165,15 +165,15 @@ out:
 }
 EXPORT_SYMBOL(ieee80211_hdrlen);
 
-int ieee80211_get_hdrlen_from_skb(const struct sk_buff *skb)
+unsigned int ieee80211_get_hdrlen_from_skb(const struct sk_buff *skb)
 {
 	const struct ieee80211_hdr *hdr = (const struct ieee80211_hdr *) skb->data;
-	int hdrlen;
+	unsigned int hdrlen;
 
-	if (unlikely(skb->len < 10))
+	if (skb->len < 10)
 		return 0;
-	hdrlen = ieee80211_get_hdrlen(le16_to_cpu(hdr->frame_control));
-	if (unlikely(hdrlen > skb->len))
+	hdrlen = ieee80211_hdrlen(hdr->frame_control);
+	if (hdrlen > skb->len)
 		return 0;
 	return hdrlen;
 }
-- 
1.5.6.rc1.257.gba91d

Re: [PATCH 4/7] mac80211: make ieee80211_get_hdrlen_from_skb return unsigned

[Michael Buesch]

On Friday 06 June 2008 19:51:12 Harvey Harrison wrote:
> Many callers already expect it to.
> 
> Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
> ---
>  include/net/mac80211.h |    2 +-
>  net/mac80211/util.c    |   10 +++++-----
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
> index 53c3b5e..7ab4ff6 100644
> --- a/include/net/mac80211.h
> +++ b/include/net/mac80211.h
> @@ -1535,7 +1535,7 @@ ieee80211_get_buffered_bc(struct ieee80211_hw *hw, struct ieee80211_vif *vif);
>   *
>   * @skb: the frame
>   */
> -int ieee80211_get_hdrlen_from_skb(const struct sk_buff *skb);
> +unsigned int ieee80211_get_hdrlen_from_skb(const struct sk_buff *skb);
>  
>  /**
>   * ieee80211_get_hdrlen - get header length from frame control
> diff --git a/net/mac80211/util.c b/net/mac80211/util.c
> index fade001..60fec1c 100644
> --- a/net/mac80211/util.c
> +++ b/net/mac80211/util.c
> @@ -165,15 +165,15 @@ out:
>  }
>  EXPORT_SYMBOL(ieee80211_hdrlen);
>  
> -int ieee80211_get_hdrlen_from_skb(const struct sk_buff *skb)
> +unsigned int ieee80211_get_hdrlen_from_skb(const struct sk_buff *skb)
>  {
>  	const struct ieee80211_hdr *hdr = (const struct ieee80211_hdr *) skb->data;
> -	int hdrlen;
> +	unsigned int hdrlen;
>  
> -	if (unlikely(skb->len < 10))
> +	if (skb->len < 10)

Why are you removing the unlikely()? This should actually be pretty unlikely.
Sane firmware will drop short packets, so the kernel won't ever see them.

>  		return 0;
> -	hdrlen = ieee80211_get_hdrlen(le16_to_cpu(hdr->frame_control));
> -	if (unlikely(hdrlen > skb->len))
> +	hdrlen = ieee80211_hdrlen(hdr->frame_control);
> +	if (hdrlen > skb->len)

I think this also is unlikely, as it can only come from corrupted packets.

>  		return 0;
>  	return hdrlen;
>  }



-- 
Greetings Michael.

Re: [PATCH 4/7] mac80211: make ieee80211_get_hdrlen_from_skb return unsigned

[Harvey Harrison]

On Fri, 2008-06-06 at 20:24 +0200, Michael Buesch wrote:
> On Friday 06 June 2008 19:51:12 Harvey Harrison wrote:
> > -int ieee80211_get_hdrlen_from_skb(const struct sk_buff *skb)
> > +unsigned int ieee80211_get_hdrlen_from_skb(const struct sk_buff *skb)
> >  {
> >  	const struct ieee80211_hdr *hdr = (const struct ieee80211_hdr *) skb->data;
> > -	int hdrlen;
> > +	unsigned int hdrlen;
> >  
> > -	if (unlikely(skb->len < 10))
> > +	if (skb->len < 10)
> 
> Why are you removing the unlikely()? This should actually be pretty unlikely.
> Sane firmware will drop short packets, so the kernel won't ever see them.
> 

When all that is in the if-block is a return, there's not much point.
It's a trivial test/return and gcc does a fine job with this.

> >  		return 0;
> > -	hdrlen = ieee80211_get_hdrlen(le16_to_cpu(hdr->frame_control));
> > -	if (unlikely(hdrlen > skb->len))
> > +	hdrlen = ieee80211_hdrlen(hdr->frame_control);
> > +	if (hdrlen > skb->len)
> 
> I think this also is unlikely, as it can only come from corrupted packets.
> 

unlikely() isn't some magic make-me-faster function, it just moves code
to the end of the function to get it out of the icache and jumps to it
in the unlikely case it is taken.  When all there is is a return, I
don't think it even makes any difference.  In both these cases a comment
is probably more appropriate.

Harvey

Re: [PATCH 4/7] mac80211: make ieee80211_get_hdrlen_from_skb return unsigned

[Pavel Roskin]

On Fri, 2008-06-06 at 11:47 -0700, Harvey Harrison wrote:

> unlikely() isn't some magic make-me-faster function, it just moves co=
de
> to the end of the function to get it out of the icache and jumps to i=
t
> in the unlikely case it is taken.  When all there is is a return, I
> don't think it even makes any difference.  In both these cases a comm=
ent
> is probably more appropriate.

If we don't more return to the end, we get something like:

compare skb->len to 10
if more or equal goto 1
return
1: <the likely case>

=EF=BB=BFI don't know much about modern processor design, but I remembe=
r reading
that even a very short jump would flush the instruction pipeline and
cause some delay.

I would keep unlikely() and let the compiler think what to do with it.

--=20
Regards,
Pavel Roskin
--
To unsubscribe from this list: send the line "unsubscribe linux-wireles=
s" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/7] mac80211: make ieee80211_get_hdrlen_from_skb return unsigned

[Harvey Harrison]

On Fri, 2008-06-06 at 16:15 -0400, Pavel Roskin wrote:
> On Fri, 2008-06-06 at 11:47 -0700, Harvey Harrison wrote:
>=20
> > unlikely() isn't some magic make-me-faster function, it just moves =
code
> > to the end of the function to get it out of the icache and jumps to=
 it
> > in the unlikely case it is taken.  When all there is is a return, I
> > don't think it even makes any difference.  In both these cases a co=
mment
> > is probably more appropriate.
>=20
> If we don't more return to the end, we get something like:
>=20
> compare skb->len to 10
> if more or equal goto 1
> return
> 1: <the likely case>
>=20
> =EF=BB=BFI don't know much about modern processor design, but I remem=
ber reading
> that even a very short jump would flush the instruction pipeline and
> cause some delay.
>=20
> I would keep unlikely() and let the compiler think what to do with it=
=2E
>=20

On X86-32 at least the generated code is exactly the same with or
without the unlikely()s.  I'll put them back in a follow-on if people
are really attached to them.

Harvey

--
To unsubscribe from this list: send the line "unsubscribe linux-wireles=
s" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/7] mac80211: make ieee80211_get_hdrlen_from_skb return unsigned

[Johannes Berg]

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


> unlikely() isn't some magic make-me-faster function, it just moves
> code
> to the end of the function to get it out of the icache and jumps to it
> in the unlikely case it is taken.  When all there is is a return, I
> don't think it even makes any difference.  In both these cases a
> comment
> is probably more appropriate.

That's not true on ppc where you can actually tell the processor about
the jump likelyhood for its branch prediction.

> On X86-32 at least the generated code is exactly the same with or
> without the unlikely()s.  I'll put them back in a follow-on if people
> are really attached to them.

Please keep them there, I think they make sense for normal operation.

johannes

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

Re: [PATCH 4/7] mac80211: make ieee80211_get_hdrlen_from_skb return unsigned

[Harvey Harrison]

On Mon, 2008-06-09 at 11:15 +0200, Johannes Berg wrote:
> > unlikely() isn't some magic make-me-faster function, it just moves
> > code
> > to the end of the function to get it out of the icache and jumps to it
> > in the unlikely case it is taken.  When all there is is a return, I
> > don't think it even makes any difference.  In both these cases a
> > comment
> > is probably more appropriate.
> 
> That's not true on ppc where you can actually tell the processor about
> the jump likelyhood for its branch prediction.

I was unaware of that.

> 
> > On X86-32 at least the generated code is exactly the same with or
> > without the unlikely()s.  I'll put them back in a follow-on if people
> > are really attached to them.
> 
> Please keep them there, I think they make sense for normal operation.

Kept, will be in a v2.

Harvey