The shrinker_info is dereferenced in a couple of places via rcu_dereference_protected
with different calling conventions, for example, using mem_cgroup_nodeinfo helper
or dereferencing memcg->nodeinfo[nid]->shrinker_info. And the later patch
will add more dereference places.
So extract the dereference into a helper to make the code more readable. No
functional change.
Signed-off-by: Yang Shi <[email protected]>
---
mm/vmscan.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 9436f9246d32..273efbf4d53c 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -190,6 +190,13 @@ static int shrinker_nr_max;
#define NR_MAX_TO_SHR_MAP_SIZE(nr_max) \
(DIV_ROUND_UP(nr_max, BITS_PER_LONG) * sizeof(unsigned long))
+static struct shrinker_info *shrinker_info_protected(struct mem_cgroup *memcg,
+ int nid)
+{
+ return rcu_dereference_protected(memcg->nodeinfo[nid]->shrinker_info,
+ lockdep_is_held(&shrinker_rwsem));
+}
+
static void free_shrinker_info_rcu(struct rcu_head *head)
{
kvfree(container_of(head, struct shrinker_info, rcu));
@@ -202,8 +209,7 @@ static int expand_one_shrinker_info(struct mem_cgroup *memcg,
int nid;
for_each_node(nid) {
- old = rcu_dereference_protected(
- mem_cgroup_nodeinfo(memcg, nid)->shrinker_info, true);
+ old = shrinker_info_protected(memcg, nid);
/* Not yet online memcg */
if (!old)
return 0;
@@ -234,7 +240,7 @@ void free_shrinker_info(struct mem_cgroup *memcg)
for_each_node(nid) {
pn = mem_cgroup_nodeinfo(memcg, nid);
- info = rcu_dereference_protected(pn->shrinker_info, true);
+ info = shrinker_info_protected(memcg, nid);
kvfree(info);
rcu_assign_pointer(pn->shrinker_info, NULL);
}
@@ -674,8 +680,7 @@ static unsigned long shrink_slab_memcg(gfp_t gfp_mask, int nid,
if (!down_read_trylock(&shrinker_rwsem))
return 0;
- info = rcu_dereference_protected(memcg->nodeinfo[nid]->shrinker_info,
- true);
+ info = shrinker_info_protected(memcg, nid);
if (unlikely(!info))
goto unlock;
--
2.26.2
On Tue, Feb 09, 2021 at 09:46:40AM -0800, Yang Shi wrote:
> The shrinker_info is dereferenced in a couple of places via rcu_dereference_protected
> with different calling conventions, for example, using mem_cgroup_nodeinfo helper
> or dereferencing memcg->nodeinfo[nid]->shrinker_info. And the later patch
> will add more dereference places.
>
> So extract the dereference into a helper to make the code more readable. No
> functional change.
>
> Signed-off-by: Yang Shi <[email protected]>
> ---
> mm/vmscan.c | 15 ++++++++++-----
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 9436f9246d32..273efbf4d53c 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -190,6 +190,13 @@ static int shrinker_nr_max;
> #define NR_MAX_TO_SHR_MAP_SIZE(nr_max) \
> (DIV_ROUND_UP(nr_max, BITS_PER_LONG) * sizeof(unsigned long))
>
> +static struct shrinker_info *shrinker_info_protected(struct mem_cgroup *memcg,
> + int nid)
> +{
> + return rcu_dereference_protected(memcg->nodeinfo[nid]->shrinker_info,
> + lockdep_is_held(&shrinker_rwsem));
> +}
> +
I'd probably drop the "protected" suffix (because there is no unprotected version,
right?).
Other than that LGTM.
Acked-by: Roman Gushchin <[email protected]>
On Tue, Feb 9, 2021 at 4:22 PM Roman Gushchin <[email protected]> wrote:
>
> On Tue, Feb 09, 2021 at 09:46:40AM -0800, Yang Shi wrote:
> > The shrinker_info is dereferenced in a couple of places via rcu_dereference_protected
> > with different calling conventions, for example, using mem_cgroup_nodeinfo helper
> > or dereferencing memcg->nodeinfo[nid]->shrinker_info. And the later patch
> > will add more dereference places.
> >
> > So extract the dereference into a helper to make the code more readable. No
> > functional change.
> >
> > Signed-off-by: Yang Shi <[email protected]>
> > ---
> > mm/vmscan.c | 15 ++++++++++-----
> > 1 file changed, 10 insertions(+), 5 deletions(-)
> >
> > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > index 9436f9246d32..273efbf4d53c 100644
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> > @@ -190,6 +190,13 @@ static int shrinker_nr_max;
> > #define NR_MAX_TO_SHR_MAP_SIZE(nr_max) \
> > (DIV_ROUND_UP(nr_max, BITS_PER_LONG) * sizeof(unsigned long))
> >
> > +static struct shrinker_info *shrinker_info_protected(struct mem_cgroup *memcg,
> > + int nid)
> > +{
> > + return rcu_dereference_protected(memcg->nodeinfo[nid]->shrinker_info,
> > + lockdep_is_held(&shrinker_rwsem));
> > +}
> > +
>
>
> I'd probably drop the "protected" suffix (because there is no unprotected version,
> right?).
No, actually there is one "unprotected" call in set_shrinker_bit().
>
> Other than that LGTM.
>
> Acked-by: Roman Gushchin <[email protected]>
On Tue, Feb 09, 2021 at 05:07:07PM -0800, Yang Shi wrote:
> On Tue, Feb 9, 2021 at 4:22 PM Roman Gushchin <[email protected]> wrote:
> >
> > On Tue, Feb 09, 2021 at 09:46:40AM -0800, Yang Shi wrote:
> > > The shrinker_info is dereferenced in a couple of places via rcu_dereference_protected
> > > with different calling conventions, for example, using mem_cgroup_nodeinfo helper
> > > or dereferencing memcg->nodeinfo[nid]->shrinker_info. And the later patch
> > > will add more dereference places.
> > >
> > > So extract the dereference into a helper to make the code more readable. No
> > > functional change.
> > >
> > > Signed-off-by: Yang Shi <[email protected]>
> > > ---
> > > mm/vmscan.c | 15 ++++++++++-----
> > > 1 file changed, 10 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > > index 9436f9246d32..273efbf4d53c 100644
> > > --- a/mm/vmscan.c
> > > +++ b/mm/vmscan.c
> > > @@ -190,6 +190,13 @@ static int shrinker_nr_max;
> > > #define NR_MAX_TO_SHR_MAP_SIZE(nr_max) \
> > > (DIV_ROUND_UP(nr_max, BITS_PER_LONG) * sizeof(unsigned long))
> > >
> > > +static struct shrinker_info *shrinker_info_protected(struct mem_cgroup *memcg,
> > > + int nid)
> > > +{
> > > + return rcu_dereference_protected(memcg->nodeinfo[nid]->shrinker_info,
> > > + lockdep_is_held(&shrinker_rwsem));
> > > +}
> > > +
> >
> >
> > I'd probably drop the "protected" suffix (because there is no unprotected version,
> > right?).
>
> No, actually there is one "unprotected" call in set_shrinker_bit().
Ah, ok. Then it makes sense. Sorry.
>
> >
> > Other than that LGTM.
> >
> > Acked-by: Roman Gushchin <[email protected]>
On 2/9/21 6:46 PM, Yang Shi wrote:
> The shrinker_info is dereferenced in a couple of places via rcu_dereference_protected
> with different calling conventions, for example, using mem_cgroup_nodeinfo helper
> or dereferencing memcg->nodeinfo[nid]->shrinker_info. And the later patch
> will add more dereference places.
>
> So extract the dereference into a helper to make the code more readable. No
> functional change.
>
> Signed-off-by: Yang Shi <[email protected]>
Acked-by: Vlastimil Babka <[email protected]>
Greeting,
FYI, we noticed the following commit (built with gcc-9):
commit: bd741fb2adf349f6bf931e43331b45ef828e1da4 ("mm: vmscan: add shrinker_info_protected() helper")
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git Yang-Shi/Make-shrinker-s-nr_deferred-memcg-aware/20210210-020209
in testcase: locktorture
version:
with following parameters:
runtime: 300s
test: default
test-description: This torture test consists of creating a number of kernel threads which acquire the lock and hold it for specific amount of time, thus simulating different critical region behaviors.
test-url: https://www.kernel.org/doc/Documentation/locking/locktorture.txt
on test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -smp 2 -m 8G
caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
+----------------------------------------------------------+------------+------------+
| | b4182da22d | bd741fb2ad |
+----------------------------------------------------------+------------+------------+
| boot_successes | 25 | 0 |
| boot_failures | 3 | 28 |
| WARNING:kernel_stack | 2 | 2 |
| RIP:clear_page_rep | 1 | |
| WARNING:suspicious_RCU_usage | 0 | 28 |
| mm/vmscan.c:#suspicious_rcu_dereference_protected()usage | 0 | 28 |
+----------------------------------------------------------+------------+------------+
If you fix the issue, kindly add following tag
Reported-by: kernel test robot <[email protected]>
[ 131.472262] WARNING: suspicious RCU usage
[ 131.473301] 5.11.0-rc4-next-20210125-00006-gbd741fb2adf3 #1 Not tainted
[ 131.475075] -----------------------------
[ 131.475925] mm/vmscan.c:196 suspicious rcu_dereference_protected() usage!
[ 131.477392]
[ 131.477392] other info that might help us debug this:
[ 131.477392]
[ 131.479707]
[ 131.479707] rcu_scheduler_active = 2, debug_locks = 1
[ 131.481293] 2 locks held by kworker/1:2/96:
[ 131.482137] #0: ffff888100b74d38 ((wq_completion)cgroup_destroy){+.+.}-{0:0}, at: process_one_work (kbuild/src/consumer/kernel/workqueue.c:2250)
[ 131.484496] #1: ffffc900001b7de0 ((work_completion)(&(&css->destroy_rwork)->work)){+.+.}-{0:0}, at: process_one_work (kbuild/src/consumer/kernel/workqueue.c:2251)
[ 131.487459]
[ 131.487459] stack backtrace:
[ 131.488491] CPU: 1 PID: 96 Comm: kworker/1:2 Not tainted 5.11.0-rc4-next-20210125-00006-gbd741fb2adf3 #1
[ 131.490642] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014
[ 131.492332] Workqueue: cgroup_destroy css_free_rwork_fn
[ 131.493425] Call Trace:
[ 131.494300] dump_stack (kbuild/src/consumer/lib/dump_stack.c:122)
[ 131.495555] lockdep_rcu_suspicious (kbuild/src/consumer/kernel/locking/lockdep.c:6428)
[ 131.496543] shrinker_info_protected (kbuild/src/consumer/mm/vmscan.c:196 (discriminator 7))
[ 131.497622] free_shrinker_info (kbuild/src/consumer/mm/vmscan.c:243 kbuild/src/consumer/mm/vmscan.c:232)
[ 131.498440] mem_cgroup_css_free (kbuild/src/consumer/mm/memcontrol.c:3650 kbuild/src/consumer/mm/memcontrol.c:5325)
[ 131.499407] css_free_rwork_fn (kbuild/src/consumer/include/linux/spinlock.h:359 kbuild/src/consumer/kernel/cgroup/cgroup.c:319 kbuild/src/consumer/kernel/cgroup/cgroup.c:4916)
[ 131.500291] process_one_work (kbuild/src/consumer/kernel/workqueue.c:2280)
[ 131.501441] ? pwq_dec_nr_in_flight (kbuild/src/consumer/kernel/workqueue.c:2171)
[ 131.502342] ? __kasan_check_write (kbuild/src/consumer/mm/kasan/shadow.c:38)
[ 131.503206] ? worker_clr_flags (kbuild/src/consumer/arch/x86/include/asm/atomic.h:95 kbuild/src/consumer/include/asm-generic/atomic-instrumented.h:241 kbuild/src/consumer/kernel/workqueue.c:989)
[ 131.507718] worker_thread (kbuild/src/consumer/include/linux/list.h:282 kbuild/src/consumer/kernel/workqueue.c:2422)
[ 131.508516] ? __kasan_check_read (kbuild/src/consumer/mm/kasan/shadow.c:32)
[ 131.509409] kthread (kbuild/src/consumer/kernel/kthread.c:292)
[ 131.510107] ? process_scheduled_works (kbuild/src/consumer/kernel/workqueue.c:2364)
[ 131.511132] ? kthread_unpark (kbuild/src/consumer/kernel/kthread.c:245)
[ 131.511923] ret_from_fork (kbuild/src/consumer/arch/x86/entry/entry_64.S:302)
[ OK ] Started Load Kernel Modules.
[ OK ] Mounted Huge Pages File System.
Starting Apply Kernel Variables...
Mounting FUSE Control File System...
Starting Load/Save Random Seed...
Starting Create System Users...
[ OK ] Started Apply Kernel Variables.
[ OK ] Mounted FUSE Control File System.
[ OK ] Started Load/Save Random Seed.
[ OK ] Started Create System Users.
Starting Create Static Device Nodes in /dev...
[ OK ] Started Journal Service.
Starting Flush Journal to Persistent Storage...
[ OK ] Started Create Static Device Nodes in /dev.
Starting udev Kernel Device Manager...
[ OK ] Reached target Local File Systems (Pre).
[ OK ] Reached target Local File Systems.
Starting Preprocess NFS configuration...
[ OK ] Started Flush Journal to Persistent Storage.
[ OK ] Started udev Kernel Device Manager.
[ OK ] Started Preprocess NFS configuration.
[ OK ] Reached target NFS client services.
Starting Create Volatile Files and Directories...
[ OK ] Started Create Volatile Files and Directories.
Starting RPC bind portmap service...
Starting Network Time Synchronization...
Starting Update UTMP about System Boot/Shutdown...
[ OK ] Started RPC bind portmap service.
[ OK ] Reached target RPC Port Mapper.
[ OK ] Reached target Remote File Systems (Pre).
[ OK ] Reached target Remote File Systems.
[ OK ] Started Update UTMP about System Boot/Shutdown.
[ OK ] Started Network Time Synchronization.
[ OK ] Reached target System Time Synchronized.
[* ] A start job is running for udev Coldplug all Devices (21s / no limit)
[ 157.124407] random: fast init done
[ OK ] Reached target System Initialization.
[ OK ] Started Daily apt download activities.
[ OK ] Started Daily rotation of log files.
[ OK ] Listening on D-Bus System Message Bus Socket.
[ OK ] Reached target Sockets.
[ OK ] Reached target Basic System.
Starting System Logging Service...
Starting LSB: OpenIPMI Driver init script...
[ OK ] Started Daily apt upgrade and clean activities.
[ OK ] Started Daily Cleanup of Temporary Directories.
[ OK ] Reached target Timers.
[ OK ] Started D-Bus System Message Bus.
[ OK ] Started Regular background program processing daemon.
Starting Login Service...
[ OK ] Started System Logging Service.
[ OK ] Started Helper to synchronize boot up for ifupdown.
Starting Raise network interfaces...
[ OK ] Started Login Service.
Starting LSB: Load kernel image with kexec...
[ OK ] Started Raise network interfaces.
[FAILED] Failed to start LSB: OpenIPMI Driver init script.
See 'systemctl status openipmi.service' for details.
[ OK ] Reached target Network.
Starting Permit User Sessions...
Starting LKP bootstrap...
Starting /etc/rc.local Compatibility...
Starting OpenBSD Secure Shell server...
[ OK ] Started LSB: Load kernel image with kexec.
[ OK ] Started Permit User Sessions.
[ OK ] Started LKP bootstrap.
[ 175.007810] random: crng init done
To reproduce:
# build kernel
cd linux
cp config-5.11.0-rc4-next-20210125-00006-gbd741fb2adf3 .config
make HOSTCC=gcc-9 CC=gcc-9 ARCH=x86_64 olddefconfig prepare modules_prepare bzImage modules
make HOSTCC=gcc-9 CC=gcc-9 ARCH=x86_64 INSTALL_MOD_PATH=<mod-install-dir> modules_install
cd <mod-install-dir>
find lib/ | cpio -o -H newc --quiet | gzip > modules.cgz
git clone https://github.com/intel/lkp-tests.git
cd lkp-tests
bin/lkp qemu -k <bzImage> -m modules.cgz job-script # job-script is attached in this email
Thanks,
Oliver Sang