All of lore.kernel.org
 help / color / mirror / Atom feed
* Re: [PATCH bpf-next] bpf: using rcu_read_lock for bpf_sk_storage_map iterator
@ 2020-09-15 10:53 kernel test robot
  0 siblings, 0 replies; 10+ messages in thread
From: kernel test robot @ 2020-09-15 10:53 UTC (permalink / raw)
  To: kbuild

[-- Attachment #1: Type: text/plain, Size: 7356 bytes --]

CC: kbuild-all(a)lists.01.org
In-Reply-To: <20200914184630.1048718-1-yhs@fb.com>
References: <20200914184630.1048718-1-yhs@fb.com>
TO: Yonghong Song <yhs@fb.com>
TO: bpf(a)vger.kernel.org
TO: netdev(a)vger.kernel.org
CC: Alexei Starovoitov <ast@kernel.org>
CC: Daniel Borkmann <daniel@iogearbox.net>
CC: kernel-team(a)fb.com
CC: Martin KaFai Lau <kafai@fb.com>

Hi Yonghong,

I love your patch! Perhaps something to improve:

[auto build test WARNING on bpf-next/master]

url:    https://github.com/0day-ci/linux/commits/Yonghong-Song/bpf-using-rcu_read_lock-for-bpf_sk_storage_map-iterator/20200915-024727
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
:::::: branch date: 16 hours ago
:::::: commit date: 16 hours ago
config: i386-randconfig-s001-20200914 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.2-191-g10164920-dirty
        # save the attached .config to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)

   net/core/bpf_sk_storage.c: note: in included file (through include/linux/rculist.h):
>> include/linux/rcupdate.h:686:9: sparse: sparse: context imbalance in 'bpf_sk_storage_map_seq_find_next' - unexpected unlock
   include/linux/rcupdate.h:686:9: sparse: sparse: context imbalance in 'bpf_sk_storage_map_seq_stop' - unexpected unlock

# https://github.com/0day-ci/linux/commit/a9b8e045577010eb33328af8e1a226f29c432afd
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Yonghong-Song/bpf-using-rcu_read_lock-for-bpf_sk_storage_map-iterator/20200915-024727
git checkout a9b8e045577010eb33328af8e1a226f29c432afd
vim +/bpf_sk_storage_map_seq_find_next +686 include/linux/rcupdate.h

^1da177e4c3f41 Linus Torvalds      2005-04-16  638  
^1da177e4c3f41 Linus Torvalds      2005-04-16  639  /*
^1da177e4c3f41 Linus Torvalds      2005-04-16  640   * So where is rcu_write_lock()?  It does not exist, as there is no
^1da177e4c3f41 Linus Torvalds      2005-04-16  641   * way for writers to lock out RCU readers.  This is a feature, not
^1da177e4c3f41 Linus Torvalds      2005-04-16  642   * a bug -- this property is what provides RCU's performance benefits.
^1da177e4c3f41 Linus Torvalds      2005-04-16  643   * Of course, writers must coordinate with each other.  The normal
^1da177e4c3f41 Linus Torvalds      2005-04-16  644   * spinlock primitives work well for this, but any other technique may be
^1da177e4c3f41 Linus Torvalds      2005-04-16  645   * used as well.  RCU does not care how the writers keep out of each
^1da177e4c3f41 Linus Torvalds      2005-04-16  646   * others' way, as long as they do so.
^1da177e4c3f41 Linus Torvalds      2005-04-16  647   */
3d76c082907e8f Paul E. McKenney    2009-09-28  648  
3d76c082907e8f Paul E. McKenney    2009-09-28  649  /**
ca5ecddfa8fcbd Paul E. McKenney    2010-04-28  650   * rcu_read_unlock() - marks the end of an RCU read-side critical section.
3d76c082907e8f Paul E. McKenney    2009-09-28  651   *
f27bc4873fa8b7 Paul E. McKenney    2014-05-04  652   * In most situations, rcu_read_unlock() is immune from deadlock.
f27bc4873fa8b7 Paul E. McKenney    2014-05-04  653   * However, in kernels built with CONFIG_RCU_BOOST, rcu_read_unlock()
f27bc4873fa8b7 Paul E. McKenney    2014-05-04  654   * is responsible for deboosting, which it does via rt_mutex_unlock().
f27bc4873fa8b7 Paul E. McKenney    2014-05-04  655   * Unfortunately, this function acquires the scheduler's runqueue and
f27bc4873fa8b7 Paul E. McKenney    2014-05-04  656   * priority-inheritance spinlocks.  This means that deadlock could result
f27bc4873fa8b7 Paul E. McKenney    2014-05-04  657   * if the caller of rcu_read_unlock() already holds one of these locks or
ec84b27f9b3b56 Anna-Maria Gleixner 2018-05-25  658   * any lock that is ever acquired while holding them.
f27bc4873fa8b7 Paul E. McKenney    2014-05-04  659   *
f27bc4873fa8b7 Paul E. McKenney    2014-05-04  660   * That said, RCU readers are never priority boosted unless they were
f27bc4873fa8b7 Paul E. McKenney    2014-05-04  661   * preempted.  Therefore, one way to avoid deadlock is to make sure
f27bc4873fa8b7 Paul E. McKenney    2014-05-04  662   * that preemption never happens within any RCU read-side critical
f27bc4873fa8b7 Paul E. McKenney    2014-05-04  663   * section whose outermost rcu_read_unlock() is called with one of
f27bc4873fa8b7 Paul E. McKenney    2014-05-04  664   * rt_mutex_unlock()'s locks held.  Such preemption can be avoided in
f27bc4873fa8b7 Paul E. McKenney    2014-05-04  665   * a number of ways, for example, by invoking preempt_disable() before
f27bc4873fa8b7 Paul E. McKenney    2014-05-04  666   * critical section's outermost rcu_read_lock().
f27bc4873fa8b7 Paul E. McKenney    2014-05-04  667   *
f27bc4873fa8b7 Paul E. McKenney    2014-05-04  668   * Given that the set of locks acquired by rt_mutex_unlock() might change
f27bc4873fa8b7 Paul E. McKenney    2014-05-04  669   * at any time, a somewhat more future-proofed approach is to make sure
f27bc4873fa8b7 Paul E. McKenney    2014-05-04  670   * that that preemption never happens within any RCU read-side critical
f27bc4873fa8b7 Paul E. McKenney    2014-05-04  671   * section whose outermost rcu_read_unlock() is called with irqs disabled.
f27bc4873fa8b7 Paul E. McKenney    2014-05-04  672   * This approach relies on the fact that rt_mutex_unlock() currently only
f27bc4873fa8b7 Paul E. McKenney    2014-05-04  673   * acquires irq-disabled locks.
f27bc4873fa8b7 Paul E. McKenney    2014-05-04  674   *
f27bc4873fa8b7 Paul E. McKenney    2014-05-04  675   * The second of these two approaches is best in most situations,
f27bc4873fa8b7 Paul E. McKenney    2014-05-04  676   * however, the first approach can also be useful, at least to those
f27bc4873fa8b7 Paul E. McKenney    2014-05-04  677   * developers willing to keep abreast of the set of locks acquired by
f27bc4873fa8b7 Paul E. McKenney    2014-05-04  678   * rt_mutex_unlock().
f27bc4873fa8b7 Paul E. McKenney    2014-05-04  679   *
3d76c082907e8f Paul E. McKenney    2009-09-28  680   * See rcu_read_lock() for more information.
3d76c082907e8f Paul E. McKenney    2009-09-28  681   */
bc33f24bdca8b6 Paul E. McKenney    2009-08-22  682  static inline void rcu_read_unlock(void)
bc33f24bdca8b6 Paul E. McKenney    2009-08-22  683  {
f78f5b90c4ffa5 Paul E. McKenney    2015-06-18  684  	RCU_LOCKDEP_WARN(!rcu_is_watching(),
bde23c6892878e Heiko Carstens      2012-02-01  685  			 "rcu_read_unlock() used illegally while idle");
bc33f24bdca8b6 Paul E. McKenney    2009-08-22 @686  	__release(RCU);
bc33f24bdca8b6 Paul E. McKenney    2009-08-22  687  	__rcu_read_unlock();
d24209bb689e2c Paul E. McKenney    2015-01-21  688  	rcu_lock_release(&rcu_lock_map); /* Keep acq info for rls diags. */
bc33f24bdca8b6 Paul E. McKenney    2009-08-22  689  }
^1da177e4c3f41 Linus Torvalds      2005-04-16  690  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 34566 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next] bpf: using rcu_read_lock for bpf_sk_storage_map iterator
@ 2020-09-15 10:09 kernel test robot
  0 siblings, 0 replies; 10+ messages in thread
From: kernel test robot @ 2020-09-15 10:09 UTC (permalink / raw)
  To: kbuild

[-- Attachment #1: Type: text/plain, Size: 7357 bytes --]

CC: kbuild-all(a)lists.01.org
In-Reply-To: <20200914184630.1048718-1-yhs@fb.com>
References: <20200914184630.1048718-1-yhs@fb.com>
TO: Yonghong Song <yhs@fb.com>
TO: bpf(a)vger.kernel.org
TO: netdev(a)vger.kernel.org
CC: Alexei Starovoitov <ast@kernel.org>
CC: Daniel Borkmann <daniel@iogearbox.net>
CC: kernel-team(a)fb.com
CC: Martin KaFai Lau <kafai@fb.com>

Hi Yonghong,

I love your patch! Perhaps something to improve:

[auto build test WARNING on bpf-next/master]

url:    https://github.com/0day-ci/linux/commits/Yonghong-Song/bpf-using-rcu_read_lock-for-bpf_sk_storage_map-iterator/20200915-024727
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
:::::: branch date: 15 hours ago
:::::: commit date: 15 hours ago
config: x86_64-randconfig-s021-20200914 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.2-191-g10164920-dirty
        # save the attached .config to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)

   net/core/bpf_sk_storage.c:703:25: sparse: sparse: context imbalance in 'bpf_sk_storage_map_seq_find_next' - unexpected unlock
   net/core/bpf_sk_storage.c: note: in included file (through include/linux/rculist.h):
>> include/linux/rcupdate.h:686:9: sparse: sparse: context imbalance in 'bpf_sk_storage_map_seq_stop' - unexpected unlock

# https://github.com/0day-ci/linux/commit/a9b8e045577010eb33328af8e1a226f29c432afd
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Yonghong-Song/bpf-using-rcu_read_lock-for-bpf_sk_storage_map-iterator/20200915-024727
git checkout a9b8e045577010eb33328af8e1a226f29c432afd
vim +/bpf_sk_storage_map_seq_stop +686 include/linux/rcupdate.h

^1da177e4c3f41 Linus Torvalds      2005-04-16  638  
^1da177e4c3f41 Linus Torvalds      2005-04-16  639  /*
^1da177e4c3f41 Linus Torvalds      2005-04-16  640   * So where is rcu_write_lock()?  It does not exist, as there is no
^1da177e4c3f41 Linus Torvalds      2005-04-16  641   * way for writers to lock out RCU readers.  This is a feature, not
^1da177e4c3f41 Linus Torvalds      2005-04-16  642   * a bug -- this property is what provides RCU's performance benefits.
^1da177e4c3f41 Linus Torvalds      2005-04-16  643   * Of course, writers must coordinate with each other.  The normal
^1da177e4c3f41 Linus Torvalds      2005-04-16  644   * spinlock primitives work well for this, but any other technique may be
^1da177e4c3f41 Linus Torvalds      2005-04-16  645   * used as well.  RCU does not care how the writers keep out of each
^1da177e4c3f41 Linus Torvalds      2005-04-16  646   * others' way, as long as they do so.
^1da177e4c3f41 Linus Torvalds      2005-04-16  647   */
3d76c082907e8f Paul E. McKenney    2009-09-28  648  
3d76c082907e8f Paul E. McKenney    2009-09-28  649  /**
ca5ecddfa8fcbd Paul E. McKenney    2010-04-28  650   * rcu_read_unlock() - marks the end of an RCU read-side critical section.
3d76c082907e8f Paul E. McKenney    2009-09-28  651   *
f27bc4873fa8b7 Paul E. McKenney    2014-05-04  652   * In most situations, rcu_read_unlock() is immune from deadlock.
f27bc4873fa8b7 Paul E. McKenney    2014-05-04  653   * However, in kernels built with CONFIG_RCU_BOOST, rcu_read_unlock()
f27bc4873fa8b7 Paul E. McKenney    2014-05-04  654   * is responsible for deboosting, which it does via rt_mutex_unlock().
f27bc4873fa8b7 Paul E. McKenney    2014-05-04  655   * Unfortunately, this function acquires the scheduler's runqueue and
f27bc4873fa8b7 Paul E. McKenney    2014-05-04  656   * priority-inheritance spinlocks.  This means that deadlock could result
f27bc4873fa8b7 Paul E. McKenney    2014-05-04  657   * if the caller of rcu_read_unlock() already holds one of these locks or
ec84b27f9b3b56 Anna-Maria Gleixner 2018-05-25  658   * any lock that is ever acquired while holding them.
f27bc4873fa8b7 Paul E. McKenney    2014-05-04  659   *
f27bc4873fa8b7 Paul E. McKenney    2014-05-04  660   * That said, RCU readers are never priority boosted unless they were
f27bc4873fa8b7 Paul E. McKenney    2014-05-04  661   * preempted.  Therefore, one way to avoid deadlock is to make sure
f27bc4873fa8b7 Paul E. McKenney    2014-05-04  662   * that preemption never happens within any RCU read-side critical
f27bc4873fa8b7 Paul E. McKenney    2014-05-04  663   * section whose outermost rcu_read_unlock() is called with one of
f27bc4873fa8b7 Paul E. McKenney    2014-05-04  664   * rt_mutex_unlock()'s locks held.  Such preemption can be avoided in
f27bc4873fa8b7 Paul E. McKenney    2014-05-04  665   * a number of ways, for example, by invoking preempt_disable() before
f27bc4873fa8b7 Paul E. McKenney    2014-05-04  666   * critical section's outermost rcu_read_lock().
f27bc4873fa8b7 Paul E. McKenney    2014-05-04  667   *
f27bc4873fa8b7 Paul E. McKenney    2014-05-04  668   * Given that the set of locks acquired by rt_mutex_unlock() might change
f27bc4873fa8b7 Paul E. McKenney    2014-05-04  669   * at any time, a somewhat more future-proofed approach is to make sure
f27bc4873fa8b7 Paul E. McKenney    2014-05-04  670   * that that preemption never happens within any RCU read-side critical
f27bc4873fa8b7 Paul E. McKenney    2014-05-04  671   * section whose outermost rcu_read_unlock() is called with irqs disabled.
f27bc4873fa8b7 Paul E. McKenney    2014-05-04  672   * This approach relies on the fact that rt_mutex_unlock() currently only
f27bc4873fa8b7 Paul E. McKenney    2014-05-04  673   * acquires irq-disabled locks.
f27bc4873fa8b7 Paul E. McKenney    2014-05-04  674   *
f27bc4873fa8b7 Paul E. McKenney    2014-05-04  675   * The second of these two approaches is best in most situations,
f27bc4873fa8b7 Paul E. McKenney    2014-05-04  676   * however, the first approach can also be useful, at least to those
f27bc4873fa8b7 Paul E. McKenney    2014-05-04  677   * developers willing to keep abreast of the set of locks acquired by
f27bc4873fa8b7 Paul E. McKenney    2014-05-04  678   * rt_mutex_unlock().
f27bc4873fa8b7 Paul E. McKenney    2014-05-04  679   *
3d76c082907e8f Paul E. McKenney    2009-09-28  680   * See rcu_read_lock() for more information.
3d76c082907e8f Paul E. McKenney    2009-09-28  681   */
bc33f24bdca8b6 Paul E. McKenney    2009-08-22  682  static inline void rcu_read_unlock(void)
bc33f24bdca8b6 Paul E. McKenney    2009-08-22  683  {
f78f5b90c4ffa5 Paul E. McKenney    2015-06-18  684  	RCU_LOCKDEP_WARN(!rcu_is_watching(),
bde23c6892878e Heiko Carstens      2012-02-01  685  			 "rcu_read_unlock() used illegally while idle");
bc33f24bdca8b6 Paul E. McKenney    2009-08-22 @686  	__release(RCU);
bc33f24bdca8b6 Paul E. McKenney    2009-08-22  687  	__rcu_read_unlock();
d24209bb689e2c Paul E. McKenney    2015-01-21  688  	rcu_lock_release(&rcu_lock_map); /* Keep acq info for rls diags. */
bc33f24bdca8b6 Paul E. McKenney    2009-08-22  689  }
^1da177e4c3f41 Linus Torvalds      2005-04-16  690  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 35510 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread
* [PATCH bpf-next] bpf: using rcu_read_lock for bpf_sk_storage_map iterator
@ 2020-09-14 18:46 Yonghong Song
  2020-09-14 21:28 ` Song Liu
  2020-09-15 15:33 ` Jakub Kicinski
  0 siblings, 2 replies; 10+ messages in thread
From: Yonghong Song @ 2020-09-14 18:46 UTC (permalink / raw)
  To: bpf, netdev
  Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team, Martin KaFai Lau

Currently, we use bucket_lock when traversing bpf_sk_storage_map
elements. Since bpf_iter programs cannot use bpf_sk_storage_get()
and bpf_sk_storage_delete() helpers which may also grab bucket lock,
we do not have a deadlock issue which exists for hashmap when
using bucket_lock ([1]).

If a bucket contains a lot of sockets, during bpf_iter traversing
a bucket, concurrent bpf_sk_storage_{get,delete}() may experience
some undesirable delays. Using rcu_read_lock() is a reasonable
compromise here. Although it may lose some precision, e.g.,
access stale sockets, but it will not hurt performance of other
bpf programs.

[1] https://lore.kernel.org/bpf/20200902235341.2001534-1-yhs@fb.com

Cc: Martin KaFai Lau <kafai@fb.com>
Signed-off-by: Yonghong Song <yhs@fb.com>
---
 net/core/bpf_sk_storage.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/net/core/bpf_sk_storage.c b/net/core/bpf_sk_storage.c
index 4a86ea34f29e..a1db5e988d19 100644
--- a/net/core/bpf_sk_storage.c
+++ b/net/core/bpf_sk_storage.c
@@ -701,7 +701,7 @@ bpf_sk_storage_map_seq_find_next(struct bpf_iter_seq_sk_storage_map_info *info,
 		if (!selem) {
 			/* not found, unlock and go to the next bucket */
 			b = &smap->buckets[bucket_id++];
-			raw_spin_unlock_bh(&b->lock);
+			rcu_read_unlock();
 			skip_elems = 0;
 			break;
 		}
@@ -715,7 +715,7 @@ bpf_sk_storage_map_seq_find_next(struct bpf_iter_seq_sk_storage_map_info *info,
 
 	for (i = bucket_id; i < (1U << smap->bucket_log); i++) {
 		b = &smap->buckets[i];
-		raw_spin_lock_bh(&b->lock);
+		rcu_read_lock();
 		count = 0;
 		hlist_for_each_entry(selem, &b->list, map_node) {
 			sk_storage = rcu_dereference_raw(selem->local_storage);
@@ -726,7 +726,7 @@ bpf_sk_storage_map_seq_find_next(struct bpf_iter_seq_sk_storage_map_info *info,
 			}
 			count++;
 		}
-		raw_spin_unlock_bh(&b->lock);
+		rcu_read_unlock();
 		skip_elems = 0;
 	}
 
@@ -806,13 +806,10 @@ static void bpf_sk_storage_map_seq_stop(struct seq_file *seq, void *v)
 	struct bpf_local_storage_map *smap;
 	struct bpf_local_storage_map_bucket *b;
 
-	if (!v) {
+	if (!v)
 		(void)__bpf_sk_storage_map_seq_show(seq, v);
-	} else {
-		smap = (struct bpf_local_storage_map *)info->map;
-		b = &smap->buckets[info->bucket_id];
-		raw_spin_unlock_bh(&b->lock);
-	}
+	else
+		rcu_read_unlock();
 }
 
 static int bpf_iter_init_sk_storage_map(void *priv_data,
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2020-09-15 22:47 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-15 10:53 [PATCH bpf-next] bpf: using rcu_read_lock for bpf_sk_storage_map iterator kernel test robot
  -- strict thread matches above, loose matches on Subject: below --
2020-09-15 10:09 kernel test robot
2020-09-14 18:46 Yonghong Song
2020-09-14 21:28 ` Song Liu
2020-09-15  5:25   ` Yonghong Song
2020-09-15 15:33 ` Jakub Kicinski
2020-09-15 17:35   ` Yonghong Song
2020-09-15 17:40     ` Jakub Kicinski
2020-09-15 18:56       ` Yonghong Song
2020-09-15 19:03         ` Alexei Starovoitov

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.