* [RFC v2] average: rewrite for clearity
@ 2023-04-21 13:46 Benjamin Beichler
2023-04-21 14:37 ` Johannes Berg
` (3 more replies)
0 siblings, 4 replies; 8+ messages in thread
From: Benjamin Beichler @ 2023-04-21 13:46 UTC (permalink / raw)
To: nbd, johannes, linux-wireless; +Cc: Benjamin Beichler
Move the former *_add function with its implicit initialization into a
separate function, when the user explicitly wants to init with the first
added value, although this creates issues, when 0 is a expected value for
the internal value.
Add a separate init function with value parameter to allow init with
distinct value, which was formerly done by the implicit init of old
*_add function.
Move the compile time checks into a separate macro, as they are used
multiple times and noise up the functions.
Signed-off-by: Benjamin Beichler <benjamin.beichler@uni-rostock.de>
---
include/linux/average.h | 86 ++++++++++++++++++++++++-----------------
1 file changed, 50 insertions(+), 36 deletions(-)
diff --git a/include/linux/average.h b/include/linux/average.h
index a1a8f09631ce..7149a9ee555a 100644
--- a/include/linux/average.h
+++ b/include/linux/average.h
@@ -25,47 +25,61 @@
* that this parameter must be a power of two for efficiency.
*/
-#define DECLARE_EWMA(name, _precision, _weight_rcp) \
- struct ewma_##name { \
- unsigned long internal; \
- }; \
- static inline void ewma_##name##_init(struct ewma_##name *e) \
- { \
+#define EWMA_BUILD_TIME_CHECKS(_precision, _weight_rcp) \
+ do { \
BUILD_BUG_ON(!__builtin_constant_p(_precision)); \
BUILD_BUG_ON(!__builtin_constant_p(_weight_rcp)); \
- /* \
- * Even if you want to feed it just 0/1 you should have \
- * some bits for the non-fractional part... \
- */ \
- BUILD_BUG_ON((_precision) > 30); \
- BUILD_BUG_ON_NOT_POWER_OF_2(_weight_rcp); \
- e->internal = 0; \
- } \
- static inline unsigned long \
- ewma_##name##_read(struct ewma_##name *e) \
- { \
- BUILD_BUG_ON(!__builtin_constant_p(_precision)); \
- BUILD_BUG_ON(!__builtin_constant_p(_weight_rcp)); \
- BUILD_BUG_ON((_precision) > 30); \
- BUILD_BUG_ON_NOT_POWER_OF_2(_weight_rcp); \
- return e->internal >> (_precision); \
- } \
- static inline void ewma_##name##_add(struct ewma_##name *e, \
- unsigned long val) \
- { \
- unsigned long internal = READ_ONCE(e->internal); \
- unsigned long weight_rcp = ilog2(_weight_rcp); \
- unsigned long precision = _precision; \
\
- BUILD_BUG_ON(!__builtin_constant_p(_precision)); \
- BUILD_BUG_ON(!__builtin_constant_p(_weight_rcp)); \
BUILD_BUG_ON((_precision) > 30); \
BUILD_BUG_ON_NOT_POWER_OF_2(_weight_rcp); \
- \
- WRITE_ONCE(e->internal, internal ? \
- (((internal << weight_rcp) - internal) + \
- (val << precision)) >> weight_rcp : \
- (val << precision)); \
+ } while (0)
+
+#define DECLARE_EWMA(name, _precision, _weight_rcp) \
+ struct ewma_##name { \
+ unsigned long internal; \
+ }; \
+ static inline void ewma_##name##_init_val(struct ewma_##name *e, \
+ unsigned long init) \
+ { \
+ EWMA_BUILD_TIME_CHECKS(_precision, _weight_rcp) \
+ e->internal = init << _precision; \
+ } \
+ static inline void ewma_##name##_init(struct ewma_##name *e) \
+ { \
+ ewma_##name##_init_val(e, 0); \
+ } \
+ static inline unsigned long \
+ ewma_##name##_read(struct ewma_##name *e) \
+ { \
+ EWMA_BUILD_TIME_CHECKS(_precision, _weight_rcp) \
+ return e->internal >> (_precision); \
+ } \
+ static inline void ewma_##name##_add(struct ewma_##name *e, \
+ unsigned long val) \
+ { \
+ unsigned long internal = READ_ONCE(e->internal); \
+ unsigned long weight_rcp = ilog2(_weight_rcp); \
+ unsigned long precision = _precision; \
+ \
+ EWMA_BUILD_TIME_CHECKS(_precision, _weight_rcp) \
+ \
+ WRITE_ONCE(e->internal, \
+ (((internal << weight_rcp) - internal) + \
+ (val << precision)) >> weight_rcp); \
+ } \
+ static inline void ewma_##name##_add_or_init(struct ewma_##name *e, \
+ unsigned long val) \
+ { \
+ unsigned long internal = READ_ONCE(e->internal); \
+ unsigned long weight_rcp = ilog2(_weight_rcp); \
+ unsigned long precision = _precision; \
+ \
+ EWMA_BUILD_TIME_CHECKS(_precision, _weight_rcp) \
+ \
+ WRITE_ONCE(e->internal, internal ? \
+ (((internal << weight_rcp) - internal) + \
+ (val << precision)) >> weight_rcp : \
+ (val << precision)); \
}
#endif /* _LINUX_AVERAGE_H */
--
2.25.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [RFC v2] average: rewrite for clearity
2023-04-21 13:46 [RFC v2] average: rewrite for clearity Benjamin Beichler
@ 2023-04-21 14:37 ` Johannes Berg
2023-04-21 15:16 ` Benjamin Beichler
2023-04-21 18:41 ` kernel test robot
` (2 subsequent siblings)
3 siblings, 1 reply; 8+ messages in thread
From: Johannes Berg @ 2023-04-21 14:37 UTC (permalink / raw)
To: Benjamin Beichler, nbd, linux-wireless
On Fri, 2023-04-21 at 13:46 +0000, Benjamin Beichler wrote:
> Move the former *_add function with its implicit initialization into a
> separate function, when the user explicitly wants to init with the first
> added value, although this creates issues, when 0 is a expected value for
> the internal value.
>
> Add a separate init function with value parameter to allow init with
> distinct value, which was formerly done by the implicit init of old
> *_add function.
>
Sure, this is what you said :-)
I still don't really think it's feasible. First, this breaks all the
users, because if you have e.g. virtio's DECLARE_EWMA(pkt_len, 0, 64)
then it will take a long time to get away from 0.
So then we could say we'll just fix them, but what value actually makes
sense to init with? You don't necessarily know, right? Initially biasing
towards the first value makes a lot more sense than initially biasing
towards zero, no? And then if you want to achieve that, now you have to
either use _add_or_init(), which is probably what people will do, but
that continues having the problem ...
I don't really see how this is a net improvement - we'd still have to
fix the individual users with it, e.g. maybe the mesh case that started
this investigation could bias towards success (init with 100) and then
use _add() rather than _add_or_init(), but then again we're back to
having to make individual choices with the users. I don't really see how
that's better than fixing it for real with the existing behaviour of
biasing towards the first value?
johannes
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC v2] average: rewrite for clearity
2023-04-21 14:37 ` Johannes Berg
@ 2023-04-21 15:16 ` Benjamin Beichler
2023-04-24 15:55 ` Johannes Berg
0 siblings, 1 reply; 8+ messages in thread
From: Benjamin Beichler @ 2023-04-21 15:16 UTC (permalink / raw)
To: Johannes Berg, nbd, linux-wireless
Am 21.04.2023 um 16:37 schrieb Johannes Berg:
> ________________________________
> Achtung! Externe E-Mail: Klicken Sie erst dann auf Links und Anhänge, nachdem Sie die Vertrauenswürdigkeit der Absenderadresse geprüft haben.
> ________________________________
>
> On Fri, 2023-04-21 at 13:46 +0000, Benjamin Beichler wrote:
>> Move the former *_add function with its implicit initialization into a
>> separate function, when the user explicitly wants to init with the first
>> added value, although this creates issues, when 0 is a expected value for
>> the internal value.
>>
>> Add a separate init function with value parameter to allow init with
>> distinct value, which was formerly done by the implicit init of old
>> *_add function.
>>
> Sure, this is what you said :-)
>
> I still don't really think it's feasible. First, this breaks all the
> users, because if you have e.g. virtio's DECLARE_EWMA(pkt_len, 0, 64)
> then it will take a long time to get away from 0.
>
> So then we could say we'll just fix them, but what value actually makes
> sense to init with? You don't necessarily know, right? Initially biasing
> towards the first value makes a lot more sense than initially biasing
> towards zero, no? And then if you want to achieve that, now you have to
> either use _add_or_init(), which is probably what people will do, but
> that continues having the problem ...
I left out the the individual fixes for users, but for the samples I
looked into, either start values were given, or they were semantically
extractable.
e.g. virtios pkt_len ewma is clamped anyways, so using the clamp border
or in between will be safe.
Most others are signals-strengths, many of them also only for
statistics, where a slow convergence is okay in my opinion.
> I don't really see how this is a net improvement - we'd still have to
> fix the individual users with it, e.g. maybe the mesh case that started
> this investigation could bias towards success (init with 100) and then
> use _add() rather than _add_or_init(), but then again we're back to
> having to make individual choices with the users. I don't really see how
> that's better than fixing it for real with the existing behaviour of
> biasing towards the first value?
IMHO the net improvement is, that if you do not use the convenience
add_or_init function, it simply resembles the ewma filter of a math or
CS-textbook. The original problem was, that the ewma had a surprising
output in a special situation.
But while writing the commit, I recognized, that the current ewma
implementation is only for unsigned values, which means the problem
really only happens for many consecutive 0 values. I try to think over,
what this means for the proposal of Felix, but I'm not able to think
about unsigned C-arithmetics today :-D
If we use that fix, then we should have an additional compile time
check, that the precision has at least 2 or 4 bits, to really avoid the
problem, shouldn't we?
Benjamin
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC v2] average: rewrite for clearity
2023-04-21 13:46 [RFC v2] average: rewrite for clearity Benjamin Beichler
2023-04-21 14:37 ` Johannes Berg
@ 2023-04-21 18:41 ` kernel test robot
2023-04-21 19:53 ` kernel test robot
2023-04-21 19:53 ` kernel test robot
3 siblings, 0 replies; 8+ messages in thread
From: kernel test robot @ 2023-04-21 18:41 UTC (permalink / raw)
To: Benjamin Beichler; +Cc: oe-kbuild-all
Hi Benjamin,
[This is a private test report for your RFC patch.]
kernel test robot noticed the following build errors:
[auto build test ERROR on wireless-next/main]
[also build test ERROR on wireless/main linus/master v6.3-rc7 next-20230420]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Benjamin-Beichler/average-rewrite-for-clearity/20230421-214746
base: https://git.kernel.org/pub/scm/linux/kernel/git/wireless/wireless-next.git main
patch link: https://lore.kernel.org/r/20230421134604.211128-1-benjamin.beichler%40uni-rostock.de
patch subject: [RFC v2] average: rewrite for clearity
config: sh-allmodconfig (https://download.01.org/0day-ci/archive/20230422/202304220245.ctq7CKFg-lkp@intel.com/config)
compiler: sh4-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/f61e21185a0ffa11e3d84ed0b6159882a921347e
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Benjamin-Beichler/average-rewrite-for-clearity/20230421-214746
git checkout f61e21185a0ffa11e3d84ed0b6159882a921347e
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sh olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sh SHELL=/bin/bash drivers/net/wireless/realtek/rtw88/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304220245.ctq7CKFg-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from drivers/net/wireless/realtek/rtw88/main.h:11,
from drivers/net/wireless/realtek/rtw88/main.c:7:
drivers/net/wireless/realtek/rtw88/main.h: In function 'ewma_tp_init_val':
>> include/linux/average.h:45:17: error: expected ';' before 'e'
45 | e->internal = init << _precision; \
| ^
drivers/net/wireless/realtek/rtw88/main.h:647:1: note: in expansion of macro 'DECLARE_EWMA'
647 | DECLARE_EWMA(tp, 10, 2);
| ^~~~~~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h: In function 'ewma_tp_read':
>> include/linux/average.h:55:17: error: expected ';' before 'return'
55 | return e->internal >> (_precision); \
| ^~~~~~
drivers/net/wireless/realtek/rtw88/main.h:647:1: note: in expansion of macro 'DECLARE_EWMA'
647 | DECLARE_EWMA(tp, 10, 2);
| ^~~~~~~~~~~~
In file included from ./arch/sh/include/generated/asm/rwonce.h:1,
from include/linux/compiler.h:247,
from include/linux/dev_printk.h:14,
from include/linux/device.h:15,
from include/linux/devcoredump.h:8,
from drivers/net/wireless/realtek/rtw88/main.c:5:
drivers/net/wireless/realtek/rtw88/main.h: In function 'ewma_tp_add':
>> include/asm-generic/rwonce.h:59:1: error: expected ';' before 'do'
59 | do { \
| ^~
include/linux/average.h:66:17: note: in expansion of macro 'WRITE_ONCE'
66 | WRITE_ONCE(e->internal, \
| ^~~~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h:647:1: note: in expansion of macro 'DECLARE_EWMA'
647 | DECLARE_EWMA(tp, 10, 2);
| ^~~~~~~~~~~~
include/linux/average.h:62:31: warning: unused variable 'precision' [-Wunused-variable]
62 | unsigned long precision = _precision; \
| ^~~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h:647:1: note: in expansion of macro 'DECLARE_EWMA'
647 | DECLARE_EWMA(tp, 10, 2);
| ^~~~~~~~~~~~
include/linux/average.h:61:31: warning: unused variable 'weight_rcp' [-Wunused-variable]
61 | unsigned long weight_rcp = ilog2(_weight_rcp); \
| ^~~~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h:647:1: note: in expansion of macro 'DECLARE_EWMA'
647 | DECLARE_EWMA(tp, 10, 2);
| ^~~~~~~~~~~~
include/linux/average.h:60:31: warning: unused variable 'internal' [-Wunused-variable]
60 | unsigned long internal = READ_ONCE(e->internal); \
| ^~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h:647:1: note: in expansion of macro 'DECLARE_EWMA'
647 | DECLARE_EWMA(tp, 10, 2);
| ^~~~~~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h: In function 'ewma_tp_add_or_init':
>> include/asm-generic/rwonce.h:59:1: error: expected ';' before 'do'
59 | do { \
| ^~
include/linux/average.h:79:17: note: in expansion of macro 'WRITE_ONCE'
79 | WRITE_ONCE(e->internal, internal ? \
| ^~~~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h:647:1: note: in expansion of macro 'DECLARE_EWMA'
647 | DECLARE_EWMA(tp, 10, 2);
| ^~~~~~~~~~~~
include/linux/average.h:75:31: warning: unused variable 'precision' [-Wunused-variable]
75 | unsigned long precision = _precision; \
| ^~~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h:647:1: note: in expansion of macro 'DECLARE_EWMA'
647 | DECLARE_EWMA(tp, 10, 2);
| ^~~~~~~~~~~~
include/linux/average.h:74:31: warning: unused variable 'weight_rcp' [-Wunused-variable]
74 | unsigned long weight_rcp = ilog2(_weight_rcp); \
| ^~~~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h:647:1: note: in expansion of macro 'DECLARE_EWMA'
647 | DECLARE_EWMA(tp, 10, 2);
| ^~~~~~~~~~~~
include/linux/average.h:73:31: warning: unused variable 'internal' [-Wunused-variable]
73 | unsigned long internal = READ_ONCE(e->internal); \
| ^~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h:647:1: note: in expansion of macro 'DECLARE_EWMA'
647 | DECLARE_EWMA(tp, 10, 2);
| ^~~~~~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h: In function 'ewma_rssi_init_val':
>> include/linux/average.h:45:17: error: expected ';' before 'e'
45 | e->internal = init << _precision; \
| ^
drivers/net/wireless/realtek/rtw88/main.h:743:1: note: in expansion of macro 'DECLARE_EWMA'
743 | DECLARE_EWMA(rssi, 10, 16);
| ^~~~~~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h: In function 'ewma_rssi_read':
>> include/linux/average.h:55:17: error: expected ';' before 'return'
55 | return e->internal >> (_precision); \
| ^~~~~~
drivers/net/wireless/realtek/rtw88/main.h:743:1: note: in expansion of macro 'DECLARE_EWMA'
743 | DECLARE_EWMA(rssi, 10, 16);
| ^~~~~~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h: In function 'ewma_rssi_add':
>> include/asm-generic/rwonce.h:59:1: error: expected ';' before 'do'
59 | do { \
| ^~
include/linux/average.h:66:17: note: in expansion of macro 'WRITE_ONCE'
66 | WRITE_ONCE(e->internal, \
| ^~~~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h:743:1: note: in expansion of macro 'DECLARE_EWMA'
743 | DECLARE_EWMA(rssi, 10, 16);
| ^~~~~~~~~~~~
include/linux/average.h:62:31: warning: unused variable 'precision' [-Wunused-variable]
62 | unsigned long precision = _precision; \
| ^~~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h:743:1: note: in expansion of macro 'DECLARE_EWMA'
743 | DECLARE_EWMA(rssi, 10, 16);
| ^~~~~~~~~~~~
include/linux/average.h:61:31: warning: unused variable 'weight_rcp' [-Wunused-variable]
61 | unsigned long weight_rcp = ilog2(_weight_rcp); \
| ^~~~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h:743:1: note: in expansion of macro 'DECLARE_EWMA'
743 | DECLARE_EWMA(rssi, 10, 16);
| ^~~~~~~~~~~~
include/linux/average.h:60:31: warning: unused variable 'internal' [-Wunused-variable]
60 | unsigned long internal = READ_ONCE(e->internal); \
| ^~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h:743:1: note: in expansion of macro 'DECLARE_EWMA'
743 | DECLARE_EWMA(rssi, 10, 16);
| ^~~~~~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h: In function 'ewma_rssi_add_or_init':
>> include/asm-generic/rwonce.h:59:1: error: expected ';' before 'do'
59 | do { \
| ^~
include/linux/average.h:79:17: note: in expansion of macro 'WRITE_ONCE'
79 | WRITE_ONCE(e->internal, internal ? \
| ^~~~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h:743:1: note: in expansion of macro 'DECLARE_EWMA'
743 | DECLARE_EWMA(rssi, 10, 16);
| ^~~~~~~~~~~~
include/linux/average.h:75:31: warning: unused variable 'precision' [-Wunused-variable]
75 | unsigned long precision = _precision; \
| ^~~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h:743:1: note: in expansion of macro 'DECLARE_EWMA'
743 | DECLARE_EWMA(rssi, 10, 16);
| ^~~~~~~~~~~~
include/linux/average.h:74:31: warning: unused variable 'weight_rcp' [-Wunused-variable]
74 | unsigned long weight_rcp = ilog2(_weight_rcp); \
| ^~~~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h:743:1: note: in expansion of macro 'DECLARE_EWMA'
743 | DECLARE_EWMA(rssi, 10, 16);
| ^~~~~~~~~~~~
include/linux/average.h:73:31: warning: unused variable 'internal' [-Wunused-variable]
73 | unsigned long internal = READ_ONCE(e->internal); \
| ^~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h:743:1: note: in expansion of macro 'DECLARE_EWMA'
743 | DECLARE_EWMA(rssi, 10, 16);
| ^~~~~~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h: In function 'ewma_thermal_init_val':
>> include/linux/average.h:45:17: error: expected ';' before 'e'
45 | e->internal = init << _precision; \
| ^
drivers/net/wireless/realtek/rtw88/main.h:1548:1: note: in expansion of macro 'DECLARE_EWMA'
1548 | DECLARE_EWMA(thermal, 10, 4);
| ^~~~~~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h: In function 'ewma_thermal_read':
>> include/linux/average.h:55:17: error: expected ';' before 'return'
55 | return e->internal >> (_precision); \
| ^~~~~~
drivers/net/wireless/realtek/rtw88/main.h:1548:1: note: in expansion of macro 'DECLARE_EWMA'
1548 | DECLARE_EWMA(thermal, 10, 4);
| ^~~~~~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h: In function 'ewma_thermal_add':
>> include/asm-generic/rwonce.h:59:1: error: expected ';' before 'do'
59 | do { \
| ^~
include/linux/average.h:66:17: note: in expansion of macro 'WRITE_ONCE'
66 | WRITE_ONCE(e->internal, \
| ^~~~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h:1548:1: note: in expansion of macro 'DECLARE_EWMA'
1548 | DECLARE_EWMA(thermal, 10, 4);
| ^~~~~~~~~~~~
include/linux/average.h:62:31: warning: unused variable 'precision' [-Wunused-variable]
62 | unsigned long precision = _precision; \
| ^~~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h:1548:1: note: in expansion of macro 'DECLARE_EWMA'
1548 | DECLARE_EWMA(thermal, 10, 4);
| ^~~~~~~~~~~~
include/linux/average.h:61:31: warning: unused variable 'weight_rcp' [-Wunused-variable]
61 | unsigned long weight_rcp = ilog2(_weight_rcp); \
| ^~~~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h:1548:1: note: in expansion of macro 'DECLARE_EWMA'
1548 | DECLARE_EWMA(thermal, 10, 4);
| ^~~~~~~~~~~~
include/linux/average.h:60:31: warning: unused variable 'internal' [-Wunused-variable]
60 | unsigned long internal = READ_ONCE(e->internal); \
| ^~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h:1548:1: note: in expansion of macro 'DECLARE_EWMA'
1548 | DECLARE_EWMA(thermal, 10, 4);
| ^~~~~~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h: In function 'ewma_thermal_add_or_init':
>> include/asm-generic/rwonce.h:59:1: error: expected ';' before 'do'
59 | do { \
| ^~
include/linux/average.h:79:17: note: in expansion of macro 'WRITE_ONCE'
79 | WRITE_ONCE(e->internal, internal ? \
| ^~~~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h:1548:1: note: in expansion of macro 'DECLARE_EWMA'
1548 | DECLARE_EWMA(thermal, 10, 4);
| ^~~~~~~~~~~~
include/linux/average.h:75:31: warning: unused variable 'precision' [-Wunused-variable]
75 | unsigned long precision = _precision; \
| ^~~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h:1548:1: note: in expansion of macro 'DECLARE_EWMA'
1548 | DECLARE_EWMA(thermal, 10, 4);
| ^~~~~~~~~~~~
include/linux/average.h:74:31: warning: unused variable 'weight_rcp' [-Wunused-variable]
74 | unsigned long weight_rcp = ilog2(_weight_rcp); \
| ^~~~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h:1548:1: note: in expansion of macro 'DECLARE_EWMA'
1548 | DECLARE_EWMA(thermal, 10, 4);
| ^~~~~~~~~~~~
include/linux/average.h:73:31: warning: unused variable 'internal' [-Wunused-variable]
73 | unsigned long internal = READ_ONCE(e->internal); \
| ^~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h:1548:1: note: in expansion of macro 'DECLARE_EWMA'
1548 | DECLARE_EWMA(thermal, 10, 4);
| ^~~~~~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h: In function 'ewma_evm_init_val':
>> include/linux/average.h:45:17: error: expected ';' before 'e'
45 | e->internal = init << _precision; \
| ^
drivers/net/wireless/realtek/rtw88/main.h:1594:1: note: in expansion of macro 'DECLARE_EWMA'
1594 | DECLARE_EWMA(evm, 10, 4);
| ^~~~~~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h: In function 'ewma_evm_read':
>> include/linux/average.h:55:17: error: expected ';' before 'return'
55 | return e->internal >> (_precision); \
| ^~~~~~
drivers/net/wireless/realtek/rtw88/main.h:1594:1: note: in expansion of macro 'DECLARE_EWMA'
1594 | DECLARE_EWMA(evm, 10, 4);
| ^~~~~~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h: In function 'ewma_evm_add':
>> include/asm-generic/rwonce.h:59:1: error: expected ';' before 'do'
59 | do { \
| ^~
include/linux/average.h:66:17: note: in expansion of macro 'WRITE_ONCE'
66 | WRITE_ONCE(e->internal, \
| ^~~~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h:1594:1: note: in expansion of macro 'DECLARE_EWMA'
1594 | DECLARE_EWMA(evm, 10, 4);
| ^~~~~~~~~~~~
include/linux/average.h:62:31: warning: unused variable 'precision' [-Wunused-variable]
62 | unsigned long precision = _precision; \
| ^~~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h:1594:1: note: in expansion of macro 'DECLARE_EWMA'
1594 | DECLARE_EWMA(evm, 10, 4);
| ^~~~~~~~~~~~
include/linux/average.h:61:31: warning: unused variable 'weight_rcp' [-Wunused-variable]
61 | unsigned long weight_rcp = ilog2(_weight_rcp); \
| ^~~~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h:1594:1: note: in expansion of macro 'DECLARE_EWMA'
1594 | DECLARE_EWMA(evm, 10, 4);
| ^~~~~~~~~~~~
include/linux/average.h:60:31: warning: unused variable 'internal' [-Wunused-variable]
60 | unsigned long internal = READ_ONCE(e->internal); \
| ^~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h:1594:1: note: in expansion of macro 'DECLARE_EWMA'
1594 | DECLARE_EWMA(evm, 10, 4);
| ^~~~~~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h: In function 'ewma_evm_add_or_init':
>> include/asm-generic/rwonce.h:59:1: error: expected ';' before 'do'
59 | do { \
| ^~
include/linux/average.h:79:17: note: in expansion of macro 'WRITE_ONCE'
79 | WRITE_ONCE(e->internal, internal ? \
| ^~~~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h:1594:1: note: in expansion of macro 'DECLARE_EWMA'
1594 | DECLARE_EWMA(evm, 10, 4);
| ^~~~~~~~~~~~
include/linux/average.h:75:31: warning: unused variable 'precision' [-Wunused-variable]
75 | unsigned long precision = _precision; \
| ^~~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h:1594:1: note: in expansion of macro 'DECLARE_EWMA'
1594 | DECLARE_EWMA(evm, 10, 4);
| ^~~~~~~~~~~~
include/linux/average.h:74:31: warning: unused variable 'weight_rcp' [-Wunused-variable]
74 | unsigned long weight_rcp = ilog2(_weight_rcp); \
| ^~~~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h:1594:1: note: in expansion of macro 'DECLARE_EWMA'
1594 | DECLARE_EWMA(evm, 10, 4);
| ^~~~~~~~~~~~
include/linux/average.h:73:31: warning: unused variable 'internal' [-Wunused-variable]
73 | unsigned long internal = READ_ONCE(e->internal); \
| ^~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h:1594:1: note: in expansion of macro 'DECLARE_EWMA'
1594 | DECLARE_EWMA(evm, 10, 4);
| ^~~~~~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h: In function 'ewma_snr_init_val':
>> include/linux/average.h:45:17: error: expected ';' before 'e'
45 | e->internal = init << _precision; \
| ^
drivers/net/wireless/realtek/rtw88/main.h:1595:1: note: in expansion of macro 'DECLARE_EWMA'
1595 | DECLARE_EWMA(snr, 10, 4);
| ^~~~~~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h: In function 'ewma_snr_read':
>> include/linux/average.h:55:17: error: expected ';' before 'return'
55 | return e->internal >> (_precision); \
| ^~~~~~
drivers/net/wireless/realtek/rtw88/main.h:1595:1: note: in expansion of macro 'DECLARE_EWMA'
1595 | DECLARE_EWMA(snr, 10, 4);
| ^~~~~~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h: In function 'ewma_snr_add':
>> include/asm-generic/rwonce.h:59:1: error: expected ';' before 'do'
59 | do { \
| ^~
include/linux/average.h:66:17: note: in expansion of macro 'WRITE_ONCE'
66 | WRITE_ONCE(e->internal, \
| ^~~~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h:1595:1: note: in expansion of macro 'DECLARE_EWMA'
1595 | DECLARE_EWMA(snr, 10, 4);
| ^~~~~~~~~~~~
include/linux/average.h:62:31: warning: unused variable 'precision' [-Wunused-variable]
62 | unsigned long precision = _precision; \
| ^~~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h:1595:1: note: in expansion of macro 'DECLARE_EWMA'
1595 | DECLARE_EWMA(snr, 10, 4);
| ^~~~~~~~~~~~
include/linux/average.h:61:31: warning: unused variable 'weight_rcp' [-Wunused-variable]
61 | unsigned long weight_rcp = ilog2(_weight_rcp); \
| ^~~~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h:1595:1: note: in expansion of macro 'DECLARE_EWMA'
1595 | DECLARE_EWMA(snr, 10, 4);
| ^~~~~~~~~~~~
include/linux/average.h:60:31: warning: unused variable 'internal' [-Wunused-variable]
60 | unsigned long internal = READ_ONCE(e->internal); \
| ^~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h:1595:1: note: in expansion of macro 'DECLARE_EWMA'
1595 | DECLARE_EWMA(snr, 10, 4);
| ^~~~~~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h: In function 'ewma_snr_add_or_init':
>> include/asm-generic/rwonce.h:59:1: error: expected ';' before 'do'
59 | do { \
| ^~
include/linux/average.h:79:17: note: in expansion of macro 'WRITE_ONCE'
79 | WRITE_ONCE(e->internal, internal ? \
| ^~~~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h:1595:1: note: in expansion of macro 'DECLARE_EWMA'
1595 | DECLARE_EWMA(snr, 10, 4);
| ^~~~~~~~~~~~
include/linux/average.h:75:31: warning: unused variable 'precision' [-Wunused-variable]
75 | unsigned long precision = _precision; \
| ^~~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h:1595:1: note: in expansion of macro 'DECLARE_EWMA'
1595 | DECLARE_EWMA(snr, 10, 4);
| ^~~~~~~~~~~~
include/linux/average.h:74:31: warning: unused variable 'weight_rcp' [-Wunused-variable]
74 | unsigned long weight_rcp = ilog2(_weight_rcp); \
| ^~~~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h:1595:1: note: in expansion of macro 'DECLARE_EWMA'
1595 | DECLARE_EWMA(snr, 10, 4);
| ^~~~~~~~~~~~
include/linux/average.h:73:31: warning: unused variable 'internal' [-Wunused-variable]
73 | unsigned long internal = READ_ONCE(e->internal); \
| ^~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h:1595:1: note: in expansion of macro 'DECLARE_EWMA'
1595 | DECLARE_EWMA(snr, 10, 4);
| ^~~~~~~~~~~~
drivers/net/wireless/realtek/rtw88/main.h: In function 'ewma_tp_read':
include/linux/average.h:56:9: error: control reaches end of non-void function [-Werror=return-type]
56 | } \
| ^
drivers/net/wireless/realtek/rtw88/main.h:647:1: note: in expansion of macro 'DECLARE_EWMA'
647 | DECLARE_EWMA(tp, 10, 2);
| ^~~~~~~~~~~~
cc1: some warnings being treated as errors
..
vim +45 include/linux/average.h
8
9 /*
10 * Exponentially weighted moving average (EWMA)
11 *
12 * This implements a fixed-precision EWMA algorithm, with both the
13 * precision and fall-off coefficient determined at compile-time
14 * and built into the generated helper funtions.
15 *
16 * The first argument to the macro is the name that will be used
17 * for the struct and helper functions.
18 *
19 * The second argument, the precision, expresses how many bits are
20 * used for the fractional part of the fixed-precision values.
21 *
22 * The third argument, the weight reciprocal, determines how the
23 * new values will be weighed vs. the old state, new values will
24 * get weight 1/weight_rcp and old values 1-1/weight_rcp. Note
25 * that this parameter must be a power of two for efficiency.
26 */
27
28 #define EWMA_BUILD_TIME_CHECKS(_precision, _weight_rcp) \
29 do { \
30 BUILD_BUG_ON(!__builtin_constant_p(_precision)); \
31 BUILD_BUG_ON(!__builtin_constant_p(_weight_rcp)); \
32 \
33 BUILD_BUG_ON((_precision) > 30); \
34 BUILD_BUG_ON_NOT_POWER_OF_2(_weight_rcp); \
35 } while (0)
36
37 #define DECLARE_EWMA(name, _precision, _weight_rcp) \
38 struct ewma_##name { \
39 unsigned long internal; \
40 }; \
41 static inline void ewma_##name##_init_val(struct ewma_##name *e, \
42 unsigned long init) \
43 { \
44 EWMA_BUILD_TIME_CHECKS(_precision, _weight_rcp) \
> 45 e->internal = init << _precision; \
46 } \
47 static inline void ewma_##name##_init(struct ewma_##name *e) \
48 { \
49 ewma_##name##_init_val(e, 0); \
50 } \
51 static inline unsigned long \
52 ewma_##name##_read(struct ewma_##name *e) \
53 { \
54 EWMA_BUILD_TIME_CHECKS(_precision, _weight_rcp) \
> 55 return e->internal >> (_precision); \
56 } \
57 static inline void ewma_##name##_add(struct ewma_##name *e, \
58 unsigned long val) \
59 { \
60 unsigned long internal = READ_ONCE(e->internal); \
61 unsigned long weight_rcp = ilog2(_weight_rcp); \
62 unsigned long precision = _precision; \
63 \
64 EWMA_BUILD_TIME_CHECKS(_precision, _weight_rcp) \
65 \
66 WRITE_ONCE(e->internal, \
67 (((internal << weight_rcp) - internal) + \
68 (val << precision)) >> weight_rcp); \
69 } \
70 static inline void ewma_##name##_add_or_init(struct ewma_##name *e, \
71 unsigned long val) \
72 { \
73 unsigned long internal = READ_ONCE(e->internal); \
74 unsigned long weight_rcp = ilog2(_weight_rcp); \
75 unsigned long precision = _precision; \
76 \
77 EWMA_BUILD_TIME_CHECKS(_precision, _weight_rcp) \
78 \
79 WRITE_ONCE(e->internal, internal ? \
80 (((internal << weight_rcp) - internal) + \
81 (val << precision)) >> weight_rcp : \
82 (val << precision)); \
83 }
84
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC v2] average: rewrite for clearity
2023-04-21 13:46 [RFC v2] average: rewrite for clearity Benjamin Beichler
2023-04-21 14:37 ` Johannes Berg
2023-04-21 18:41 ` kernel test robot
@ 2023-04-21 19:53 ` kernel test robot
2023-04-21 19:53 ` kernel test robot
3 siblings, 0 replies; 8+ messages in thread
From: kernel test robot @ 2023-04-21 19:53 UTC (permalink / raw)
To: Benjamin Beichler; +Cc: oe-kbuild-all
Hi Benjamin,
[This is a private test report for your RFC patch.]
kernel test robot noticed the following build errors:
[auto build test ERROR on wireless-next/main]
[also build test ERROR on wireless/main linus/master v6.3-rc7 next-20230420]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Benjamin-Beichler/average-rewrite-for-clearity/20230421-214746
base: https://git.kernel.org/pub/scm/linux/kernel/git/wireless/wireless-next.git main
patch link: https://lore.kernel.org/r/20230421134604.211128-1-benjamin.beichler%40uni-rostock.de
patch subject: [RFC v2] average: rewrite for clearity
config: i386-randconfig-a001 (https://download.01.org/0day-ci/archive/20230422/202304220335.I8vXhd1J-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/f61e21185a0ffa11e3d84ed0b6159882a921347e
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Benjamin-Beichler/average-rewrite-for-clearity/20230421-214746
git checkout f61e21185a0ffa11e3d84ed0b6159882a921347e
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=i386 olddefconfig
make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/cxl/core/ drivers/net/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304220335.I8vXhd1J-lkp@intel.com/
All error/warnings (new ones prefixed by >>):
In file included from drivers/net/virtio_net.c:19:
drivers/net/virtio_net.c: In function 'ewma_pkt_len_init_val':
>> include/linux/average.h:45:17: error: expected ';' before 'e'
45 | e->internal = init << _precision; \
| ^
drivers/net/virtio_net.c:54:1: note: in expansion of macro 'DECLARE_EWMA'
54 | DECLARE_EWMA(pkt_len, 0, 64)
| ^~~~~~~~~~~~
drivers/net/virtio_net.c: In function 'ewma_pkt_len_read':
>> include/linux/average.h:55:17: error: expected ';' before 'return'
55 | return e->internal >> (_precision); \
| ^~~~~~
drivers/net/virtio_net.c:54:1: note: in expansion of macro 'DECLARE_EWMA'
54 | DECLARE_EWMA(pkt_len, 0, 64)
| ^~~~~~~~~~~~
In file included from ./arch/x86/include/generated/asm/rwonce.h:1,
from include/linux/compiler.h:247,
from include/linux/build_bug.h:5,
from include/linux/container_of.h:5,
from include/linux/list.h:5,
from include/linux/timer.h:5,
from include/linux/netdevice.h:24,
from drivers/net/virtio_net.c:7:
drivers/net/virtio_net.c: In function 'ewma_pkt_len_add':
>> include/asm-generic/rwonce.h:59:1: error: expected ';' before 'do'
59 | do { \
| ^~
include/linux/average.h:66:17: note: in expansion of macro 'WRITE_ONCE'
66 | WRITE_ONCE(e->internal, \
| ^~~~~~~~~~
drivers/net/virtio_net.c:54:1: note: in expansion of macro 'DECLARE_EWMA'
54 | DECLARE_EWMA(pkt_len, 0, 64)
| ^~~~~~~~~~~~
In file included from drivers/net/virtio_net.c:19:
>> include/linux/average.h:62:31: warning: unused variable 'precision' [-Wunused-variable]
62 | unsigned long precision = _precision; \
| ^~~~~~~~~
drivers/net/virtio_net.c:54:1: note: in expansion of macro 'DECLARE_EWMA'
54 | DECLARE_EWMA(pkt_len, 0, 64)
| ^~~~~~~~~~~~
>> include/linux/average.h:61:31: warning: unused variable 'weight_rcp' [-Wunused-variable]
61 | unsigned long weight_rcp = ilog2(_weight_rcp); \
| ^~~~~~~~~~
drivers/net/virtio_net.c:54:1: note: in expansion of macro 'DECLARE_EWMA'
54 | DECLARE_EWMA(pkt_len, 0, 64)
| ^~~~~~~~~~~~
>> include/linux/average.h:60:31: warning: unused variable 'internal' [-Wunused-variable]
60 | unsigned long internal = READ_ONCE(e->internal); \
| ^~~~~~~~
drivers/net/virtio_net.c:54:1: note: in expansion of macro 'DECLARE_EWMA'
54 | DECLARE_EWMA(pkt_len, 0, 64)
| ^~~~~~~~~~~~
In file included from ./arch/x86/include/generated/asm/rwonce.h:1,
from include/linux/compiler.h:247,
from include/linux/build_bug.h:5,
from include/linux/container_of.h:5,
from include/linux/list.h:5,
from include/linux/timer.h:5,
from include/linux/netdevice.h:24,
from drivers/net/virtio_net.c:7:
drivers/net/virtio_net.c: In function 'ewma_pkt_len_add_or_init':
>> include/asm-generic/rwonce.h:59:1: error: expected ';' before 'do'
59 | do { \
| ^~
include/linux/average.h:79:17: note: in expansion of macro 'WRITE_ONCE'
79 | WRITE_ONCE(e->internal, internal ? \
| ^~~~~~~~~~
drivers/net/virtio_net.c:54:1: note: in expansion of macro 'DECLARE_EWMA'
54 | DECLARE_EWMA(pkt_len, 0, 64)
| ^~~~~~~~~~~~
In file included from drivers/net/virtio_net.c:19:
include/linux/average.h:75:31: warning: unused variable 'precision' [-Wunused-variable]
75 | unsigned long precision = _precision; \
| ^~~~~~~~~
drivers/net/virtio_net.c:54:1: note: in expansion of macro 'DECLARE_EWMA'
54 | DECLARE_EWMA(pkt_len, 0, 64)
| ^~~~~~~~~~~~
include/linux/average.h:74:31: warning: unused variable 'weight_rcp' [-Wunused-variable]
74 | unsigned long weight_rcp = ilog2(_weight_rcp); \
| ^~~~~~~~~~
drivers/net/virtio_net.c:54:1: note: in expansion of macro 'DECLARE_EWMA'
54 | DECLARE_EWMA(pkt_len, 0, 64)
| ^~~~~~~~~~~~
include/linux/average.h:73:31: warning: unused variable 'internal' [-Wunused-variable]
73 | unsigned long internal = READ_ONCE(e->internal); \
| ^~~~~~~~
drivers/net/virtio_net.c:54:1: note: in expansion of macro 'DECLARE_EWMA'
54 | DECLARE_EWMA(pkt_len, 0, 64)
| ^~~~~~~~~~~~
drivers/net/virtio_net.c: In function 'ewma_pkt_len_read':
include/linux/average.h:56:9: error: control reaches end of non-void function [-Werror=return-type]
56 | } \
| ^
drivers/net/virtio_net.c:54:1: note: in expansion of macro 'DECLARE_EWMA'
54 | DECLARE_EWMA(pkt_len, 0, 64)
| ^~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +45 include/linux/average.h
8
9 /*
10 * Exponentially weighted moving average (EWMA)
11 *
12 * This implements a fixed-precision EWMA algorithm, with both the
13 * precision and fall-off coefficient determined at compile-time
14 * and built into the generated helper funtions.
15 *
16 * The first argument to the macro is the name that will be used
17 * for the struct and helper functions.
18 *
19 * The second argument, the precision, expresses how many bits are
20 * used for the fractional part of the fixed-precision values.
21 *
22 * The third argument, the weight reciprocal, determines how the
23 * new values will be weighed vs. the old state, new values will
24 * get weight 1/weight_rcp and old values 1-1/weight_rcp. Note
25 * that this parameter must be a power of two for efficiency.
26 */
27
28 #define EWMA_BUILD_TIME_CHECKS(_precision, _weight_rcp) \
29 do { \
30 BUILD_BUG_ON(!__builtin_constant_p(_precision)); \
31 BUILD_BUG_ON(!__builtin_constant_p(_weight_rcp)); \
32 \
33 BUILD_BUG_ON((_precision) > 30); \
34 BUILD_BUG_ON_NOT_POWER_OF_2(_weight_rcp); \
35 } while (0)
36
37 #define DECLARE_EWMA(name, _precision, _weight_rcp) \
38 struct ewma_##name { \
39 unsigned long internal; \
40 }; \
41 static inline void ewma_##name##_init_val(struct ewma_##name *e, \
42 unsigned long init) \
43 { \
44 EWMA_BUILD_TIME_CHECKS(_precision, _weight_rcp) \
> 45 e->internal = init << _precision; \
46 } \
47 static inline void ewma_##name##_init(struct ewma_##name *e) \
48 { \
49 ewma_##name##_init_val(e, 0); \
50 } \
51 static inline unsigned long \
52 ewma_##name##_read(struct ewma_##name *e) \
53 { \
54 EWMA_BUILD_TIME_CHECKS(_precision, _weight_rcp) \
> 55 return e->internal >> (_precision); \
56 } \
57 static inline void ewma_##name##_add(struct ewma_##name *e, \
58 unsigned long val) \
59 { \
> 60 unsigned long internal = READ_ONCE(e->internal); \
> 61 unsigned long weight_rcp = ilog2(_weight_rcp); \
> 62 unsigned long precision = _precision; \
63 \
64 EWMA_BUILD_TIME_CHECKS(_precision, _weight_rcp) \
65 \
> 66 WRITE_ONCE(e->internal, \
67 (((internal << weight_rcp) - internal) + \
68 (val << precision)) >> weight_rcp); \
69 } \
70 static inline void ewma_##name##_add_or_init(struct ewma_##name *e, \
71 unsigned long val) \
72 { \
73 unsigned long internal = READ_ONCE(e->internal); \
74 unsigned long weight_rcp = ilog2(_weight_rcp); \
75 unsigned long precision = _precision; \
76 \
77 EWMA_BUILD_TIME_CHECKS(_precision, _weight_rcp) \
78 \
79 WRITE_ONCE(e->internal, internal ? \
80 (((internal << weight_rcp) - internal) + \
81 (val << precision)) >> weight_rcp : \
82 (val << precision)); \
83 }
84
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC v2] average: rewrite for clearity
2023-04-21 13:46 [RFC v2] average: rewrite for clearity Benjamin Beichler
` (2 preceding siblings ...)
2023-04-21 19:53 ` kernel test robot
@ 2023-04-21 19:53 ` kernel test robot
3 siblings, 0 replies; 8+ messages in thread
From: kernel test robot @ 2023-04-21 19:53 UTC (permalink / raw)
To: Benjamin Beichler; +Cc: oe-kbuild-all
Hi Benjamin,
[This is a private test report for your RFC patch.]
kernel test robot noticed the following build warnings:
[auto build test WARNING on wireless-next/main]
[also build test WARNING on wireless/main linus/master v6.3-rc7 next-20230420]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Benjamin-Beichler/average-rewrite-for-clearity/20230421-214746
base: https://git.kernel.org/pub/scm/linux/kernel/git/wireless/wireless-next.git main
patch link: https://lore.kernel.org/r/20230421134604.211128-1-benjamin.beichler%40uni-rostock.de
patch subject: [RFC v2] average: rewrite for clearity
config: x86_64-defconfig (https://download.01.org/0day-ci/archive/20230422/202304220315.LBDGnyr0-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/f61e21185a0ffa11e3d84ed0b6159882a921347e
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Benjamin-Beichler/average-rewrite-for-clearity/20230421-214746
git checkout f61e21185a0ffa11e3d84ed0b6159882a921347e
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=x86_64 olddefconfig
make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/gpu/drm/i915/ drivers/net/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304220315.LBDGnyr0-lkp@intel.com/
All warnings (new ones prefixed by >>):
In file included from drivers/net/virtio_net.c:19:
drivers/net/virtio_net.c: In function 'ewma_pkt_len_init_val':
include/linux/average.h:45:17: error: expected ';' before 'e'
45 | e->internal = init << _precision; \
| ^
drivers/net/virtio_net.c:54:1: note: in expansion of macro 'DECLARE_EWMA'
54 | DECLARE_EWMA(pkt_len, 0, 64)
| ^~~~~~~~~~~~
drivers/net/virtio_net.c: In function 'ewma_pkt_len_read':
include/linux/average.h:55:17: error: expected ';' before 'return'
55 | return e->internal >> (_precision); \
| ^~~~~~
drivers/net/virtio_net.c:54:1: note: in expansion of macro 'DECLARE_EWMA'
54 | DECLARE_EWMA(pkt_len, 0, 64)
| ^~~~~~~~~~~~
In file included from ./arch/x86/include/generated/asm/rwonce.h:1,
from include/linux/compiler.h:247,
from include/linux/build_bug.h:5,
from include/linux/container_of.h:5,
from include/linux/list.h:5,
from include/linux/timer.h:5,
from include/linux/netdevice.h:24,
from drivers/net/virtio_net.c:7:
drivers/net/virtio_net.c: In function 'ewma_pkt_len_add':
include/asm-generic/rwonce.h:59:1: error: expected ';' before 'do'
59 | do { \
| ^~
include/linux/average.h:66:17: note: in expansion of macro 'WRITE_ONCE'
66 | WRITE_ONCE(e->internal, \
| ^~~~~~~~~~
drivers/net/virtio_net.c:54:1: note: in expansion of macro 'DECLARE_EWMA'
54 | DECLARE_EWMA(pkt_len, 0, 64)
| ^~~~~~~~~~~~
In file included from drivers/net/virtio_net.c:19:
>> include/linux/average.h:62:31: warning: unused variable 'precision' [-Wunused-variable]
62 | unsigned long precision = _precision; \
| ^~~~~~~~~
drivers/net/virtio_net.c:54:1: note: in expansion of macro 'DECLARE_EWMA'
54 | DECLARE_EWMA(pkt_len, 0, 64)
| ^~~~~~~~~~~~
>> include/linux/average.h:61:31: warning: unused variable 'weight_rcp' [-Wunused-variable]
61 | unsigned long weight_rcp = ilog2(_weight_rcp); \
| ^~~~~~~~~~
drivers/net/virtio_net.c:54:1: note: in expansion of macro 'DECLARE_EWMA'
54 | DECLARE_EWMA(pkt_len, 0, 64)
| ^~~~~~~~~~~~
>> include/linux/average.h:60:31: warning: unused variable 'internal' [-Wunused-variable]
60 | unsigned long internal = READ_ONCE(e->internal); \
| ^~~~~~~~
drivers/net/virtio_net.c:54:1: note: in expansion of macro 'DECLARE_EWMA'
54 | DECLARE_EWMA(pkt_len, 0, 64)
| ^~~~~~~~~~~~
In file included from ./arch/x86/include/generated/asm/rwonce.h:1,
from include/linux/compiler.h:247,
from include/linux/build_bug.h:5,
from include/linux/container_of.h:5,
from include/linux/list.h:5,
from include/linux/timer.h:5,
from include/linux/netdevice.h:24,
from drivers/net/virtio_net.c:7:
drivers/net/virtio_net.c: In function 'ewma_pkt_len_add_or_init':
include/asm-generic/rwonce.h:59:1: error: expected ';' before 'do'
59 | do { \
| ^~
include/linux/average.h:79:17: note: in expansion of macro 'WRITE_ONCE'
79 | WRITE_ONCE(e->internal, internal ? \
| ^~~~~~~~~~
drivers/net/virtio_net.c:54:1: note: in expansion of macro 'DECLARE_EWMA'
54 | DECLARE_EWMA(pkt_len, 0, 64)
| ^~~~~~~~~~~~
In file included from drivers/net/virtio_net.c:19:
include/linux/average.h:75:31: warning: unused variable 'precision' [-Wunused-variable]
75 | unsigned long precision = _precision; \
| ^~~~~~~~~
drivers/net/virtio_net.c:54:1: note: in expansion of macro 'DECLARE_EWMA'
54 | DECLARE_EWMA(pkt_len, 0, 64)
| ^~~~~~~~~~~~
include/linux/average.h:74:31: warning: unused variable 'weight_rcp' [-Wunused-variable]
74 | unsigned long weight_rcp = ilog2(_weight_rcp); \
| ^~~~~~~~~~
drivers/net/virtio_net.c:54:1: note: in expansion of macro 'DECLARE_EWMA'
54 | DECLARE_EWMA(pkt_len, 0, 64)
| ^~~~~~~~~~~~
include/linux/average.h:73:31: warning: unused variable 'internal' [-Wunused-variable]
73 | unsigned long internal = READ_ONCE(e->internal); \
| ^~~~~~~~
drivers/net/virtio_net.c:54:1: note: in expansion of macro 'DECLARE_EWMA'
54 | DECLARE_EWMA(pkt_len, 0, 64)
| ^~~~~~~~~~~~
drivers/net/virtio_net.c: In function 'ewma_pkt_len_read':
include/linux/average.h:56:9: error: control reaches end of non-void function [-Werror=return-type]
56 | } \
| ^
drivers/net/virtio_net.c:54:1: note: in expansion of macro 'DECLARE_EWMA'
54 | DECLARE_EWMA(pkt_len, 0, 64)
| ^~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +/precision +62 include/linux/average.h
ef4d9af62f47e3 Mark Rutland 2017-10-23 8
eb1e011a14748a Johannes Berg 2017-02-15 9 /*
eb1e011a14748a Johannes Berg 2017-02-15 10 * Exponentially weighted moving average (EWMA)
eb1e011a14748a Johannes Berg 2017-02-15 11 *
eb1e011a14748a Johannes Berg 2017-02-15 12 * This implements a fixed-precision EWMA algorithm, with both the
eb1e011a14748a Johannes Berg 2017-02-15 13 * precision and fall-off coefficient determined at compile-time
eb1e011a14748a Johannes Berg 2017-02-15 14 * and built into the generated helper funtions.
eb1e011a14748a Johannes Berg 2017-02-15 15 *
eb1e011a14748a Johannes Berg 2017-02-15 16 * The first argument to the macro is the name that will be used
eb1e011a14748a Johannes Berg 2017-02-15 17 * for the struct and helper functions.
eb1e011a14748a Johannes Berg 2017-02-15 18 *
eb1e011a14748a Johannes Berg 2017-02-15 19 * The second argument, the precision, expresses how many bits are
eb1e011a14748a Johannes Berg 2017-02-15 20 * used for the fractional part of the fixed-precision values.
eb1e011a14748a Johannes Berg 2017-02-15 21 *
eb1e011a14748a Johannes Berg 2017-02-15 22 * The third argument, the weight reciprocal, determines how the
eb1e011a14748a Johannes Berg 2017-02-15 23 * new values will be weighed vs. the old state, new values will
eb1e011a14748a Johannes Berg 2017-02-15 24 * get weight 1/weight_rcp and old values 1-1/weight_rcp. Note
eb1e011a14748a Johannes Berg 2017-02-15 25 * that this parameter must be a power of two for efficiency.
eb1e011a14748a Johannes Berg 2017-02-15 26 */
c5485a7e7569ab Bruno Randolf 2010-11-16 27
f61e21185a0ffa Benjamin Beichler 2023-04-21 28 #define EWMA_BUILD_TIME_CHECKS(_precision, _weight_rcp) \
f61e21185a0ffa Benjamin Beichler 2023-04-21 29 do { \
f61e21185a0ffa Benjamin Beichler 2023-04-21 30 BUILD_BUG_ON(!__builtin_constant_p(_precision)); \
f61e21185a0ffa Benjamin Beichler 2023-04-21 31 BUILD_BUG_ON(!__builtin_constant_p(_weight_rcp)); \
f61e21185a0ffa Benjamin Beichler 2023-04-21 32 \
f61e21185a0ffa Benjamin Beichler 2023-04-21 33 BUILD_BUG_ON((_precision) > 30); \
f61e21185a0ffa Benjamin Beichler 2023-04-21 34 BUILD_BUG_ON_NOT_POWER_OF_2(_weight_rcp); \
f61e21185a0ffa Benjamin Beichler 2023-04-21 35 } while (0)
f61e21185a0ffa Benjamin Beichler 2023-04-21 36
eb1e011a14748a Johannes Berg 2017-02-15 37 #define DECLARE_EWMA(name, _precision, _weight_rcp) \
2377799c084d86 Johannes Berg 2015-07-13 38 struct ewma_##name { \
2377799c084d86 Johannes Berg 2015-07-13 39 unsigned long internal; \
2377799c084d86 Johannes Berg 2015-07-13 40 }; \
f61e21185a0ffa Benjamin Beichler 2023-04-21 41 static inline void ewma_##name##_init_val(struct ewma_##name *e, \
f61e21185a0ffa Benjamin Beichler 2023-04-21 42 unsigned long init) \
f61e21185a0ffa Benjamin Beichler 2023-04-21 43 { \
f61e21185a0ffa Benjamin Beichler 2023-04-21 44 EWMA_BUILD_TIME_CHECKS(_precision, _weight_rcp) \
f61e21185a0ffa Benjamin Beichler 2023-04-21 45 e->internal = init << _precision; \
f61e21185a0ffa Benjamin Beichler 2023-04-21 46 } \
2377799c084d86 Johannes Berg 2015-07-13 47 static inline void ewma_##name##_init(struct ewma_##name *e) \
2377799c084d86 Johannes Berg 2015-07-13 48 { \
f61e21185a0ffa Benjamin Beichler 2023-04-21 49 ewma_##name##_init_val(e, 0); \
2377799c084d86 Johannes Berg 2015-07-13 50 } \
2377799c084d86 Johannes Berg 2015-07-13 51 static inline unsigned long \
2377799c084d86 Johannes Berg 2015-07-13 52 ewma_##name##_read(struct ewma_##name *e) \
2377799c084d86 Johannes Berg 2015-07-13 53 { \
f61e21185a0ffa Benjamin Beichler 2023-04-21 54 EWMA_BUILD_TIME_CHECKS(_precision, _weight_rcp) \
eb1e011a14748a Johannes Berg 2017-02-15 55 return e->internal >> (_precision); \
2377799c084d86 Johannes Berg 2015-07-13 56 } \
2377799c084d86 Johannes Berg 2015-07-13 57 static inline void ewma_##name##_add(struct ewma_##name *e, \
2377799c084d86 Johannes Berg 2015-07-13 58 unsigned long val) \
2377799c084d86 Johannes Berg 2015-07-13 59 { \
ef4d9af62f47e3 Mark Rutland 2017-10-23 @60 unsigned long internal = READ_ONCE(e->internal); \
eb1e011a14748a Johannes Berg 2017-02-15 @61 unsigned long weight_rcp = ilog2(_weight_rcp); \
eb1e011a14748a Johannes Berg 2017-02-15 @62 unsigned long precision = _precision; \
2377799c084d86 Johannes Berg 2015-07-13 63 \
f61e21185a0ffa Benjamin Beichler 2023-04-21 64 EWMA_BUILD_TIME_CHECKS(_precision, _weight_rcp) \
f61e21185a0ffa Benjamin Beichler 2023-04-21 65 \
f61e21185a0ffa Benjamin Beichler 2023-04-21 @66 WRITE_ONCE(e->internal, \
f61e21185a0ffa Benjamin Beichler 2023-04-21 67 (((internal << weight_rcp) - internal) + \
f61e21185a0ffa Benjamin Beichler 2023-04-21 68 (val << precision)) >> weight_rcp); \
f61e21185a0ffa Benjamin Beichler 2023-04-21 69 } \
f61e21185a0ffa Benjamin Beichler 2023-04-21 70 static inline void ewma_##name##_add_or_init(struct ewma_##name *e, \
f61e21185a0ffa Benjamin Beichler 2023-04-21 71 unsigned long val) \
f61e21185a0ffa Benjamin Beichler 2023-04-21 72 { \
f61e21185a0ffa Benjamin Beichler 2023-04-21 73 unsigned long internal = READ_ONCE(e->internal); \
f61e21185a0ffa Benjamin Beichler 2023-04-21 74 unsigned long weight_rcp = ilog2(_weight_rcp); \
f61e21185a0ffa Benjamin Beichler 2023-04-21 75 unsigned long precision = _precision; \
f61e21185a0ffa Benjamin Beichler 2023-04-21 76 \
f61e21185a0ffa Benjamin Beichler 2023-04-21 77 EWMA_BUILD_TIME_CHECKS(_precision, _weight_rcp) \
2377799c084d86 Johannes Berg 2015-07-13 78 \
ef4d9af62f47e3 Mark Rutland 2017-10-23 79 WRITE_ONCE(e->internal, internal ? \
eb1e011a14748a Johannes Berg 2017-02-15 80 (((internal << weight_rcp) - internal) + \
eb1e011a14748a Johannes Berg 2017-02-15 81 (val << precision)) >> weight_rcp : \
ef4d9af62f47e3 Mark Rutland 2017-10-23 82 (val << precision)); \
2377799c084d86 Johannes Berg 2015-07-13 83 }
2377799c084d86 Johannes Berg 2015-07-13 84
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC v2] average: rewrite for clearity
2023-04-21 15:16 ` Benjamin Beichler
@ 2023-04-24 15:55 ` Johannes Berg
2023-04-24 21:32 ` Benjamin Beichler
0 siblings, 1 reply; 8+ messages in thread
From: Johannes Berg @ 2023-04-24 15:55 UTC (permalink / raw)
To: Benjamin Beichler, nbd, linux-wireless
On Fri, 2023-04-21 at 17:16 +0200, Benjamin Beichler wrote:
> >
> > So then we could say we'll just fix them, but what value actually makes
> > sense to init with? You don't necessarily know, right? Initially biasing
> > towards the first value makes a lot more sense than initially biasing
> > towards zero, no? And then if you want to achieve that, now you have to
> > either use _add_or_init(), which is probably what people will do, but
> > that continues having the problem ...
>
> I left out the the individual fixes for users, but for the samples I
> looked into, either start values were given, or they were semantically
> extractable.
>
> e.g. virtios pkt_len ewma is clamped anyways, so using the clamp border
> or in between will be safe.
>
> Most others are signals-strengths, many of them also only for
> statistics, where a slow convergence is okay in my opinion.
Yeah I guess that's the thing, we can accept slower convergence in many
cases, and "safe" start values mean that mostly. But why accept it when
we don't have to?
> IMHO the net improvement is, that if you do not use the convenience
> add_or_init function, it simply resembles the ewma filter of a math or
> CS-textbook.
Not sure I see that as a good argument. The practical engineering side
tends to always be more complex than the theory, and that's not really
unexpected. We can comment why it's different :-)
> The original problem was, that the ewma had a surprising
> output in a special situation.
Right, but that's an implementation issue, because we thought 0 ==
uninit was clever, without realizing the corner case.
> But while writing the commit, I recognized, that the current ewma
> implementation is only for unsigned values, which means the problem
> really only happens for many consecutive 0 values. I try to think over,
> what this means for the proposal of Felix, but I'm not able to think
> about unsigned C-arithmetics today :-D
Not much really, I think? It eases thinking about it though :)
> If we use that fix, then we should have an additional compile time
> check, that the precision has at least 2 or 4 bits, to really avoid the
> problem, shouldn't we?
Yes. I think 1 bit is enough, but yes, it can't be 0 bits.
johannes
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC v2] average: rewrite for clearity
2023-04-24 15:55 ` Johannes Berg
@ 2023-04-24 21:32 ` Benjamin Beichler
0 siblings, 0 replies; 8+ messages in thread
From: Benjamin Beichler @ 2023-04-24 21:32 UTC (permalink / raw)
To: Johannes Berg, nbd, linux-wireless
Am 24.04.2023 um 17:55 schrieb Johannes Berg:
> ________________________________
> Achtung! Externe E-Mail: Klicken Sie erst dann auf Links und Anhänge, nachdem Sie die Vertrauenswürdigkeit der Absenderadresse geprüft haben.
> ________________________________
>
> On Fri, 2023-04-21 at 17:16 +0200, Benjamin Beichler wrote:
>> IMHO the net improvement is, that if you do not use the convenience
>> add_or_init function, it simply resembles the ewma filter of a math or
>> CS-textbook.
> Not sure I see that as a good argument. The practical engineering side
> tends to always be more complex than the theory, and that's not really
> unexpected. We can comment why it's different :-)
In my opinion, the behavior was without good reason unexpected, but I
think I found a solution, that we are all happy with. See v3.
>
>> The original problem was, that the ewma had a surprising
>> output in a special situation.
> Right, but that's an implementation issue, because we thought 0 ==
> uninit was clever, without realizing the corner case.
>
>> But while writing the commit, I recognized, that the current ewma
>> implementation is only for unsigned values, which means the problem
>> really only happens for many consecutive 0 values. I try to think over,
>> what this means for the proposal of Felix, but I'm not able to think
>> about unsigned C-arithmetics today :-D
> Not much really, I think? It eases thinking about it though :)
>
>> If we use that fix, then we should have an additional compile time
>> check, that the precision has at least 2 or 4 bits, to really avoid the
>> problem, shouldn't we?
> Yes. I think 1 bit is enough, but yes, it can't be 0 bits.
I have wrapped my head around it again, and just send a new version
(which I may split in two patches for the final round). I played around
with the fixed point arithmetic and came to the conclusion, that
ULONG_MAX is a better candidate for initial state, since for valid EWMA
configurations, i.e. a weight_rcp bigger than 1, ULONG_MAX is never
reached. Actually, a precision (or fraction) bigger than 0 seems to be
not needed. However, this means maybe some unexpected oscillation at the
lower bits, so we may enforce it by another compile time check, but
actually I' not really concerned about that :-D
In contrast, I recognized, that if val is too big for the
weight_rcp/precision combination, it can overflow the internal state and
result into much lower values and unexpected jumps of the state.
Currently, I just added a WARN_ONCE for this. For all current users,
this problem should never happen, but a separate clamping of the input
val is maybe too much, and could also be easily implemented by the user
of the function.
Benjamin
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-04-24 21:32 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-04-21 13:46 [RFC v2] average: rewrite for clearity Benjamin Beichler
2023-04-21 14:37 ` Johannes Berg
2023-04-21 15:16 ` Benjamin Beichler
2023-04-24 15:55 ` Johannes Berg
2023-04-24 21:32 ` Benjamin Beichler
2023-04-21 18:41 ` kernel test robot
2023-04-21 19:53 ` kernel test robot
2023-04-21 19:53 ` kernel test robot
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.