* [PATCH] srcu: Add detection of boot CPU srcu_data structure's->srcu_cblist @ 2022-11-17 11:20 Zqiang 2022-11-17 14:20 ` Frederic Weisbecker 0 siblings, 1 reply; 9+ messages in thread From: Zqiang @ 2022-11-17 11:20 UTC (permalink / raw) To: paulmck, frederic, joel; +Cc: rcu, linux-kernel Before SRCU_SIZE_WAIT_CALL srcu_size_state is reached, the srcu callback only insert to boot-CPU srcu_data structure's->srcu_cblist and other CPU srcu_data structure's->srcu_cblist is always empty. so before SRCU_SIZE_WAIT_CALL is reached, need to correctly check boot-CPU pend cbs in srcu_might_be_idle(). Signed-off-by: Zqiang <qiang1.zhang@intel.com> --- kernel/rcu/srcutree.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c index 6af031200580..6d9af9901765 100644 --- a/kernel/rcu/srcutree.c +++ b/kernel/rcu/srcutree.c @@ -1098,8 +1098,11 @@ static bool srcu_might_be_idle(struct srcu_struct *ssp) unsigned long tlast; check_init_srcu_struct(ssp); - /* If the local srcu_data structure has callbacks, not idle. */ - sdp = raw_cpu_ptr(ssp->sda); + /* If the boot CPU or local srcu_data structure has callbacks, not idle. */ + if (smp_load_acquire(&ssp->srcu_size_state) < SRCU_SIZE_WAIT_CALL) + sdp = per_cpu_ptr(ssp->sda, get_boot_cpu_id()); + else + sdp = raw_cpu_ptr(ssp->sda); spin_lock_irqsave_rcu_node(sdp, flags); if (rcu_segcblist_pend_cbs(&sdp->srcu_cblist)) { spin_unlock_irqrestore_rcu_node(sdp, flags); -- 2.25.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] srcu: Add detection of boot CPU srcu_data structure's->srcu_cblist 2022-11-17 11:20 [PATCH] srcu: Add detection of boot CPU srcu_data structure's->srcu_cblist Zqiang @ 2022-11-17 14:20 ` Frederic Weisbecker 2022-11-18 4:53 ` Zhang, Qiang1 2022-11-28 8:32 ` Pingfan Liu 0 siblings, 2 replies; 9+ messages in thread From: Frederic Weisbecker @ 2022-11-17 14:20 UTC (permalink / raw) To: Zqiang; +Cc: paulmck, joel, rcu, linux-kernel On Thu, Nov 17, 2022 at 07:20:50PM +0800, Zqiang wrote: > Before SRCU_SIZE_WAIT_CALL srcu_size_state is reached, the srcu > callback only insert to boot-CPU srcu_data structure's->srcu_cblist > and other CPU srcu_data structure's->srcu_cblist is always empty. so > before SRCU_SIZE_WAIT_CALL is reached, need to correctly check boot-CPU > pend cbs in srcu_might_be_idle(). > > Signed-off-by: Zqiang <qiang1.zhang@intel.com> > --- > kernel/rcu/srcutree.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c > index 6af031200580..6d9af9901765 100644 > --- a/kernel/rcu/srcutree.c > +++ b/kernel/rcu/srcutree.c > @@ -1098,8 +1098,11 @@ static bool srcu_might_be_idle(struct srcu_struct *ssp) > unsigned long tlast; > > check_init_srcu_struct(ssp); > - /* If the local srcu_data structure has callbacks, not idle. */ > - sdp = raw_cpu_ptr(ssp->sda); > + /* If the boot CPU or local srcu_data structure has callbacks, not idle. */ > + if (smp_load_acquire(&ssp->srcu_size_state) < SRCU_SIZE_WAIT_CALL) > + sdp = per_cpu_ptr(ssp->sda, get_boot_cpu_id()); > + else > + sdp = raw_cpu_ptr(ssp->sda); While at it if someone is interested in documenting/commenting on the meaning of all these SRCU_SIZE_* things, it would be much appreciated! Thanks. > spin_lock_irqsave_rcu_node(sdp, flags); > if (rcu_segcblist_pend_cbs(&sdp->srcu_cblist)) { > spin_unlock_irqrestore_rcu_node(sdp, flags); > -- > 2.25.1 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH] srcu: Add detection of boot CPU srcu_data structure's->srcu_cblist 2022-11-17 14:20 ` Frederic Weisbecker @ 2022-11-18 4:53 ` Zhang, Qiang1 2022-11-23 2:01 ` Zhang, Qiang1 2022-11-28 8:32 ` Pingfan Liu 1 sibling, 1 reply; 9+ messages in thread From: Zhang, Qiang1 @ 2022-11-18 4:53 UTC (permalink / raw) To: Frederic Weisbecker, paulmck; +Cc: joel, rcu, linux-kernel On Thu, Nov 17, 2022 at 07:20:50PM +0800, Zqiang wrote: > Before SRCU_SIZE_WAIT_CALL srcu_size_state is reached, the srcu > callback only insert to boot-CPU srcu_data structure's->srcu_cblist > and other CPU srcu_data structure's->srcu_cblist is always empty. so > before SRCU_SIZE_WAIT_CALL is reached, need to correctly check boot-CPU > pend cbs in srcu_might_be_idle(). > > Signed-off-by: Zqiang <qiang1.zhang@intel.com> > --- > kernel/rcu/srcutree.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c > index 6af031200580..6d9af9901765 100644 > --- a/kernel/rcu/srcutree.c > +++ b/kernel/rcu/srcutree.c > @@ -1098,8 +1098,11 @@ static bool srcu_might_be_idle(struct srcu_struct *ssp) > unsigned long tlast; > > check_init_srcu_struct(ssp); > - /* If the local srcu_data structure has callbacks, not idle. */ > - sdp = raw_cpu_ptr(ssp->sda); > + /* If the boot CPU or local srcu_data structure has callbacks, not idle. */ > + if (smp_load_acquire(&ssp->srcu_size_state) < SRCU_SIZE_WAIT_CALL) > + sdp = per_cpu_ptr(ssp->sda, get_boot_cpu_id()); > + else > + sdp = raw_cpu_ptr(ssp->sda); > >While at it if someone is interested in documenting/commenting on the meaning of >all these SRCU_SIZE_* things, it would be much appreciated! In the initial stage(SRCU_SIZE_SMALL), there is no srcu_node structures, only initialized per-CPU srcu_data structures, at this time, we only use boot-cpu srcu_data structures's ->srcu_cblist to store srcu callbacks. if the contention of srcu_struct and srcu_data structure's->lock become busy, transition to SRCU_SIZE_ALLOC. allocated memory for srcu_node structure at end of the SRCU grace period. if allocated success, transition to SRCU_SIZE_WAIT_BARRIER, in this state, invoke srcu_barrier() will iterate over all possible CPUs, but we still only use boot-CPU srcu_cblist to store callbacks. the task calling call_srcu() may have access to a new srcu_node structure or may not, because call_srcu() is protected by SRCU, we may not transition to SRCU_SIZE_WAIT_CALL quickly, need to wait in this state for a SRCU grace period to end. After transition to SRCU_SIZE_WAIT_CALL, we enqueue srcu callbacks to own srcu_data structures's ->srcu_cblist. Does my description make my commit more detailed? Thanks Zqiang > >Thanks. > > spin_lock_irqsave_rcu_node(sdp, flags); > if (rcu_segcblist_pend_cbs(&sdp->srcu_cblist)) { > spin_unlock_irqrestore_rcu_node(sdp, flags); > -- > 2.25.1 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH] srcu: Add detection of boot CPU srcu_data structure's->srcu_cblist 2022-11-18 4:53 ` Zhang, Qiang1 @ 2022-11-23 2:01 ` Zhang, Qiang1 2022-11-23 19:04 ` Paul E. McKenney 0 siblings, 1 reply; 9+ messages in thread From: Zhang, Qiang1 @ 2022-11-23 2:01 UTC (permalink / raw) To: Zhang, Qiang1, Frederic Weisbecker, paulmck; +Cc: joel, rcu, linux-kernel >On Thu, Nov 17, 2022 at 07:20:50PM +0800, Zqiang wrote: > Before SRCU_SIZE_WAIT_CALL srcu_size_state is reached, the srcu > callback only insert to boot-CPU srcu_data structure's->srcu_cblist > and other CPU srcu_data structure's->srcu_cblist is always empty. so > before SRCU_SIZE_WAIT_CALL is reached, need to correctly check boot-CPU > pend cbs in srcu_might_be_idle(). > > Signed-off-by: Zqiang <qiang1.zhang@intel.com> > --- > kernel/rcu/srcutree.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c > index 6af031200580..6d9af9901765 100644 > --- a/kernel/rcu/srcutree.c > +++ b/kernel/rcu/srcutree.c > @@ -1098,8 +1098,11 @@ static bool srcu_might_be_idle(struct srcu_struct *ssp) > unsigned long tlast; > > check_init_srcu_struct(ssp); > - /* If the local srcu_data structure has callbacks, not idle. */ > - sdp = raw_cpu_ptr(ssp->sda); > + /* If the boot CPU or local srcu_data structure has callbacks, not idle. */ > + if (smp_load_acquire(&ssp->srcu_size_state) < SRCU_SIZE_WAIT_CALL) > + sdp = per_cpu_ptr(ssp->sda, get_boot_cpu_id()); > + else > + sdp = raw_cpu_ptr(ssp->sda); > Hi Paul, For the convert_to_big default configuration(SRCU_SIZING_AUTO), I found that the srcu is in SRCU_SIZE_SMALL state most of the time in the system, I think this change is meaningful. Thoughts ? Thanks Zqiang >While at it if someone is interested in documenting/commenting on the meaning of >all these SRCU_SIZE_* things, it would be much appreciated! > >In the initial stage(SRCU_SIZE_SMALL), there is no srcu_node structures, only initialized >per-CPU srcu_data structures, at this time, we only use boot-cpu srcu_data structures's ->srcu_cblist >to store srcu callbacks. >if the contention of srcu_struct and srcu_data structure's->lock become busy, >transition to SRCU_SIZE_ALLOC. allocated memory for srcu_node structure at end of the SRCU >grace period. >if allocated success, transition to SRCU_SIZE_WAIT_BARRIER, in this state, invoke >srcu_barrier() will iterate over all possible CPUs, but we still only use boot-CPU srcu_cblist to store callbacks. >the task calling call_srcu() may have access to a new srcu_node structure or may not, >because call_srcu() is protected by SRCU, we may not transition to SRCU_SIZE_WAIT_CALL quickly, >need to wait in this state for a SRCU grace period to end. >After transition to SRCU_SIZE_WAIT_CALL, we enqueue srcu callbacks to own srcu_data structures's ->srcu_cblist. > >Does my description make my commit more detailed? > >Thanks >Zqiang > > > >Thanks. > > spin_lock_irqsave_rcu_node(sdp, flags); > if (rcu_segcblist_pend_cbs(&sdp->srcu_cblist)) { > spin_unlock_irqrestore_rcu_node(sdp, flags); > -- > 2.25.1 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] srcu: Add detection of boot CPU srcu_data structure's->srcu_cblist 2022-11-23 2:01 ` Zhang, Qiang1 @ 2022-11-23 19:04 ` Paul E. McKenney 2022-11-24 1:43 ` Zhang, Qiang1 0 siblings, 1 reply; 9+ messages in thread From: Paul E. McKenney @ 2022-11-23 19:04 UTC (permalink / raw) To: Zhang, Qiang1; +Cc: Frederic Weisbecker, joel, rcu, linux-kernel On Wed, Nov 23, 2022 at 02:01:50AM +0000, Zhang, Qiang1 wrote: > > >On Thu, Nov 17, 2022 at 07:20:50PM +0800, Zqiang wrote: > > Before SRCU_SIZE_WAIT_CALL srcu_size_state is reached, the srcu > > callback only insert to boot-CPU srcu_data structure's->srcu_cblist > > and other CPU srcu_data structure's->srcu_cblist is always empty. so > > before SRCU_SIZE_WAIT_CALL is reached, need to correctly check boot-CPU > > pend cbs in srcu_might_be_idle(). > > > > Signed-off-by: Zqiang <qiang1.zhang@intel.com> > > --- > > kernel/rcu/srcutree.c | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c > > index 6af031200580..6d9af9901765 100644 > > --- a/kernel/rcu/srcutree.c > > +++ b/kernel/rcu/srcutree.c > > @@ -1098,8 +1098,11 @@ static bool srcu_might_be_idle(struct srcu_struct *ssp) > > unsigned long tlast; > > > > check_init_srcu_struct(ssp); > > - /* If the local srcu_data structure has callbacks, not idle. */ > > - sdp = raw_cpu_ptr(ssp->sda); > > + /* If the boot CPU or local srcu_data structure has callbacks, not idle. */ > > + if (smp_load_acquire(&ssp->srcu_size_state) < SRCU_SIZE_WAIT_CALL) > > + sdp = per_cpu_ptr(ssp->sda, get_boot_cpu_id()); > > + else > > + sdp = raw_cpu_ptr(ssp->sda); > > > > Hi Paul, > > For the convert_to_big default configuration(SRCU_SIZING_AUTO), I found that the srcu is in > SRCU_SIZE_SMALL state most of the time in the system, I think this change is meaningful. > > Thoughts ? You are right that this change might make srcu_might_be_idle() return a more accurate value in the common case. As things are now, some other CPU might just now have added a callback, but might not yet have started that new grace period. In that case, we might expedite an SRCU grace period when we would not have otherwise done so. However, this change would also increase contention on the get_boot_cpu_id() CPU's srcu_data structure's ->lock. So the result of that inaccurate return value is that the first two SRCU grace periods in a burst might be expedited instead of only the first one, and even then only if we hit a very narrow race window. Besides, this same thing can happen if two CPUs do synchronize_srcu() at the same time after a long-enough pause between grace periods. Both see no callbacks, so both ask for an expedited grace period. So again, the first two grace periods are expedited. As a result, I am having a very hard time justifying the increased lock contention. Am I missing something here? Thanx, Paul > Thanks > Zqiang > > > >While at it if someone is interested in documenting/commenting on the meaning of > >all these SRCU_SIZE_* things, it would be much appreciated! > > > >In the initial stage(SRCU_SIZE_SMALL), there is no srcu_node structures, only initialized > >per-CPU srcu_data structures, at this time, we only use boot-cpu srcu_data structures's ->srcu_cblist > >to store srcu callbacks. > >if the contention of srcu_struct and srcu_data structure's->lock become busy, > >transition to SRCU_SIZE_ALLOC. allocated memory for srcu_node structure at end of the SRCU > >grace period. > >if allocated success, transition to SRCU_SIZE_WAIT_BARRIER, in this state, invoke > >srcu_barrier() will iterate over all possible CPUs, but we still only use boot-CPU srcu_cblist to store callbacks. > >the task calling call_srcu() may have access to a new srcu_node structure or may not, > >because call_srcu() is protected by SRCU, we may not transition to SRCU_SIZE_WAIT_CALL quickly, > >need to wait in this state for a SRCU grace period to end. > >After transition to SRCU_SIZE_WAIT_CALL, we enqueue srcu callbacks to own srcu_data structures's ->srcu_cblist. > > > >Does my description make my commit more detailed? > > > >Thanks > >Zqiang > > > > > > > > > >Thanks. > > > > spin_lock_irqsave_rcu_node(sdp, flags); > > if (rcu_segcblist_pend_cbs(&sdp->srcu_cblist)) { > > spin_unlock_irqrestore_rcu_node(sdp, flags); > > -- > > 2.25.1 > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH] srcu: Add detection of boot CPU srcu_data structure's->srcu_cblist 2022-11-23 19:04 ` Paul E. McKenney @ 2022-11-24 1:43 ` Zhang, Qiang1 2022-11-24 5:25 ` Paul E. McKenney 0 siblings, 1 reply; 9+ messages in thread From: Zhang, Qiang1 @ 2022-11-24 1:43 UTC (permalink / raw) To: paulmck; +Cc: Frederic Weisbecker, joel, rcu, linux-kernel On Wed, Nov 23, 2022 at 02:01:50AM +0000, Zhang, Qiang1 wrote: > > >On Thu, Nov 17, 2022 at 07:20:50PM +0800, Zqiang wrote: > > Before SRCU_SIZE_WAIT_CALL srcu_size_state is reached, the srcu > > callback only insert to boot-CPU srcu_data structure's->srcu_cblist > > and other CPU srcu_data structure's->srcu_cblist is always empty. so > > before SRCU_SIZE_WAIT_CALL is reached, need to correctly check boot-CPU > > pend cbs in srcu_might_be_idle(). > > > > Signed-off-by: Zqiang <qiang1.zhang@intel.com> > > --- > > kernel/rcu/srcutree.c | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c > > index 6af031200580..6d9af9901765 100644 > > --- a/kernel/rcu/srcutree.c > > +++ b/kernel/rcu/srcutree.c > > @@ -1098,8 +1098,11 @@ static bool srcu_might_be_idle(struct srcu_struct *ssp) > > unsigned long tlast; > > > > check_init_srcu_struct(ssp); > > - /* If the local srcu_data structure has callbacks, not idle. */ > > - sdp = raw_cpu_ptr(ssp->sda); > > + /* If the boot CPU or local srcu_data structure has callbacks, not idle. */ > > + if (smp_load_acquire(&ssp->srcu_size_state) < SRCU_SIZE_WAIT_CALL) > > + sdp = per_cpu_ptr(ssp->sda, get_boot_cpu_id()); > > + else > > + sdp = raw_cpu_ptr(ssp->sda); > > > > Hi Paul, > > For the convert_to_big default configuration(SRCU_SIZING_AUTO), I found that the srcu is in > SRCU_SIZE_SMALL state most of the time in the system, I think this change is meaningful. > > Thoughts ? >You are right that this change might make srcu_might_be_idle() return a >more accurate value in the common case. As things are now, some other >CPU might just now have added a callback, but might not yet have started >that new grace period. In that case, we might expedite an SRCU grace >period when we would not have otherwise done so. However, this change >would also increase contention on the get_boot_cpu_id() CPU's srcu_data >structure's ->lock. > >So the result of that inaccurate return value is that the first two SRCU >grace periods in a burst might be expedited instead of only the first one, >and even then only if we hit a very narrow race window. > >Besides, this same thing can happen if two CPUs do synchronize_srcu() >at the same time after a long-enough pause between grace periods. >Both see no callbacks, so both ask for an expedited grace period. >So again, the first two grace periods are expedited. > >As a result, I am having a very hard time justifying the increased >lock contention. Thanks reply, I have another question, Is this srcu_data structure's->lock necessary? the rcu_segcblist_pend_cbs() only check *tails[RCU_DONE_TAIL] is empty. or can use rcu_segcblist_get_seglen(RCU_WAIT_TAIL + RCU_NEXT_READY_TAIL + RCU_NEXT_TAIL) instead of rcu_segcblist_pend_cbs() to avoid locking? (although this is also inaccurate) Thanks Zqiang > >Am I missing something here? > > Thanx, Paul > > Thanks > Zqiang > > > >While at it if someone is interested in documenting/commenting on the meaning of > >all these SRCU_SIZE_* things, it would be much appreciated! > > > >In the initial stage(SRCU_SIZE_SMALL), there is no srcu_node structures, only initialized > >per-CPU srcu_data structures, at this time, we only use boot-cpu srcu_data structures's ->srcu_cblist > >to store srcu callbacks. > >if the contention of srcu_struct and srcu_data structure's->lock become busy, > >transition to SRCU_SIZE_ALLOC. allocated memory for srcu_node structure at end of the SRCU > >grace period. > >if allocated success, transition to SRCU_SIZE_WAIT_BARRIER, in this state, invoke > >srcu_barrier() will iterate over all possible CPUs, but we still only use boot-CPU srcu_cblist to store callbacks. > >the task calling call_srcu() may have access to a new srcu_node structure or may not, > >because call_srcu() is protected by SRCU, we may not transition to SRCU_SIZE_WAIT_CALL quickly, > >need to wait in this state for a SRCU grace period to end. > >After transition to SRCU_SIZE_WAIT_CALL, we enqueue srcu callbacks to own srcu_data structures's ->srcu_cblist. > > > >Does my description make my commit more detailed? > > > >Thanks > >Zqiang > > > > > > > > > >Thanks. > > > > spin_lock_irqsave_rcu_node(sdp, flags); > > if (rcu_segcblist_pend_cbs(&sdp->srcu_cblist)) { > > spin_unlock_irqrestore_rcu_node(sdp, flags); > > -- > > 2.25.1 > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] srcu: Add detection of boot CPU srcu_data structure's->srcu_cblist 2022-11-24 1:43 ` Zhang, Qiang1 @ 2022-11-24 5:25 ` Paul E. McKenney 2022-11-24 5:59 ` Zhang, Qiang1 0 siblings, 1 reply; 9+ messages in thread From: Paul E. McKenney @ 2022-11-24 5:25 UTC (permalink / raw) To: Zhang, Qiang1; +Cc: Frederic Weisbecker, joel, rcu, linux-kernel On Thu, Nov 24, 2022 at 01:43:42AM +0000, Zhang, Qiang1 wrote: > On Wed, Nov 23, 2022 at 02:01:50AM +0000, Zhang, Qiang1 wrote: > > > > >On Thu, Nov 17, 2022 at 07:20:50PM +0800, Zqiang wrote: > > > Before SRCU_SIZE_WAIT_CALL srcu_size_state is reached, the srcu > > > callback only insert to boot-CPU srcu_data structure's->srcu_cblist > > > and other CPU srcu_data structure's->srcu_cblist is always empty. so > > > before SRCU_SIZE_WAIT_CALL is reached, need to correctly check boot-CPU > > > pend cbs in srcu_might_be_idle(). > > > > > > Signed-off-by: Zqiang <qiang1.zhang@intel.com> > > > --- > > > kernel/rcu/srcutree.c | 7 +++++-- > > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c > > > index 6af031200580..6d9af9901765 100644 > > > --- a/kernel/rcu/srcutree.c > > > +++ b/kernel/rcu/srcutree.c > > > @@ -1098,8 +1098,11 @@ static bool srcu_might_be_idle(struct srcu_struct *ssp) > > > unsigned long tlast; > > > > > > check_init_srcu_struct(ssp); > > > - /* If the local srcu_data structure has callbacks, not idle. */ > > > - sdp = raw_cpu_ptr(ssp->sda); > > > + /* If the boot CPU or local srcu_data structure has callbacks, not idle. */ > > > + if (smp_load_acquire(&ssp->srcu_size_state) < SRCU_SIZE_WAIT_CALL) > > > + sdp = per_cpu_ptr(ssp->sda, get_boot_cpu_id()); > > > + else > > > + sdp = raw_cpu_ptr(ssp->sda); > > > > > > > Hi Paul, > > > > For the convert_to_big default configuration(SRCU_SIZING_AUTO), I found that the srcu is in > > SRCU_SIZE_SMALL state most of the time in the system, I think this change is meaningful. > > > > Thoughts ? > > >You are right that this change might make srcu_might_be_idle() return a > >more accurate value in the common case. As things are now, some other > >CPU might just now have added a callback, but might not yet have started > >that new grace period. In that case, we might expedite an SRCU grace > >period when we would not have otherwise done so. However, this change > >would also increase contention on the get_boot_cpu_id() CPU's srcu_data > >structure's ->lock. > > > >So the result of that inaccurate return value is that the first two SRCU > >grace periods in a burst might be expedited instead of only the first one, > >and even then only if we hit a very narrow race window. > > > >Besides, this same thing can happen if two CPUs do synchronize_srcu() > >at the same time after a long-enough pause between grace periods. > >Both see no callbacks, so both ask for an expedited grace period. > >So again, the first two grace periods are expedited. > > > >As a result, I am having a very hard time justifying the increased > >lock contention. > > Thanks reply, I have another question, Is this srcu_data structure's->lock necessary? > the rcu_segcblist_pend_cbs() only check *tails[RCU_DONE_TAIL] is empty. > or can use rcu_segcblist_get_seglen(RCU_WAIT_TAIL + RCU_NEXT_READY_TAIL + RCU_NEXT_TAIL) > instead of rcu_segcblist_pend_cbs() to avoid locking? (although this is also inaccurate) That extra "*" means that the lock must be acquired. Otherwise, the pointed-to callback might even be unmapped between the time we fetched the pointer and the time we dereferenced it. Thanx, Paul. > Thanks > Zqiang > > > > >Am I missing something here? > > > > Thanx, Paul > > > > Thanks > > Zqiang > > > > > > >While at it if someone is interested in documenting/commenting on the meaning of > > >all these SRCU_SIZE_* things, it would be much appreciated! > > > > > >In the initial stage(SRCU_SIZE_SMALL), there is no srcu_node structures, only initialized > > >per-CPU srcu_data structures, at this time, we only use boot-cpu srcu_data structures's ->srcu_cblist > > >to store srcu callbacks. > > >if the contention of srcu_struct and srcu_data structure's->lock become busy, > > >transition to SRCU_SIZE_ALLOC. allocated memory for srcu_node structure at end of the SRCU > > >grace period. > > >if allocated success, transition to SRCU_SIZE_WAIT_BARRIER, in this state, invoke > > >srcu_barrier() will iterate over all possible CPUs, but we still only use boot-CPU srcu_cblist to store callbacks. > > >the task calling call_srcu() may have access to a new srcu_node structure or may not, > > >because call_srcu() is protected by SRCU, we may not transition to SRCU_SIZE_WAIT_CALL quickly, > > >need to wait in this state for a SRCU grace period to end. > > >After transition to SRCU_SIZE_WAIT_CALL, we enqueue srcu callbacks to own srcu_data structures's ->srcu_cblist. > > > > > >Does my description make my commit more detailed? > > > > > >Thanks > > >Zqiang > > > > > > > > > > > > > > > >Thanks. > > > > > > spin_lock_irqsave_rcu_node(sdp, flags); > > > if (rcu_segcblist_pend_cbs(&sdp->srcu_cblist)) { > > > spin_unlock_irqrestore_rcu_node(sdp, flags); > > > -- > > > 2.25.1 > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH] srcu: Add detection of boot CPU srcu_data structure's->srcu_cblist 2022-11-24 5:25 ` Paul E. McKenney @ 2022-11-24 5:59 ` Zhang, Qiang1 0 siblings, 0 replies; 9+ messages in thread From: Zhang, Qiang1 @ 2022-11-24 5:59 UTC (permalink / raw) To: paulmck; +Cc: Frederic Weisbecker, joel, rcu, linux-kernel On Thu, Nov 24, 2022 at 01:43:42AM +0000, Zhang, Qiang1 wrote: > On Wed, Nov 23, 2022 at 02:01:50AM +0000, Zhang, Qiang1 wrote: > > > > >On Thu, Nov 17, 2022 at 07:20:50PM +0800, Zqiang wrote: > > > Before SRCU_SIZE_WAIT_CALL srcu_size_state is reached, the srcu > > > callback only insert to boot-CPU srcu_data structure's->srcu_cblist > > > and other CPU srcu_data structure's->srcu_cblist is always empty. so > > > before SRCU_SIZE_WAIT_CALL is reached, need to correctly check boot-CPU > > > pend cbs in srcu_might_be_idle(). > > > > > > Signed-off-by: Zqiang <qiang1.zhang@intel.com> > > > --- > > > kernel/rcu/srcutree.c | 7 +++++-- > > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c > > > index 6af031200580..6d9af9901765 100644 > > > --- a/kernel/rcu/srcutree.c > > > +++ b/kernel/rcu/srcutree.c > > > @@ -1098,8 +1098,11 @@ static bool srcu_might_be_idle(struct srcu_struct *ssp) > > > unsigned long tlast; > > > > > > check_init_srcu_struct(ssp); > > > - /* If the local srcu_data structure has callbacks, not idle. */ > > > - sdp = raw_cpu_ptr(ssp->sda); > > > + /* If the boot CPU or local srcu_data structure has callbacks, not idle. */ > > > + if (smp_load_acquire(&ssp->srcu_size_state) < SRCU_SIZE_WAIT_CALL) > > > + sdp = per_cpu_ptr(ssp->sda, get_boot_cpu_id()); > > > + else > > > + sdp = raw_cpu_ptr(ssp->sda); > > > > > > > Hi Paul, > > > > For the convert_to_big default configuration(SRCU_SIZING_AUTO), I found that the srcu is in > > SRCU_SIZE_SMALL state most of the time in the system, I think this change is meaningful. > > > > Thoughts ? > > >You are right that this change might make srcu_might_be_idle() return a > >more accurate value in the common case. As things are now, some other > >CPU might just now have added a callback, but might not yet have started > >that new grace period. In that case, we might expedite an SRCU grace > >period when we would not have otherwise done so. However, this change > >would also increase contention on the get_boot_cpu_id() CPU's srcu_data > >structure's ->lock. > > > >So the result of that inaccurate return value is that the first two SRCU > >grace periods in a burst might be expedited instead of only the first one, > >and even then only if we hit a very narrow race window. > > > >Besides, this same thing can happen if two CPUs do synchronize_srcu() > >at the same time after a long-enough pause between grace periods. > >Both see no callbacks, so both ask for an expedited grace period. > >So again, the first two grace periods are expedited. > > > >As a result, I am having a very hard time justifying the increased > >lock contention. > > Thanks reply, I have another question, Is this srcu_data structure's->lock necessary? > the rcu_segcblist_pend_cbs() only check *tails[RCU_DONE_TAIL] is empty. > or can use rcu_segcblist_get_seglen(RCU_WAIT_TAIL + RCU_NEXT_READY_TAIL + RCU_NEXT_TAIL) > instead of rcu_segcblist_pend_cbs() to avoid locking? (although this is also inaccurate) >That extra "*" means that the lock must be acquired. Otherwise, the >pointed-to callback might even be unmapped between the time we fetched >the pointer and the time we dereferenced it. Thanks for detailed explanation , learn more😊. > > Thanx, Paul. > Thanks > Zqiang > > > > >Am I missing something here? > > > > Thanx, Paul > > > > Thanks > > Zqiang > > > > > > >While at it if someone is interested in documenting/commenting on the meaning of > > >all these SRCU_SIZE_* things, it would be much appreciated! > > > > > >In the initial stage(SRCU_SIZE_SMALL), there is no srcu_node structures, only initialized > > >per-CPU srcu_data structures, at this time, we only use boot-cpu srcu_data structures's ->srcu_cblist > > >to store srcu callbacks. > > >if the contention of srcu_struct and srcu_data structure's->lock become busy, > > >transition to SRCU_SIZE_ALLOC. allocated memory for srcu_node structure at end of the SRCU > > >grace period. > > >if allocated success, transition to SRCU_SIZE_WAIT_BARRIER, in this state, invoke > > >srcu_barrier() will iterate over all possible CPUs, but we still only use boot-CPU srcu_cblist to store callbacks. > > >the task calling call_srcu() may have access to a new srcu_node structure or may not, > > >because call_srcu() is protected by SRCU, we may not transition to SRCU_SIZE_WAIT_CALL quickly, > > >need to wait in this state for a SRCU grace period to end. > > >After transition to SRCU_SIZE_WAIT_CALL, we enqueue srcu callbacks to own srcu_data structures's ->srcu_cblist. > > > > > >Does my description make my commit more detailed? > > > > > >Thanks > > >Zqiang > > > > > > > > > > > > > > > >Thanks. > > > > > > spin_lock_irqsave_rcu_node(sdp, flags); > > > if (rcu_segcblist_pend_cbs(&sdp->srcu_cblist)) { > > > spin_unlock_irqrestore_rcu_node(sdp, flags); > > > -- > > > 2.25.1 > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] srcu: Add detection of boot CPU srcu_data structure's->srcu_cblist 2022-11-17 14:20 ` Frederic Weisbecker 2022-11-18 4:53 ` Zhang, Qiang1 @ 2022-11-28 8:32 ` Pingfan Liu 1 sibling, 0 replies; 9+ messages in thread From: Pingfan Liu @ 2022-11-28 8:32 UTC (permalink / raw) To: Frederic Weisbecker; +Cc: Zqiang, paulmck, joel, rcu, linux-kernel On Thu, Nov 17, 2022 at 03:20:25PM +0100, Frederic Weisbecker wrote: > On Thu, Nov 17, 2022 at 07:20:50PM +0800, Zqiang wrote: > > Before SRCU_SIZE_WAIT_CALL srcu_size_state is reached, the srcu > > callback only insert to boot-CPU srcu_data structure's->srcu_cblist > > and other CPU srcu_data structure's->srcu_cblist is always empty. so > > before SRCU_SIZE_WAIT_CALL is reached, need to correctly check boot-CPU > > pend cbs in srcu_might_be_idle(). > > > > Signed-off-by: Zqiang <qiang1.zhang@intel.com> > > --- > > kernel/rcu/srcutree.c | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c > > index 6af031200580..6d9af9901765 100644 > > --- a/kernel/rcu/srcutree.c > > +++ b/kernel/rcu/srcutree.c > > @@ -1098,8 +1098,11 @@ static bool srcu_might_be_idle(struct srcu_struct *ssp) > > unsigned long tlast; > > > > check_init_srcu_struct(ssp); > > - /* If the local srcu_data structure has callbacks, not idle. */ > > - sdp = raw_cpu_ptr(ssp->sda); > > + /* If the boot CPU or local srcu_data structure has callbacks, not idle. */ > > + if (smp_load_acquire(&ssp->srcu_size_state) < SRCU_SIZE_WAIT_CALL) > > + sdp = per_cpu_ptr(ssp->sda, get_boot_cpu_id()); > > + else > > + sdp = raw_cpu_ptr(ssp->sda); > > While at it if someone is interested in documenting/commenting on the meaning of > all these SRCU_SIZE_* things, it would be much appreciated! > Due to a conflict understanding to the code, once hesitate to jump in. But anyway, just bold to move ahead. I have send a series "[PATCH 0/3] srcu: shrink the num of srcu_size_state", which opens the discussion. (Have Cced you and Zqiang) Please have a look. Thanks, Pingfan > Thanks. > > > spin_lock_irqsave_rcu_node(sdp, flags); > > if (rcu_segcblist_pend_cbs(&sdp->srcu_cblist)) { > > spin_unlock_irqrestore_rcu_node(sdp, flags); > > -- > > 2.25.1 > > ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-11-28 8:32 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-11-17 11:20 [PATCH] srcu: Add detection of boot CPU srcu_data structure's->srcu_cblist Zqiang 2022-11-17 14:20 ` Frederic Weisbecker 2022-11-18 4:53 ` Zhang, Qiang1 2022-11-23 2:01 ` Zhang, Qiang1 2022-11-23 19:04 ` Paul E. McKenney 2022-11-24 1:43 ` Zhang, Qiang1 2022-11-24 5:25 ` Paul E. McKenney 2022-11-24 5:59 ` Zhang, Qiang1 2022-11-28 8:32 ` Pingfan Liu
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.