2007-12-23 12:19:54

by Mattias Nissler

[permalink] [raw]
Subject:

Hey,

I'm a little late on this and away from my development machine anyway, =
but when reading my email, I noticed the following and thought this is =
important enough to point out, even though I only have that crappy webm=
ailer and the only kernel tree I can look at is wireless-2.6/everything=
on git.kernel.org

In rc80211_pid.h, you have the following (this from the wireless-2.6 tr=
ee):

/* Sampling period for measuring percentage of failed frames. *=
/
#define RC_PID_INTERVAL (HZ / 8)

But rc80211_pid_algo.c says:

period =3D (HZ * pinfo->sampling_period + 500) / 1000;
if (!period)
period =3D 1;

You see this is completely bogus. My original patch had:

/* Sampling period for measuring percentage of failed frames in=
0.001s. */
#define RC_PID_INTERVAL 1000

You see why what you posted and John commited is bogus?

I'm getting a little fed up with this, you keep screwing up the code wi=
th my name on it *sigh*

And again, I still don't think guarding against period =3D=3D 0 is the =
right thing to do. As I explained privately in IRC, we should make sure=
the sample_interval is larger than HZ when setting the sample_interval=
variable (once nl80211 is done). It doesn't make sense to do this che=
ck every time we execute the code when we can do it once in advance.

Sigh.

Mattias
--=20
Der GMX SmartSurfer hilft bis zu 70% Ihrer Onlinekosten zu sparen!=20
Ideal f=FCr Modem und ISDN: http://www.gmx.net/de/go/smartsurfer


2007-12-23 12:45:38

by Stefano Brivio

[permalink] [raw]
Subject: Re:

On Sun, 23 Dec 2007 13:19:51 +0100
"Mattias Nissler" <[email protected]> wrote:

> Hey,
>
> I'm a little late on this and away from my development machine anyway, but
> when reading my email, I noticed the following and thought this is
> important enough to point out, even though I only have that crappy
> webmailer and the only kernel tree I can look at is wireless-2.6/everything
> on git.kernel.org
>
> In rc80211_pid.h, you have the following (this from the wireless-2.6 tree):
>
> /* Sampling period for measuring percentage of failed frames. */
> #define RC_PID_INTERVAL (HZ / 8)
>
> But rc80211_pid_algo.c says:
>
> period = (HZ * pinfo->sampling_period + 500) / 1000;
> if (!period)
> period = 1;
>
> You see this is completely bogus. My original patch had:
>
> /* Sampling period for measuring percentage of failed frames in 0.001s. */
> #define RC_PID_INTERVAL 1000
>
> You see why what you posted and John commited is bogus?

Indeed. I didn't notice about this because HZ == 1000 on my laptop. But I
didn't even sign off that patch, I just added a From: on top of it,
and changed this:

+#define RC_PID_INTERVAL (HZ / 1)
[yes, this is present in your original - as sent through IRC - patch]

to:

+#define RC_PID_INTERVAL (HZ / 8)
[and IIRC we discussed about this on IRC]

I'll post a fix for this.

> I'm getting a little fed up with this, you keep screwing up the code with
> my name on it *sigh*

Please, could you show _exactly_ which patches with your name (as in,
From: you) you think I screwed up?

I don't think I screwed up anything yours. I would be very sorry otherwise.

> And again, I still don't think guarding against period == 0 is the right
> thing to do. As I explained privately in IRC, we should make sure the
> sample_interval is larger than HZ when setting the sample_interval
> variable (once nl80211 is done). It doesn't make sense to do this check
> every time we execute the code when we can do it once in advance.

Exactly, once nl80211 is done. Now, what if the control interval gets
changed after initialization? How often should we check whether it's larger
than HZ? If you got another solution which would work together with the
current debugfs interface, please post it.

> Sigh.

Please stop this. I think that this is:
1) not correct (but I don't care that much);
2) useless;
3) confusing.

Sorry again if I actually screwed up anything.


--
Ciao
Stefano