[PATCH] rcu: Benefit from expedited grace period in __wait_rcu_gp

[PATCH] rcu: Benefit from expedited grace period in __wait_rcu_gp

[KarimAllah Ahmed]

When expedited grace-period is set, both synchronize_sched
synchronize_rcu_bh can be optimized to have a significantly lower latency.

Improve wait_rcu_gp handling to also account for expedited grace-period.
The downside is that wait_rcu_gp will not wait anymore for all RCU variants
concurrently when an expedited grace-period is set, however, given the
improved latency it does not really matter.

Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Josh Triplett <josh@joshtriplett.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Lai Jiangshan <jiangshanlai@gmail.com>
Cc: linux-kernel@vger.kernel.org
Signed-off-by: KarimAllah Ahmed <karahmed@amazon.de>
---
 kernel/rcu/update.c | 34 ++++++++++++++++++++++++++++------
 1 file changed, 28 insertions(+), 6 deletions(-)

diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
index 68fa19a..44b8817 100644
--- a/kernel/rcu/update.c
+++ b/kernel/rcu/update.c
@@ -392,13 +392,27 @@ void __wait_rcu_gp(bool checktiny, int n, call_rcu_func_t *crcu_array,
 			might_sleep();
 			continue;
 		}
-		init_rcu_head_on_stack(&rs_array[i].head);
-		init_completion(&rs_array[i].completion);
+
 		for (j = 0; j < i; j++)
 			if (crcu_array[j] == crcu_array[i])
 				break;
-		if (j == i)
-			(crcu_array[i])(&rs_array[i].head, wakeme_after_rcu);
+		if (j != i)
+			continue;
+
+		if ((crcu_array[i] == call_rcu_sched ||
+		     crcu_array[i] == call_rcu_bh)
+		    && rcu_gp_is_expedited()) {
+			if (crcu_array[i] == call_rcu_sched)
+				synchronize_sched_expedited();
+			else
+				synchronize_rcu_bh_expedited();
+
+			continue;
+		}
+
+		init_rcu_head_on_stack(&rs_array[i].head);
+		init_completion(&rs_array[i].completion);
+		(crcu_array[i])(&rs_array[i].head, wakeme_after_rcu);
 	}
 
 	/* Wait for all callbacks to be invoked. */
@@ -407,11 +421,19 @@ void __wait_rcu_gp(bool checktiny, int n, call_rcu_func_t *crcu_array,
 		    (crcu_array[i] == call_rcu ||
 		     crcu_array[i] == call_rcu_bh))
 			continue;
+
+		if ((crcu_array[i] == call_rcu_sched ||
+		     crcu_array[i] == call_rcu_bh)
+		    && rcu_gp_is_expedited())
+			continue;
+
 		for (j = 0; j < i; j++)
 			if (crcu_array[j] == crcu_array[i])
 				break;
-		if (j == i)
-			wait_for_completion(&rs_array[i].completion);
+		if (j != i)
+			continue;
+
+		wait_for_completion(&rs_array[i].completion);
 		destroy_rcu_head_on_stack(&rs_array[i].head);
 	}
 }
-- 
2.7.4

Re: [PATCH] rcu: Benefit from expedited grace period in __wait_rcu_gp

[Paul E. McKenney]

On Fri, Oct 19, 2018 at 02:49:05AM +0200, KarimAllah Ahmed wrote:
> When expedited grace-period is set, both synchronize_sched
> synchronize_rcu_bh can be optimized to have a significantly lower latency.
> 
> Improve wait_rcu_gp handling to also account for expedited grace-period.
> The downside is that wait_rcu_gp will not wait anymore for all RCU variants
> concurrently when an expedited grace-period is set, however, given the
> improved latency it does not really matter.
> 
> Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Cc: Josh Triplett <josh@joshtriplett.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: KarimAllah Ahmed <karahmed@amazon.de>

Cute!

Unfortunately, there are a few problems with this patch:

1.	I will be eliminating synchronize_rcu_mult() due to the fact that
	the upcoming RCU flavor consolidation eliminates its sole caller.
	See 5fc9d4e000b1 ("rcu: Eliminate synchronize_rcu_mult()")
	in my -rcu tree.  This would of course also eliminate the effects
	of this patch.

2.	The real-time guys' users are not going to be at all happy
	with the IPIs resulting from the _expedited() API members.
	Yes, they can boot with rcupdate.rcu_normal=1, but they don't
	always need that big a hammer, and use of this kernel parameter
	can slow down boot, hibernation, suspend, network configuration,
	and much else besides.	We therefore don't want them to have to
	use rcupdate.rcu_normal=1 unless absolutely necessary.

3.	If the real-time guys' users were to have booted with
	rcupdate.rcu_normal=1, then synchronize_sched_expedited()
	would invoke _synchronize_rcu_expedited, which would invoke
	wait_rcu_gp(), which would invoke _wait_rcu_gp() which would
	invoke __wait_rcu_gp(), which, given your patch, would in turn
	invoke synchronize_sched_expedited().  This situation could
	well prevent their systems from meeting their response-time
	requirements.

So I cannot accept this patch nor for that matter any similar patch.

But what were you really trying to get done here?  If you were thinking
of adding another synchronize_rcu_mult(), the flavor consolidation will
make that unnecessary in most cases.  If you are trying to speed up
CPU-hotplug operations, I suggest using the rcu_expedited sysctl variable
when taking a CPU offline.  If something else, please let me know what
it is so that we can work out how the problem might best be solved.

							Thanx, Paul

> ---
>  kernel/rcu/update.c | 34 ++++++++++++++++++++++++++++------
>  1 file changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
> index 68fa19a..44b8817 100644
> --- a/kernel/rcu/update.c
> +++ b/kernel/rcu/update.c
> @@ -392,13 +392,27 @@ void __wait_rcu_gp(bool checktiny, int n, call_rcu_func_t *crcu_array,
>  			might_sleep();
>  			continue;
>  		}
> -		init_rcu_head_on_stack(&rs_array[i].head);
> -		init_completion(&rs_array[i].completion);
> +
>  		for (j = 0; j < i; j++)
>  			if (crcu_array[j] == crcu_array[i])
>  				break;
> -		if (j == i)
> -			(crcu_array[i])(&rs_array[i].head, wakeme_after_rcu);
> +		if (j != i)
> +			continue;
> +
> +		if ((crcu_array[i] == call_rcu_sched ||
> +		     crcu_array[i] == call_rcu_bh)
> +		    && rcu_gp_is_expedited()) {
> +			if (crcu_array[i] == call_rcu_sched)
> +				synchronize_sched_expedited();
> +			else
> +				synchronize_rcu_bh_expedited();
> +
> +			continue;
> +		}
> +
> +		init_rcu_head_on_stack(&rs_array[i].head);
> +		init_completion(&rs_array[i].completion);
> +		(crcu_array[i])(&rs_array[i].head, wakeme_after_rcu);
>  	}
> 
>  	/* Wait for all callbacks to be invoked. */
> @@ -407,11 +421,19 @@ void __wait_rcu_gp(bool checktiny, int n, call_rcu_func_t *crcu_array,
>  		    (crcu_array[i] == call_rcu ||
>  		     crcu_array[i] == call_rcu_bh))
>  			continue;
> +
> +		if ((crcu_array[i] == call_rcu_sched ||
> +		     crcu_array[i] == call_rcu_bh)
> +		    && rcu_gp_is_expedited())
> +			continue;
> +
>  		for (j = 0; j < i; j++)
>  			if (crcu_array[j] == crcu_array[i])
>  				break;
> -		if (j == i)
> -			wait_for_completion(&rs_array[i].completion);
> +		if (j != i)
> +			continue;
> +
> +		wait_for_completion(&rs_array[i].completion);
>  		destroy_rcu_head_on_stack(&rs_array[i].head);
>  	}
>  }
> -- 
> 2.7.4
> 

Re: [PATCH] rcu: Benefit from expedited grace period in __wait_rcu_gp

[Raslan, KarimAllah]

On Fri, 2018-10-19 at 05:31 -0700, Paul E. McKenney wrote:
> On Fri, Oct 19, 2018 at 02:49:05AM +0200, KarimAllah Ahmed wrote:
> > 
> > When expedited grace-period is set, both synchronize_sched
> > synchronize_rcu_bh can be optimized to have a significantly lower latency.
> > 
> > Improve wait_rcu_gp handling to also account for expedited grace-period.
> > The downside is that wait_rcu_gp will not wait anymore for all RCU variants
> > concurrently when an expedited grace-period is set, however, given the
> > improved latency it does not really matter.
> > 
> > Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > Cc: Josh Triplett <josh@joshtriplett.org>
> > Cc: Steven Rostedt <rostedt@goodmis.org>
> > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> > Cc: linux-kernel@vger.kernel.org
> > Signed-off-by: KarimAllah Ahmed <karahmed@amazon.de>
> 
> Cute!
> 
> Unfortunately, there are a few problems with this patch:
> 
> 1.	I will be eliminating synchronize_rcu_mult() due to the fact that
> 	the upcoming RCU flavor consolidation eliminates its sole caller.
> 	See 5fc9d4e000b1 ("rcu: Eliminate synchronize_rcu_mult()")
> 	in my -rcu tree.  This would of course also eliminate the effects
> 	of this patch.

Your patch covers our use-case already, but I still think that the semantics 
for wait_rcu_gp is not clear to me.

The problem for us was that sched_cpu_deactivate would call
synchronize_rcu_mult which does not check for "expedited" at all. So even
though we are already using rcu_expedited sysctl variable, synchronize_rcu_mult 
was just ignoring it.

That being said, I indeed overlooked rcu_normal and that it takes precedence 
over expedited and I did not notice at all the deadlock you mentioned below!

That can however be easily fixed by also checking for !rcu_gp_is_normal.

> 
> 2.	The real-time guys' users are not going to be at all happy
> 	with the IPIs resulting from the _expedited() API members.
> 	Yes, they can boot with rcupdate.rcu_normal=1, but they don't
> 	always need that big a hammer, and use of this kernel parameter
> 	can slow down boot, hibernation, suspend, network configuration,
> 	and much else besides.	We therefore don't want them to have to
> 	use rcupdate.rcu_normal=1 unless absolutely necessary.

I might be missing something here. Why would they need to "explicitly" use 
rcu_normal? If rcu_expedited is set, would not the expected behavior is to call 
into the expedited version?

My patch should only activate *expedited* if and only if it is set.

I think I might be misunderstanding the expected behavior 
from synchronize_rcu_mult. My understanding is that something like:

synchronize_rcu_mult(call_rcu_sched) and synchronize_rcu() should have an 
identical behavior, right?

At least in this commit:

commit d7d34d5e46140 ("sched: Rely on synchronize_rcu_mult() de-duplication")

.. the change clearly gives the impression that they can be used 
interchangeably. The problem is that this is not true when you look at the 
implementation. One of them (i.e. synchronize_rcu) will respect the
expedite_rcu flag set by sysfs while the other (i.e. synchronize_rcu_mult) 
simply ignores it.

So my patch is about making sure that both of the variants actually respect 
it.


> 3.	If the real-time guys' users were to have booted with
> 	rcupdate.rcu_normal=1, then synchronize_sched_expedited()
> 	would invoke _synchronize_rcu_expedited, which would invoke
> 	wait_rcu_gp(), which would invoke _wait_rcu_gp() which would
> 	invoke __wait_rcu_gp(), which, given your patch, would in turn
> 	invoke synchronize_sched_expedited().  This situation could
> 	well prevent their systems from meeting their response-time
> 	requirements.
> 
> So I cannot accept this patch nor for that matter any similar patch.
> 
> But what were you really trying to get done here?  If you were thinking
> of adding another synchronize_rcu_mult(), the flavor consolidation will
> make that unnecessary in most cases.  If you are trying to speed up
> CPU-hotplug operations, I suggest using the rcu_expedited sysctl variable
> when taking a CPU offline.  If something else, please let me know what
> it is so that we can work out how the problem might best be solved.
> 
> 							Thanx, Paul
> 
> > 
> > ---
> >  kernel/rcu/update.c | 34 ++++++++++++++++++++++++++++------
> >  1 file changed, 28 insertions(+), 6 deletions(-)
> > 
> > diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
> > index 68fa19a..44b8817 100644
> > --- a/kernel/rcu/update.c
> > +++ b/kernel/rcu/update.c
> > @@ -392,13 +392,27 @@ void __wait_rcu_gp(bool checktiny, int n, call_rcu_func_t *crcu_array,
> >  			might_sleep();
> >  			continue;
> >  		}
> > -		init_rcu_head_on_stack(&rs_array[i].head);
> > -		init_completion(&rs_array[i].completion);
> > +
> >  		for (j = 0; j < i; j++)
> >  			if (crcu_array[j] == crcu_array[i])
> >  				break;
> > -		if (j == i)
> > -			(crcu_array[i])(&rs_array[i].head, wakeme_after_rcu);
> > +		if (j != i)
> > +			continue;
> > +
> > +		if ((crcu_array[i] == call_rcu_sched ||
> > +		     crcu_array[i] == call_rcu_bh)
> > +		    && rcu_gp_is_expedited()) {
> > +			if (crcu_array[i] == call_rcu_sched)
> > +				synchronize_sched_expedited();
> > +			else
> > +				synchronize_rcu_bh_expedited();
> > +
> > +			continue;
> > +		}
> > +
> > +		init_rcu_head_on_stack(&rs_array[i].head);
> > +		init_completion(&rs_array[i].completion);
> > +		(crcu_array[i])(&rs_array[i].head, wakeme_after_rcu);
> >  	}
> > 
> >  	/* Wait for all callbacks to be invoked. */
> > @@ -407,11 +421,19 @@ void __wait_rcu_gp(bool checktiny, int n, call_rcu_func_t *crcu_array,
> >  		    (crcu_array[i] == call_rcu ||
> >  		     crcu_array[i] == call_rcu_bh))
> >  			continue;
> > +
> > +		if ((crcu_array[i] == call_rcu_sched ||
> > +		     crcu_array[i] == call_rcu_bh)
> > +		    && rcu_gp_is_expedited())
> > +			continue;
> > +
> >  		for (j = 0; j < i; j++)
> >  			if (crcu_array[j] == crcu_array[i])
> >  				break;
> > -		if (j == i)
> > -			wait_for_completion(&rs_array[i].completion);
> > +		if (j != i)
> > +			continue;
> > +
> > +		wait_for_completion(&rs_array[i].completion);
> >  		destroy_rcu_head_on_stack(&rs_array[i].head);
> >  	}
> >  }
> > -- 
> > 2.7.4
> > 
> 
Amazon Development Center Germany GmbH
Berlin - Dresden - Aachen
main office: Krausenstr. 38, 10117 Berlin
Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger
Ust-ID: DE289237879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B

Re: [PATCH] rcu: Benefit from expedited grace period in __wait_rcu_gp

[Paul E. McKenney]

On Fri, Oct 19, 2018 at 07:45:51PM +0000, Raslan, KarimAllah wrote:
> On Fri, 2018-10-19 at 05:31 -0700, Paul E. McKenney wrote:
> > On Fri, Oct 19, 2018 at 02:49:05AM +0200, KarimAllah Ahmed wrote:
> > > 
> > > When expedited grace-period is set, both synchronize_sched
> > > synchronize_rcu_bh can be optimized to have a significantly lower latency.
> > > 
> > > Improve wait_rcu_gp handling to also account for expedited grace-period.
> > > The downside is that wait_rcu_gp will not wait anymore for all RCU variants
> > > concurrently when an expedited grace-period is set, however, given the
> > > improved latency it does not really matter.
> > > 
> > > Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > > Cc: Josh Triplett <josh@joshtriplett.org>
> > > Cc: Steven Rostedt <rostedt@goodmis.org>
> > > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > > Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> > > Cc: linux-kernel@vger.kernel.org
> > > Signed-off-by: KarimAllah Ahmed <karahmed@amazon.de>
> > 
> > Cute!
> > 
> > Unfortunately, there are a few problems with this patch:
> > 
> > 1.	I will be eliminating synchronize_rcu_mult() due to the fact that
> > 	the upcoming RCU flavor consolidation eliminates its sole caller.
> > 	See 5fc9d4e000b1 ("rcu: Eliminate synchronize_rcu_mult()")
> > 	in my -rcu tree.  This would of course also eliminate the effects
> > 	of this patch.
> 
> Your patch covers our use-case already, but I still think that the semantics 
> for wait_rcu_gp is not clear to me.
> 
> The problem for us was that sched_cpu_deactivate would call
> synchronize_rcu_mult which does not check for "expedited" at all. So even
> though we are already using rcu_expedited sysctl variable, synchronize_rcu_mult 
> was just ignoring it.
> 
> That being said, I indeed overlooked rcu_normal and that it takes precedence 
> over expedited and I did not notice at all the deadlock you mentioned below!
> 
> That can however be easily fixed by also checking for !rcu_gp_is_normal.

???

The aforementioned 5fc9d4e000b1 commit replaces the synchronize_rcu_mult()
with synchronize_rcu(), which really would be subject to the sysfs
variables.  Of course, this is not yet in mainline, so it perhaps cannot
solve your immediate problem, which probably involve older kernels in
any case.  More on this below...

> > 2.	The real-time guys' users are not going to be at all happy
> > 	with the IPIs resulting from the _expedited() API members.
> > 	Yes, they can boot with rcupdate.rcu_normal=1, but they don't
> > 	always need that big a hammer, and use of this kernel parameter
> > 	can slow down boot, hibernation, suspend, network configuration,
> > 	and much else besides.	We therefore don't want them to have to
> > 	use rcupdate.rcu_normal=1 unless absolutely necessary.
> 
> I might be missing something here. Why would they need to "explicitly" use 
> rcu_normal? If rcu_expedited is set, would not the expected behavior is to call 
> into the expedited version?
> 
> My patch should only activate *expedited* if and only if it is set.

You are right, I was confused.  However...

> I think I might be misunderstanding the expected behavior 
> from synchronize_rcu_mult. My understanding is that something like:
> 
> synchronize_rcu_mult(call_rcu_sched) and synchronize_rcu() should have an 
> identical behavior, right?

You would clearly prefer that it did, and the commit log does seem to
read that way, but synchronize_rcu_mult() is going away anyway, so there
isn't a whole lot of point in arguing about what it should have done.
And the eventual implementation (with 5fc9d4e000b1 or its successor)
will act as you want.

> At least in this commit:
> 
> commit d7d34d5e46140 ("sched: Rely on synchronize_rcu_mult() de-duplication")
> 
> .. the change clearly gives the impression that they can be used 
> interchangeably. The problem is that this is not true when you look at the 
> implementation. One of them (i.e. synchronize_rcu) will respect the
> expedite_rcu flag set by sysfs while the other (i.e. synchronize_rcu_mult) 
> simply ignores it.
> 
> So my patch is about making sure that both of the variants actually respect 
> it.

I am guessing that you need to make an older version of the kernel
expedite the CPU-hotplug grace periods.  I am also guessing that you can
carry patches to your kernels.  If so, I suggest the following simpler
change to sched_cpu_deactivate() in kernel/sched/core.c:

	if (rcu_gp_is_expedited()) {
		synchronize_sched_expedited();
		if (IS_ENABLED(CONFIG_PREEMPT))
			synchronize_rcu_expedited();
	} else {
		synchronize_rcu_mult(call_rcu, call_rcu_sched);
	}

As soon as this patch conflicts due to the synchronize_rcu_mult()
becoming synchronize_rcu(), you can drop the patch.  And this is the only
use of synchronize_rcu_mult(), so this approach loses no generality.
Longer term, this patch might possibly be the backport of 5fc9d4e000b1
back to v4.14, but at the end of the day this is up to the various
-stable maintainers.

Hmmm...  If you are running CONFIG_PREEMPT=n, you can use an even
simpler replacement for synchronize_rcu_mult():

	synchronize_sched_expedited(); /* Bug if CONFIG_PREEMPT=y!!! */

Would either of those two approaches work for you, or am I still missing
something?

							Thanx, Paul

> > 3.	If the real-time guys' users were to have booted with
> > 	rcupdate.rcu_normal=1, then synchronize_sched_expedited()
> > 	would invoke _synchronize_rcu_expedited, which would invoke
> > 	wait_rcu_gp(), which would invoke _wait_rcu_gp() which would
> > 	invoke __wait_rcu_gp(), which, given your patch, would in turn
> > 	invoke synchronize_sched_expedited().  This situation could
> > 	well prevent their systems from meeting their response-time
> > 	requirements.
> > 
> > So I cannot accept this patch nor for that matter any similar patch.
> > 
> > But what were you really trying to get done here?  If you were thinking
> > of adding another synchronize_rcu_mult(), the flavor consolidation will
> > make that unnecessary in most cases.  If you are trying to speed up
> > CPU-hotplug operations, I suggest using the rcu_expedited sysctl variable
> > when taking a CPU offline.  If something else, please let me know what
> > it is so that we can work out how the problem might best be solved.
> > 
> > 							Thanx, Paul
> > 
> > > 
> > > ---
> > >  kernel/rcu/update.c | 34 ++++++++++++++++++++++++++++------
> > >  1 file changed, 28 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
> > > index 68fa19a..44b8817 100644
> > > --- a/kernel/rcu/update.c
> > > +++ b/kernel/rcu/update.c
> > > @@ -392,13 +392,27 @@ void __wait_rcu_gp(bool checktiny, int n, call_rcu_func_t *crcu_array,
> > >  			might_sleep();
> > >  			continue;
> > >  		}
> > > -		init_rcu_head_on_stack(&rs_array[i].head);
> > > -		init_completion(&rs_array[i].completion);
> > > +
> > >  		for (j = 0; j < i; j++)
> > >  			if (crcu_array[j] == crcu_array[i])
> > >  				break;
> > > -		if (j == i)
> > > -			(crcu_array[i])(&rs_array[i].head, wakeme_after_rcu);
> > > +		if (j != i)
> > > +			continue;
> > > +
> > > +		if ((crcu_array[i] == call_rcu_sched ||
> > > +		     crcu_array[i] == call_rcu_bh)
> > > +		    && rcu_gp_is_expedited()) {
> > > +			if (crcu_array[i] == call_rcu_sched)
> > > +				synchronize_sched_expedited();
> > > +			else
> > > +				synchronize_rcu_bh_expedited();
> > > +
> > > +			continue;
> > > +		}
> > > +
> > > +		init_rcu_head_on_stack(&rs_array[i].head);
> > > +		init_completion(&rs_array[i].completion);
> > > +		(crcu_array[i])(&rs_array[i].head, wakeme_after_rcu);
> > >  	}
> > > 
> > >  	/* Wait for all callbacks to be invoked. */
> > > @@ -407,11 +421,19 @@ void __wait_rcu_gp(bool checktiny, int n, call_rcu_func_t *crcu_array,
> > >  		    (crcu_array[i] == call_rcu ||
> > >  		     crcu_array[i] == call_rcu_bh))
> > >  			continue;
> > > +
> > > +		if ((crcu_array[i] == call_rcu_sched ||
> > > +		     crcu_array[i] == call_rcu_bh)
> > > +		    && rcu_gp_is_expedited())
> > > +			continue;
> > > +
> > >  		for (j = 0; j < i; j++)
> > >  			if (crcu_array[j] == crcu_array[i])
> > >  				break;
> > > -		if (j == i)
> > > -			wait_for_completion(&rs_array[i].completion);
> > > +		if (j != i)
> > > +			continue;
> > > +
> > > +		wait_for_completion(&rs_array[i].completion);
> > >  		destroy_rcu_head_on_stack(&rs_array[i].head);
> > >  	}
> > >  }
> > > -- 
> > > 2.7.4
> > > 
> > 
> Amazon Development Center Germany GmbH
> Berlin - Dresden - Aachen
> main office: Krausenstr. 38, 10117 Berlin
> Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger
> Ust-ID: DE289237879
> Eingetragen am Amtsgericht Charlottenburg HRB 149173 B

Re: [PATCH] rcu: Benefit from expedited grace period in __wait_rcu_gp

[Raslan, KarimAllah]

On Fri, 2018-10-19 at 13:21 -0700, Paul E. McKenney wrote:
> On Fri, Oct 19, 2018 at 07:45:51PM +0000, Raslan, KarimAllah wrote:
> > 
> > On Fri, 2018-10-19 at 05:31 -0700, Paul E. McKenney wrote:
> > > 
> > > On Fri, Oct 19, 2018 at 02:49:05AM +0200, KarimAllah Ahmed wrote:
> > > > 
> > > > 
> > > > When expedited grace-period is set, both synchronize_sched
> > > > synchronize_rcu_bh can be optimized to have a significantly lower latency.
> > > > 
> > > > Improve wait_rcu_gp handling to also account for expedited grace-period.
> > > > The downside is that wait_rcu_gp will not wait anymore for all RCU variants
> > > > concurrently when an expedited grace-period is set, however, given the
> > > > improved latency it does not really matter.
> > > > 
> > > > Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > > > Cc: Josh Triplett <josh@joshtriplett.org>
> > > > Cc: Steven Rostedt <rostedt@goodmis.org>
> > > > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > > > Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> > > > Cc: linux-kernel@vger.kernel.org
> > > > Signed-off-by: KarimAllah Ahmed <karahmed@amazon.de>
> > > 
> > > Cute!
> > > 
> > > Unfortunately, there are a few problems with this patch:
> > > 
> > > 1.	I will be eliminating synchronize_rcu_mult() due to the fact that
> > > 	the upcoming RCU flavor consolidation eliminates its sole caller.
> > > 	See 5fc9d4e000b1 ("rcu: Eliminate synchronize_rcu_mult()")
> > > 	in my -rcu tree.  This would of course also eliminate the effects
> > > 	of this patch.
> > 
> > Your patch covers our use-case already, but I still think that the semantics 
> > for wait_rcu_gp is not clear to me.
> > 
> > The problem for us was that sched_cpu_deactivate would call
> > synchronize_rcu_mult which does not check for "expedited" at all. So even
> > though we are already using rcu_expedited sysctl variable, synchronize_rcu_mult 
> > was just ignoring it.
> > 
> > That being said, I indeed overlooked rcu_normal and that it takes precedence 
> > over expedited and I did not notice at all the deadlock you mentioned below!
> > 
> > That can however be easily fixed by also checking for !rcu_gp_is_normal.
> 
> ???
> 
> The aforementioned 5fc9d4e000b1 commit replaces the synchronize_rcu_mult()
> with synchronize_rcu(), which really would be subject to the sysfs
> variables.  Of course, this is not yet in mainline, so it perhaps cannot
> solve your immediate problem, which probably involve older kernels in
> any case.  More on this below...
> 
> > 
> > > 
> > > 2.	The real-time guys' users are not going to be at all happy
> > > 	with the IPIs resulting from the _expedited() API members.
> > > 	Yes, they can boot with rcupdate.rcu_normal=1, but they don't
> > > 	always need that big a hammer, and use of this kernel parameter
> > > 	can slow down boot, hibernation, suspend, network configuration,
> > > 	and much else besides.	We therefore don't want them to have to
> > > 	use rcupdate.rcu_normal=1 unless absolutely necessary.
> > 
> > I might be missing something here. Why would they need to "explicitly" use 
> > rcu_normal? If rcu_expedited is set, would not the expected behavior is to call 
> > into the expedited version?
> > 
> > My patch should only activate *expedited* if and only if it is set.
> 
> You are right, I was confused.  However...
> 
> > 
> > I think I might be misunderstanding the expected behavior 
> > from synchronize_rcu_mult. My understanding is that something like:
> > 
> > synchronize_rcu_mult(call_rcu_sched) and synchronize_rcu() should have an 
> > identical behavior, right?
> 
> You would clearly prefer that it did, and the commit log does seem to
> read that way, but synchronize_rcu_mult() is going away anyway, so there
> isn't a whole lot of point in arguing about what it should have done.
> And the eventual implementation (with 5fc9d4e000b1 or its successor)
> will act as you want.
> 
> > 
> > At least in this commit:
> > 
> > commit d7d34d5e46140 ("sched: Rely on synchronize_rcu_mult() de-duplication")
> > 
> > .. the change clearly gives the impression that they can be used 
> > interchangeably. The problem is that this is not true when you look at the 
> > implementation. One of them (i.e. synchronize_rcu) will respect the
> > expedite_rcu flag set by sysfs while the other (i.e. synchronize_rcu_mult) 
> > simply ignores it.
> > 
> > So my patch is about making sure that both of the variants actually respect 
> > it.
> 
> I am guessing that you need to make an older version of the kernel
> expedite the CPU-hotplug grace periods.  I am also guessing that you can
> carry patches to your kernels.  If so, I suggest the following simpler
> change to sched_cpu_deactivate() in kernel/sched/core.c:
> 
> 	if (rcu_gp_is_expedited()) {
> 		synchronize_sched_expedited();
> 		if (IS_ENABLED(CONFIG_PREEMPT))
> 			synchronize_rcu_expedited();
> 	} else {
> 		synchronize_rcu_mult(call_rcu, call_rcu_sched);
> 	}
> 
> As soon as this patch conflicts due to the synchronize_rcu_mult()
> becoming synchronize_rcu(), you can drop the patch.  And this is the only
> use of synchronize_rcu_mult(), so this approach loses no generality.
> Longer term, this patch might possibly be the backport of 5fc9d4e000b1
> back to v4.14, but at the end of the day this is up to the various
> -stable maintainers.
> 
> Hmmm...  If you are running CONFIG_PREEMPT=n, you can use an even
> simpler replacement for synchronize_rcu_mult():
> 
> 	synchronize_sched_expedited(); /* Bug if CONFIG_PREEMPT=y!!! */
> 
> Would either of those two approaches work for you, or am I still missing
> something?

Sorry, I was not clear. The original commit in your -rcu tree already works for 
us. So that is sorted out, thank you :)

In my previous reply, when I was referring to synchronize_rcu_mult I really
also wanted to also point to __wait_rcu_gp. With the current state in your -rcu 
tree, __wait_rcu_gp does not look whether "expedited" is enabled or not. This
is currently delegated to the callers (e.g. synchronize_rcu and 
synchronize_rcu_bh). So any new direct users of __wait_rcu_gp would also miss 
this check (i.e. just like what happened in synchronize_rcu_mult). If we want 
__wait_rcu_gp to always respect rcu_expedited and rcu_normal flags, we might 
want to pull these checks into __wait_rcu_gp instead and remove them from the 
callers.


> 
> 							Thanx, Paul
> 
> > 
> > > 
> > > 3.	If the real-time guys' users were to have booted with
> > > 	rcupdate.rcu_normal=1, then synchronize_sched_expedited()
> > > 	would invoke _synchronize_rcu_expedited, which would invoke
> > > 	wait_rcu_gp(), which would invoke _wait_rcu_gp() which would
> > > 	invoke __wait_rcu_gp(), which, given your patch, would in turn
> > > 	invoke synchronize_sched_expedited().  This situation could
> > > 	well prevent their systems from meeting their response-time
> > > 	requirements.
> > > 
> > > So I cannot accept this patch nor for that matter any similar patch.
> > > 
> > > But what were you really trying to get done here?  If you were thinking
> > > of adding another synchronize_rcu_mult(), the flavor consolidation will
> > > make that unnecessary in most cases.  If you are trying to speed up
> > > CPU-hotplug operations, I suggest using the rcu_expedited sysctl variable
> > > when taking a CPU offline.  If something else, please let me know what
> > > it is so that we can work out how the problem might best be solved.
> > > 
> > > 							Thanx, Paul
> > > 
> > > > 
> > > > 
> > > > ---
> > > >  kernel/rcu/update.c | 34 ++++++++++++++++++++++++++++------
> > > >  1 file changed, 28 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
> > > > index 68fa19a..44b8817 100644
> > > > --- a/kernel/rcu/update.c
> > > > +++ b/kernel/rcu/update.c
> > > > @@ -392,13 +392,27 @@ void __wait_rcu_gp(bool checktiny, int n, call_rcu_func_t *crcu_array,
> > > >  			might_sleep();
> > > >  			continue;
> > > >  		}
> > > > -		init_rcu_head_on_stack(&rs_array[i].head);
> > > > -		init_completion(&rs_array[i].completion);
> > > > +
> > > >  		for (j = 0; j < i; j++)
> > > >  			if (crcu_array[j] == crcu_array[i])
> > > >  				break;
> > > > -		if (j == i)
> > > > -			(crcu_array[i])(&rs_array[i].head, wakeme_after_rcu);
> > > > +		if (j != i)
> > > > +			continue;
> > > > +
> > > > +		if ((crcu_array[i] == call_rcu_sched ||
> > > > +		     crcu_array[i] == call_rcu_bh)
> > > > +		    && rcu_gp_is_expedited()) {
> > > > +			if (crcu_array[i] == call_rcu_sched)
> > > > +				synchronize_sched_expedited();
> > > > +			else
> > > > +				synchronize_rcu_bh_expedited();
> > > > +
> > > > +			continue;
> > > > +		}
> > > > +
> > > > +		init_rcu_head_on_stack(&rs_array[i].head);
> > > > +		init_completion(&rs_array[i].completion);
> > > > +		(crcu_array[i])(&rs_array[i].head, wakeme_after_rcu);
> > > >  	}
> > > > 
> > > >  	/* Wait for all callbacks to be invoked. */
> > > > @@ -407,11 +421,19 @@ void __wait_rcu_gp(bool checktiny, int n, call_rcu_func_t *crcu_array,
> > > >  		    (crcu_array[i] == call_rcu ||
> > > >  		     crcu_array[i] == call_rcu_bh))
> > > >  			continue;
> > > > +
> > > > +		if ((crcu_array[i] == call_rcu_sched ||
> > > > +		     crcu_array[i] == call_rcu_bh)
> > > > +		    && rcu_gp_is_expedited())
> > > > +			continue;
> > > > +
> > > >  		for (j = 0; j < i; j++)
> > > >  			if (crcu_array[j] == crcu_array[i])
> > > >  				break;
> > > > -		if (j == i)
> > > > -			wait_for_completion(&rs_array[i].completion);
> > > > +		if (j != i)
> > > > +			continue;
> > > > +
> > > > +		wait_for_completion(&rs_array[i].completion);
> > > >  		destroy_rcu_head_on_stack(&rs_array[i].head);
> > > >  	}
> > > >  }
> > > > -- 
> > > > 2.7.4
> > > > 
> > > 
> > Amazon Development Center Germany GmbH
> > Berlin - Dresden - Aachen
> > main office: Krausenstr. 38, 10117 Berlin
> > Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger
> > Ust-ID: DE289237879
> > Eingetragen am Amtsgericht Charlottenburg HRB 149173 B
> 
> 
Amazon Development Center Germany GmbH
Berlin - Dresden - Aachen
main office: Krausenstr. 38, 10117 Berlin
Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger
Ust-ID: DE289237879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B

Re: [PATCH] rcu: Benefit from expedited grace period in __wait_rcu_gp

[Paul E. McKenney]

On Tue, Oct 23, 2018 at 03:13:43PM +0000, Raslan, KarimAllah wrote:
> On Fri, 2018-10-19 at 13:21 -0700, Paul E. McKenney wrote:
> > On Fri, Oct 19, 2018 at 07:45:51PM +0000, Raslan, KarimAllah wrote:
> > > 
> > > On Fri, 2018-10-19 at 05:31 -0700, Paul E. McKenney wrote:
> > > > 
> > > > On Fri, Oct 19, 2018 at 02:49:05AM +0200, KarimAllah Ahmed wrote:
> > > > > 
> > > > > 
> > > > > When expedited grace-period is set, both synchronize_sched
> > > > > synchronize_rcu_bh can be optimized to have a significantly lower latency.
> > > > > 
> > > > > Improve wait_rcu_gp handling to also account for expedited grace-period.
> > > > > The downside is that wait_rcu_gp will not wait anymore for all RCU variants
> > > > > concurrently when an expedited grace-period is set, however, given the
> > > > > improved latency it does not really matter.
> > > > > 
> > > > > Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > > > > Cc: Josh Triplett <josh@joshtriplett.org>
> > > > > Cc: Steven Rostedt <rostedt@goodmis.org>
> > > > > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > > > > Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> > > > > Cc: linux-kernel@vger.kernel.org
> > > > > Signed-off-by: KarimAllah Ahmed <karahmed@amazon.de>
> > > > 
> > > > Cute!
> > > > 
> > > > Unfortunately, there are a few problems with this patch:
> > > > 
> > > > 1.	I will be eliminating synchronize_rcu_mult() due to the fact that
> > > > 	the upcoming RCU flavor consolidation eliminates its sole caller.
> > > > 	See 5fc9d4e000b1 ("rcu: Eliminate synchronize_rcu_mult()")
> > > > 	in my -rcu tree.  This would of course also eliminate the effects
> > > > 	of this patch.
> > > 
> > > Your patch covers our use-case already, but I still think that the semantics 
> > > for wait_rcu_gp is not clear to me.
> > > 
> > > The problem for us was that sched_cpu_deactivate would call
> > > synchronize_rcu_mult which does not check for "expedited" at all. So even
> > > though we are already using rcu_expedited sysctl variable, synchronize_rcu_mult 
> > > was just ignoring it.
> > > 
> > > That being said, I indeed overlooked rcu_normal and that it takes precedence 
> > > over expedited and I did not notice at all the deadlock you mentioned below!
> > > 
> > > That can however be easily fixed by also checking for !rcu_gp_is_normal.
> > 
> > ???
> > 
> > The aforementioned 5fc9d4e000b1 commit replaces the synchronize_rcu_mult()
> > with synchronize_rcu(), which really would be subject to the sysfs
> > variables.  Of course, this is not yet in mainline, so it perhaps cannot
> > solve your immediate problem, which probably involve older kernels in
> > any case.  More on this below...
> > 
> > > 
> > > > 
> > > > 2.	The real-time guys' users are not going to be at all happy
> > > > 	with the IPIs resulting from the _expedited() API members.
> > > > 	Yes, they can boot with rcupdate.rcu_normal=1, but they don't
> > > > 	always need that big a hammer, and use of this kernel parameter
> > > > 	can slow down boot, hibernation, suspend, network configuration,
> > > > 	and much else besides.	We therefore don't want them to have to
> > > > 	use rcupdate.rcu_normal=1 unless absolutely necessary.
> > > 
> > > I might be missing something here. Why would they need to "explicitly" use 
> > > rcu_normal? If rcu_expedited is set, would not the expected behavior is to call 
> > > into the expedited version?
> > > 
> > > My patch should only activate *expedited* if and only if it is set.
> > 
> > You are right, I was confused.  However...
> > 
> > > 
> > > I think I might be misunderstanding the expected behavior 
> > > from synchronize_rcu_mult. My understanding is that something like:
> > > 
> > > synchronize_rcu_mult(call_rcu_sched) and synchronize_rcu() should have an 
> > > identical behavior, right?
> > 
> > You would clearly prefer that it did, and the commit log does seem to
> > read that way, but synchronize_rcu_mult() is going away anyway, so there
> > isn't a whole lot of point in arguing about what it should have done.
> > And the eventual implementation (with 5fc9d4e000b1 or its successor)
> > will act as you want.
> > 
> > > 
> > > At least in this commit:
> > > 
> > > commit d7d34d5e46140 ("sched: Rely on synchronize_rcu_mult() de-duplication")
> > > 
> > > .. the change clearly gives the impression that they can be used 
> > > interchangeably. The problem is that this is not true when you look at the 
> > > implementation. One of them (i.e. synchronize_rcu) will respect the
> > > expedite_rcu flag set by sysfs while the other (i.e. synchronize_rcu_mult) 
> > > simply ignores it.
> > > 
> > > So my patch is about making sure that both of the variants actually respect 
> > > it.
> > 
> > I am guessing that you need to make an older version of the kernel
> > expedite the CPU-hotplug grace periods.  I am also guessing that you can
> > carry patches to your kernels.  If so, I suggest the following simpler
> > change to sched_cpu_deactivate() in kernel/sched/core.c:
> > 
> > 	if (rcu_gp_is_expedited()) {
> > 		synchronize_sched_expedited();
> > 		if (IS_ENABLED(CONFIG_PREEMPT))
> > 			synchronize_rcu_expedited();
> > 	} else {
> > 		synchronize_rcu_mult(call_rcu, call_rcu_sched);
> > 	}
> > 
> > As soon as this patch conflicts due to the synchronize_rcu_mult()
> > becoming synchronize_rcu(), you can drop the patch.  And this is the only
> > use of synchronize_rcu_mult(), so this approach loses no generality.
> > Longer term, this patch might possibly be the backport of 5fc9d4e000b1
> > back to v4.14, but at the end of the day this is up to the various
> > -stable maintainers.
> > 
> > Hmmm...  If you are running CONFIG_PREEMPT=n, you can use an even
> > simpler replacement for synchronize_rcu_mult():
> > 
> > 	synchronize_sched_expedited(); /* Bug if CONFIG_PREEMPT=y!!! */
> > 
> > Would either of those two approaches work for you, or am I still missing
> > something?
> 
> Sorry, I was not clear. The original commit in your -rcu tree already works for 
> us. So that is sorted out, thank you :)

Very good, thank you!

Just to confirm:

1.	By "original commit", you mean 5fc9d4e000b1 ("rcu: Eliminate
	synchronize_rcu_mult()"), right?

2.	Please note that 5fc9d4e000b1 can only be backported to the
	upcoming v4.20 release, and given that it is only an uncommon-case
	performance regression, I would not submit it to -stable at all.
	So if you need this performance regression to work in v4.19 or
	earlier, please:

	a.	Let me know.

	b.	Test this replacement for the current call to
		synchronize_rcu_mult() from sched_cpu_deactivate()
		in kernel/sched/core.c:

		if (rcu_gp_is_expedited()) {
			synchronize_sched_expedited();
			if (IS_ENABLED(CONFIG_PREEMPT))
				synchronize_rcu_expedited();
		} else {
			synchronize_rcu_mult(call_rcu, call_rcu_sched);
		}

> In my previous reply, when I was referring to synchronize_rcu_mult I really
> also wanted to also point to __wait_rcu_gp. With the current state in your -rcu 
> tree, __wait_rcu_gp does not look whether "expedited" is enabled or not. This
> is currently delegated to the callers (e.g. synchronize_rcu and 
> synchronize_rcu_bh). So any new direct users of __wait_rcu_gp would also miss 
> this check (i.e. just like what happened in synchronize_rcu_mult). If we want 
> __wait_rcu_gp to always respect rcu_expedited and rcu_normal flags, we might 
> want to pull these checks into __wait_rcu_gp instead and remove them from the 
> callers.

Yes, __wait_rcu_gp() does arguably have odd semantics.  It is after all
used to implement the rcu_barrier() call in Tiny RCU and waits for a
normal (not expedited!) grace period in Tree RCU.  But there are many
other places in RCU where the semantics are quite a bit more unusual.

So welcome to my world!  ;-)

							Thanx, Paul

> > > > 3.	If the real-time guys' users were to have booted with
> > > > 	rcupdate.rcu_normal=1, then synchronize_sched_expedited()
> > > > 	would invoke _synchronize_rcu_expedited, which would invoke
> > > > 	wait_rcu_gp(), which would invoke _wait_rcu_gp() which would
> > > > 	invoke __wait_rcu_gp(), which, given your patch, would in turn
> > > > 	invoke synchronize_sched_expedited().  This situation could
> > > > 	well prevent their systems from meeting their response-time
> > > > 	requirements.
> > > > 
> > > > So I cannot accept this patch nor for that matter any similar patch.
> > > > 
> > > > But what were you really trying to get done here?  If you were thinking
> > > > of adding another synchronize_rcu_mult(), the flavor consolidation will
> > > > make that unnecessary in most cases.  If you are trying to speed up
> > > > CPU-hotplug operations, I suggest using the rcu_expedited sysctl variable
> > > > when taking a CPU offline.  If something else, please let me know what
> > > > it is so that we can work out how the problem might best be solved.
> > > > 
> > > > 							Thanx, Paul
> > > > 
> > > > > 
> > > > > 
> > > > > ---
> > > > >  kernel/rcu/update.c | 34 ++++++++++++++++++++++++++++------
> > > > >  1 file changed, 28 insertions(+), 6 deletions(-)
> > > > > 
> > > > > diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
> > > > > index 68fa19a..44b8817 100644
> > > > > --- a/kernel/rcu/update.c
> > > > > +++ b/kernel/rcu/update.c
> > > > > @@ -392,13 +392,27 @@ void __wait_rcu_gp(bool checktiny, int n, call_rcu_func_t *crcu_array,
> > > > >  			might_sleep();
> > > > >  			continue;
> > > > >  		}
> > > > > -		init_rcu_head_on_stack(&rs_array[i].head);
> > > > > -		init_completion(&rs_array[i].completion);
> > > > > +
> > > > >  		for (j = 0; j < i; j++)
> > > > >  			if (crcu_array[j] == crcu_array[i])
> > > > >  				break;
> > > > > -		if (j == i)
> > > > > -			(crcu_array[i])(&rs_array[i].head, wakeme_after_rcu);
> > > > > +		if (j != i)
> > > > > +			continue;
> > > > > +
> > > > > +		if ((crcu_array[i] == call_rcu_sched ||
> > > > > +		     crcu_array[i] == call_rcu_bh)
> > > > > +		    && rcu_gp_is_expedited()) {
> > > > > +			if (crcu_array[i] == call_rcu_sched)
> > > > > +				synchronize_sched_expedited();
> > > > > +			else
> > > > > +				synchronize_rcu_bh_expedited();
> > > > > +
> > > > > +			continue;
> > > > > +		}
> > > > > +
> > > > > +		init_rcu_head_on_stack(&rs_array[i].head);
> > > > > +		init_completion(&rs_array[i].completion);
> > > > > +		(crcu_array[i])(&rs_array[i].head, wakeme_after_rcu);
> > > > >  	}
> > > > > 
> > > > >  	/* Wait for all callbacks to be invoked. */
> > > > > @@ -407,11 +421,19 @@ void __wait_rcu_gp(bool checktiny, int n, call_rcu_func_t *crcu_array,
> > > > >  		    (crcu_array[i] == call_rcu ||
> > > > >  		     crcu_array[i] == call_rcu_bh))
> > > > >  			continue;
> > > > > +
> > > > > +		if ((crcu_array[i] == call_rcu_sched ||
> > > > > +		     crcu_array[i] == call_rcu_bh)
> > > > > +		    && rcu_gp_is_expedited())
> > > > > +			continue;
> > > > > +
> > > > >  		for (j = 0; j < i; j++)
> > > > >  			if (crcu_array[j] == crcu_array[i])
> > > > >  				break;
> > > > > -		if (j == i)
> > > > > -			wait_for_completion(&rs_array[i].completion);
> > > > > +		if (j != i)
> > > > > +			continue;
> > > > > +
> > > > > +		wait_for_completion(&rs_array[i].completion);
> > > > >  		destroy_rcu_head_on_stack(&rs_array[i].head);
> > > > >  	}
> > > > >  }
> > > > > -- 
> > > > > 2.7.4
> > > > > 
> > > > 
> > > Amazon Development Center Germany GmbH
> > > Berlin - Dresden - Aachen
> > > main office: Krausenstr. 38, 10117 Berlin
> > > Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger
> > > Ust-ID: DE289237879
> > > Eingetragen am Amtsgericht Charlottenburg HRB 149173 B
> > 
> > 
> Amazon Development Center Germany GmbH
> Berlin - Dresden - Aachen
> main office: Krausenstr. 38, 10117 Berlin
> Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger
> Ust-ID: DE289237879
> Eingetragen am Amtsgericht Charlottenburg HRB 149173 B