* [PATCH 4/7] mac80211: make ieee80211_get_hdrlen_from_skb return unsigned
@ 2008-06-06 17:51 Harvey Harrison
2008-06-06 18:24 ` Michael Buesch
0 siblings, 1 reply; 7+ messages in thread
From: Harvey Harrison @ 2008-06-06 17:51 UTC (permalink / raw
To: Johannes Berg; +Cc: linux-wireless
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 4/7] mac80211: make ieee80211_get_hdrlen_from_skb return unsigned
2008-06-06 17:51 [PATCH 4/7] mac80211: make ieee80211_get_hdrlen_from_skb return unsigned Harvey Harrison
@ 2008-06-06 18:24 ` Michael Buesch
2008-06-06 18:47 ` Harvey Harrison
0 siblings, 1 reply; 7+ messages in thread
From: Michael Buesch @ 2008-06-06 18:24 UTC (permalink / raw
To: Harvey Harrison; +Cc: Johannes Berg, linux-wireless
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 4/7] mac80211: make ieee80211_get_hdrlen_from_skb return unsigned
2008-06-06 18:24 ` Michael Buesch
@ 2008-06-06 18:47 ` Harvey Harrison
2008-06-06 20:15 ` Pavel Roskin
0 siblings, 1 reply; 7+ messages in thread
From: Harvey Harrison @ 2008-06-06 18:47 UTC (permalink / raw
To: Michael Buesch; +Cc: Johannes Berg, linux-wireless
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 4/7] mac80211: make ieee80211_get_hdrlen_from_skb return unsigned
2008-06-06 18:47 ` Harvey Harrison
@ 2008-06-06 20:15 ` Pavel Roskin
2008-06-07 2:24 ` Harvey Harrison
0 siblings, 1 reply; 7+ messages in thread
From: Pavel Roskin @ 2008-06-06 20:15 UTC (permalink / raw
To: Harvey Harrison; +Cc: Michael Buesch, Johannes Berg, linux-wireless
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 4/7] mac80211: make ieee80211_get_hdrlen_from_skb return unsigned
2008-06-06 20:15 ` Pavel Roskin
@ 2008-06-07 2:24 ` Harvey Harrison
2008-06-09 9:15 ` Johannes Berg
0 siblings, 1 reply; 7+ messages in thread
From: Harvey Harrison @ 2008-06-07 2:24 UTC (permalink / raw
To: Pavel Roskin; +Cc: Michael Buesch, Johannes Berg, linux-wireless
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 4/7] mac80211: make ieee80211_get_hdrlen_from_skb return unsigned
2008-06-07 2:24 ` Harvey Harrison
@ 2008-06-09 9:15 ` Johannes Berg
2008-06-09 16:25 ` Harvey Harrison
0 siblings, 1 reply; 7+ messages in thread
From: Johannes Berg @ 2008-06-09 9:15 UTC (permalink / raw
To: Harvey Harrison; +Cc: Pavel Roskin, Michael Buesch, linux-wireless
[-- 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 --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 4/7] mac80211: make ieee80211_get_hdrlen_from_skb return unsigned
2008-06-09 9:15 ` Johannes Berg
@ 2008-06-09 16:25 ` Harvey Harrison
0 siblings, 0 replies; 7+ messages in thread
From: Harvey Harrison @ 2008-06-09 16:25 UTC (permalink / raw
To: Johannes Berg; +Cc: Pavel Roskin, Michael Buesch, linux-wireless
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2008-06-09 16:25 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-06-06 17:51 [PATCH 4/7] mac80211: make ieee80211_get_hdrlen_from_skb return unsigned Harvey Harrison
2008-06-06 18:24 ` Michael Buesch
2008-06-06 18:47 ` Harvey Harrison
2008-06-06 20:15 ` Pavel Roskin
2008-06-07 2:24 ` Harvey Harrison
2008-06-09 9:15 ` Johannes Berg
2008-06-09 16:25 ` Harvey Harrison
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.