On Tue, Mar 31 2020, Joel Fernandes wrote:

> On Wed, Apr 01, 2020 at 09:19:49AM +1100, NeilBrown wrote:
>> On Tue, Mar 31 2020, Joel Fernandes wrote:
>> 
>> > On Tue, Mar 31, 2020 at 05:34:50PM +0200, Michal Hocko wrote:
>> >> On Tue 31-03-20 10:58:06, Joel Fernandes wrote:
>> >> [...]
>> >> > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
>> >> > > index 4be763355c9fb..965deefffdd58 100644
>> >> > > --- a/kernel/rcu/tree.c
>> >> > > +++ b/kernel/rcu/tree.c
>> >> > > @@ -3149,7 +3149,7 @@ static inline struct rcu_head *attach_rcu_head_to_object(void *obj)
>> >> > >  
>> >> > >  	if (!ptr)
>> >> > >  		ptr = kmalloc(sizeof(unsigned long *) +
>> >> > > -				sizeof(struct rcu_head), GFP_ATOMIC | __GFP_NOWARN);
>> >> > > +				sizeof(struct rcu_head), GFP_MEMALLOC);
>> >> > 
>> >> > Just to add, the main requirements here are:
>> >> > 1. Allocation should be bounded in time.
>> >> > 2. Allocation should try hard (possibly tapping into reserves)
>> >> > 3. Sleeping is Ok but should not affect the time bound.
>> >> 
>> >> 
>> >> __GFP_ATOMIC | __GFP_HIGH is the way to get an additional access to
>> >> memory reserves regarless of the sleeping status.
>> >> 
>> >> Using __GFP_MEMALLOC is quite dangerous because it can deplete _all_ the
>> >> memory. What does prevent the above code path to do that?
>> >
>> > Can you suggest what prevents other users of GFP_MEMALLOC from doing that
>> > also? That's the whole point of having a reserve, in normal usage no one will
>> > use it, but some times you need to use it. Keep in mind this is not a common
>> > case in this code here, this is triggered only if earlier allocation attempts
>> > failed. Only *then* we try with GFP_MEMALLOC with promises to free additional
>> > memory soon.
>> 
>> I think that "soon" is the key point.  Users of __GFP_MEMALLOC certainly
>> must be working to free other memory, that other memory needs to be freed
>> "soon".  In particular - sooner than all the reserve is exhausted.  This
>> can require rate-limiting.  If one allocation can result in one page
>> being freed, that is good and it is probably OK to have 1000 allocations
>> resulting in 1000 pages being freed soon.  But 10 million allocation to
>> gain 10 million pages is not such a good thing and shouldn't be needed.
>> Once those first 1000 pages have been freed, you won't need
>> __GFP_MEMALLOC allocations any more, and you must be prepare to wait for
>> them.
>> 
>> So where does the rate-limiting happen in your proposal?  A GP can be
>> multiple milliseconds, which is time for lots of memory to be allocated
>> and for rcu-free queues to grow quite large.
>> 
>> You mention a possible fall-back of calling synchronize_rcu().  I think
>> that needs to be a fallback that happens well before __GFP_MEMALLOC is
>> exhausted.   You need to choose some maximum amount that you will
>> allocate, then use synchronize_rcu() (or probably the _expedited
>> version) after that.  The pool of reserves are certainly there for you
>> to use, but not for you to exhaust.
>> 
>> If you have your own rate-limiting, then I think __GFP_MEMALLOC is
>> probably OK, and also you *don't* want the memalloc to wait.  If memory
>> cannot be allocated immediately, you need to use your own fallback.
>
> Thanks a lot for explaining in detail, the RFC patch has served its purpose
> well ;-)
>
> On discussing with RCU comrades, we agreed to not use GFP_MEMALLOC. But
> instead pre-allocate a cache (we do have a cache but it is not yet
> pre-allocated, just allocated on demand).
>
> About the rate limiting, we would fallback to synchronize_rcu() instead of
> sleeping in case of trobule. However I would like to add a warning if we ever
> hit the troublesome path mainly because that means we depleted the
> pre-allocated cache and perhaps the user should switch to adding an rcu_head
> in their structure to reduce latency. I'm adding that warning to my tree:

If this warning is only interesting to developers, I think you should
only show it to developers, not to end-users. i.e. protect it with
CONFIG_DEBUG_RCU or something like that.

NeilBrown


>
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 4be763355c9fb..6172e6296dd7d 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -110,6 +110,10 @@ module_param(rcu_fanout_exact, bool, 0444);
>  static int rcu_fanout_leaf = RCU_FANOUT_LEAF;
>  module_param(rcu_fanout_leaf, int, 0444);
>  int rcu_num_lvls __read_mostly = RCU_NUM_LVLS;
> +/* Silence the kvfree_rcu() complaint (warning) that it blocks */
> +int rcu_kfree_nowarn;
> +module_param(rcu_kfree_nowarn, int, 0444);
> +
>  /* Number of rcu_nodes at specified level. */
>  int num_rcu_lvl[] = NUM_RCU_LVL_INIT;
>  int rcu_num_nodes __read_mostly = NUM_RCU_NODES; /* Total # rcu_nodes in use. */
> @@ -3266,6 +3270,12 @@ void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
>  	 * state.
>  	 */
>  	if (!success) {
> +		/*
> +		 * Please embed an rcu_head and pass it along if you hit this
> +		 * warning. Doing so would avoid long kfree_rcu() latencies.
> +		 */
> +		if (!rcu_kfree_nowarn)
> +			WARN_ON_ONCE(1);
>  		debug_rcu_head_unqueue(ptr);
>  		synchronize_rcu();
>  		kvfree(ptr);