> On 20. Apr 2023, at 15:00, Benjamin Beichler <Benjamin.Beichler@xxxxxxxxxxxxxx> wrote: > > Am 20.04.2023 um 14:22 schrieb Felix Fietkau: >>> On 20.04.23 12:27, Johannes Berg wrote: >>> On Thu, 2023-04-20 at 11:30 +0200, Benjamin Beichler wrote: >>>> > To me, basically, I see two ways to solve this: >>>> > >>>> > 1) we have DECLARE_EWMA_ZERO_VALID() or something like that which >>>> > *doesn't* treat 0 as an uninitialized value, and either has a >>>> > separate "not initialized yet" bit (but that's iffy storage wise), >>>> > or simply has another argument to _init() for the initial value or >>>> > so. >>>> > >>>> > 2) you don't just don't use 0 and 100 but say 1 and 100, that results in >>>> > basically the same behaviour, but avoids the special 0. >>>> > >>>> > johannes >>>> >>>> I also ran into that problem in the past, and reviewing it again with a >>>> college, I think, this is a real bug in the EWMA implementation. I try >>>> to provide a proper patch in the next days, but actually the EWMA >>>> handles the internal value zero, always like in the initialization, >>>> which is wrong, e.g., for positive/negative averaged values. >>> >>> Yes, it's always wrong as long as you feed it something zero, or values >>> with different sign. >>> >>> For a lot of use cases, however, that doesn't matter. Originally, it was >>> used e.g. for signal strength averaging, average packet lengths, etc. >>> where it really doesn't matter since you can never use 0 or values that >>> have different sign. >>> >>>> A quick research shows, this bug is since the first implementation of >>>> the ewma in the code ... >>>> >>> >>> Yeah, I'm aware of that, I was around for it ;-) >>> >>> But see above, I'm not sure I'd even call it a bug, at least not >>> originally with the users that we had intended. >>> >>> Hence I don't know if it's really good to fix this in general - for many >>> of these cases zero can still be treated specially (and like I mentioned >>> in my previous email, we can even here avoid 0), and then we don't spend >>> an extra byte (or likely 4) to hold a "first time" flag. >>> >>> Dunno. Maybe it's not worth thinking about the extra memory space vs. >>> the extra maintenance cost. But maybe at least on 64-bit we could steal >>> a bit from the unsigned long? Not sure what all the users are.. >> We don't actually need a full bit. We can just add 1 to the internal >> value for initialized values. How about this (completely untested): >> https://nbd.name/p/69b00c5b >> > Nice hack, but even a bit more dirty than before :-D I disagree. With my “hack” at least the implementation will do the right thing without the API user having to worry about 0 as a value. It could use some comments to clarify the details, but I do think it makes things easier. - Felix
