12:43 p.m.
Hi John,
I love your patch! Perhaps something to improve:
[auto build test WARNING on next-20210607]
url:
https://github.com/0day-ci/linux/commits/John-Ogness/introduce-printk-cpu...
base: 7f09e895a7f3e0af63bf9ec6c7c22893ec7e6c8e
config: mips-randconfig-r036-20210607 (attached as .config)
compiler: mips-linux-gcc (GCC) 9.3.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/0day-ci/linux/commit/136bcc2980e636b2ae156ca63fbe95c71...
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review
John-Ogness/introduce-printk-cpu-lock/20210608-040454
git checkout 136bcc2980e636b2ae156ca63fbe95c713e44c1b
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=mips
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp(a)intel.com>
All warnings (new ones prefixed by >>):
lib/dump_stack.c: In function 'dump_stack_lvl':
> lib/dump_stack.c:107:2: warning: 'lock_flag' is used
uninitialized in this function [-Wuninitialized]
107 |
printk_cpu_unlock_irqrestore(lock_flag, irq_flags);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
vim +/lock_flag +107 lib/dump_stack.c
90
91 /**
92 * dump_stack - dump the current task information and its stack trace
93 *
94 * Architectures can override this implementation by implementing its own.
95 */
96 asmlinkage __visible void dump_stack_lvl(const char *log_lvl)
97 {
98 unsigned long irq_flags;
99 bool lock_flag;
100
101 /*
102 * Permit this cpu to perform nested stack dumps while serialising
103 * against other CPUs
104 */
105 printk_cpu_lock_irqsave(&lock_flag, &irq_flags);
106 __dump_stack(log_lvl);
107 printk_cpu_unlock_irqrestore(lock_flag, irq_flags);
108 }
109 EXPORT_SYMBOL(dump_stack_lvl);
110
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
11:48 p.m.
New subject: [PATCH next v2 1/2] dump_stack: move cpu lock to printk.c
On Tue 2021-06-08 10:43:46, kernel test robot wrote:
Hi John,
I love your patch! Perhaps something to improve:
[auto build test WARNING on next-20210607]
url:
https://github.com/0day-ci/linux/commits/John-Ogness/introduce-printk-cpu...
base: 7f09e895a7f3e0af63bf9ec6c7c22893ec7e6c8e
config: mips-randconfig-r036-20210607 (attached as .config)
compiler: mips-linux-gcc (GCC) 9.3.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/0day-ci/linux/commit/136bcc2980e636b2ae156ca63fbe95c71...
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review
John-Ogness/introduce-printk-cpu-lock/20210608-040454
git checkout 136bcc2980e636b2ae156ca63fbe95c713e44c1b
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=mips
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp(a)intel.com>
All warnings (new ones prefixed by >>):
lib/dump_stack.c: In function 'dump_stack_lvl':
>> lib/dump_stack.c:107:2: warning: 'lock_flag' is used uninitialized in
this function [-Wuninitialized]
107 | printk_cpu_unlock_irqrestore(lock_flag, irq_flags);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Interesting. I am curious that it does not complain also about
irq_flags. But it is possible the it reports only the first problem.
Anyway, we will likely need to do some trickery via #define to tell
the compiler that the value is set. I mean to do similar thing as:
#define raw_local_irq_save(flags) \
do { \
typecheck(unsigned long, flags); \
flags = arch_local_irq_save(); \
} while (0)
In our case, it might look like:
#define printk_cpu_lock_irqsave(lock_nested, irq_flags) \
do { \
local_irq_save(irq_flags); \
typecheck(bool, lock_nested); \
lock_nested = __printk_cpu_lock(irq_flags); \
} while (0)
then we would need to do in __prink_cpu_lock(unsigned long irq_flags)
} else {
local_irq_restore(irq_flags);
/*
* Wait for the lock to release before jumping to cmpxchg()
* in order to mitigate the thundering herd problem.
*/
do {
cpu_relax();
} while (atomic_read(&printk_cpulock_owner) != -1);
local_irq_save(irq_flags)
goto retry;
}
Best Regards,
Petr
11:26 p.m.
New subject: [PATCH next v2 1/2] dump_stack: move cpu lock to printk.c
On 2021-06-08, Petr Mladek <pmladek(a)suse.com> wrote:
> lib/dump_stack.c: In function 'dump_stack_lvl':
> >> lib/dump_stack.c:107:2: warning: 'lock_flag' is used uninitialized
in this function [-Wuninitialized]
> 107 | printk_cpu_unlock_irqrestore(lock_flag, irq_flags);
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Interesting. I am curious that it does not complain also about
irq_flags. But it is possible the it reports only the first problem.
Strangely enough, if I set a value for @lock_flag, it is happy and does
not complain about @irq_flags. Probably a compiler oversight.
Anyway, we will likely need to do some trickery via #define to tell
the compiler that the value is set.
This is on ARCH=mips and !CONFIG_SMP. So the value is _not_ getting
set. (The static inline function does nothing.)
By changing printk_cpu_unlock_irqrestore() to use pointers:
static inline void printk_cpu_unlock_irqrestore(bool *lock_flag, unsigned long
*irq_flags)
then the warning disappears. Indeed, by not using pointers on unlock,
technically data is copied that was never initialized. I thought maybe
the compiler would optimize all that out, but it seems that it does not.
I have no problems using pointers for unlock(). It was strange using
pointers for lock(), but not for unlock() anyway.
Or would you prefer something else?
John Ogness
5 p.m.
New subject: [PATCH next v2 1/2] dump_stack: move cpu lock to printk.c
On Thu 2021-06-10 15:26:15, John Ogness wrote:
On 2021-06-08, Petr Mladek <pmladek(a)suse.com> wrote:
>> lib/dump_stack.c: In function 'dump_stack_lvl':
>> >> lib/dump_stack.c:107:2: warning: 'lock_flag' is used
uninitialized in this function [-Wuninitialized]
>> 107 | printk_cpu_unlock_irqrestore(lock_flag, irq_flags);
>> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Interesting. I am curious that it does not complain also about
> irq_flags. But it is possible the it reports only the first problem.
Strangely enough, if I set a value for @lock_flag, it is happy and does
not complain about @irq_flags. Probably a compiler oversight.
Yeah, it is strange.
> Anyway, we will likely need to do some trickery via #define to
tell
> the compiler that the value is set.
This is on ARCH=mips and !CONFIG_SMP. So the value is _not_ getting
set. (The static inline function does nothing.)
By changing printk_cpu_unlock_irqrestore() to use pointers:
static inline void printk_cpu_unlock_irqrestore(bool *lock_flag, unsigned long
*irq_flags)
then the warning disappears. Indeed, by not using pointers on unlock,
technically data is copied that was never initialized. I thought maybe
the compiler would optimize all that out, but it seems that it does not.
I have no problems using pointers for unlock(). It was strange using
pointers for lock(), but not for unlock() anyway.
Or would you prefer something else?
I would actually prefer to introduce the macros and pass the flags
without referencing.
I was about to write that I did not mind. But then it came to me that
it might be worth being compatible with the other
irqsafe()/irqrestore() APIs. It seems that people are pretty used
to pass flags directly:
$> git grep irqsave.*flags | wc -l
17084
$> git grep irqsave.*\&flags | wc -l
15
That said, I do not resist on it. It will not block the patchset
if you decided to used the pointers. The lock should not be used
widely...
Best Regards,
Petr
466
days inactive
469
days old
3 comments
3 participants
participants (3)
-
John Ogness -
kernel test robot -
Petr Mladek