On Wed, Jun 22, 2022 at 01:47:11PM +0200, Michal Hocko wrote:
> From: Michal Hocko <mhocko@xxxxxxxx>
>
> fill_page_cache_func allocates couple of pages to store
> kvfree_rcu_bulk_data. This is a lightweight (GFP_NORETRY) allocation
> which can fail under memory pressure. The function will, however keep
> retrying even when the previous attempt has failed.
>
> While this is not really incorrect there is one thing to consider. This
> allocation is invoked from the WQ context and that means that if the
> memory reclaim gets stuck it can hog the worker for quite some time.
> WQ concurrency is only triggered when the worker context sleeps and that
> is not guaranteed for __GFP_NORETRY allocation attempts (see
> should_reclaim_retry).
>
> We have seen WQ lockups
> kernel: BUG: workqueue lockup - pool cpus=93 node=1 flags=0x1 nice=0 stuck for 32s!
> [...]
> kernel: pool 74: cpus=37 node=0 flags=0x1 nice=0 hung=32s workers=2 manager: 2146
> kernel: pwq 498: cpus=249 node=1 flags=0x1 nice=0 active=4/256 refcnt=5
> kernel: in-flight: 1917:fill_page_cache_func
> kernel: pending: dbs_work_handler, free_work, kfree_rcu_monitor
>
> Originaly, we thought that several retries with direct reclaim being
> stuck is the underlying reason but we couldn't have confirmed that and
> have seen a similar lockups detected even without any heavy memory
> pressure so there is likely something else/more going on. On the other
> hand failing the allocation shouldn't have a big impact and from the
> code it is not really obvious why retrying is desirable so back off
> after the allocation failure.
>
> Cc: Uladzislau Rezki (Sony) <urezki@xxxxxxxxx>
> Cc: "Paul E. McKenney" <paulmck@xxxxxxxxxx>
> Cc: Frederic Weisbecker <frederic@xxxxxxxxxx>
> Cc: Neeraj Upadhyay <quic_neeraju@xxxxxxxxxxx>
> Cc: Josh Triplett <josh@xxxxxxxxxxxxxxxx>
> Cc: Steven Rostedt <rostedt@xxxxxxxxxxx>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@xxxxxxxxxxxx>
> Cc: Lai Jiangshan <jiangshanlai@xxxxxxxxx>
> Cc: Joel Fernandes <joel@xxxxxxxxxxxxxxxxx>
> Signed-off-by: Michal Hocko <mhocko@xxxxxxxx>
> ---
>
> Hi,
> I am sending this as an RFC because I couldn't prove that the WQ
> concurency issue as a result from the allocation retry is really a
> problem. On the other hand I couldn't see a good reason to retry after a
> previous failure. While the kswapd running in the background could have
> released some memory this is a not really guaranteed and mostly a
> wishful thinking.
>
> I do not understand the code well enough so I could be easily missing
> something. If the patch is a wrong thing to do then it would be really
> nice to add a comment why the retry is desirable and a good thing to do.
>
> The retry loop should be bound to rcu_min_cached_objs which is quite
> small but configurable so this can get large in some setups.
>
> Thanks
>
> kernel/rcu/tree.c | 17 +++++++++--------
> 1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index c25ba442044a..54a3a19c4c0b 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -3508,15 +3508,16 @@ static void fill_page_cache_func(struct work_struct *work)
> bnode = (struct kvfree_rcu_bulk_data *)
> __get_free_page(GFP_KERNEL | __GFP_NORETRY | __GFP_NOMEMALLOC | __GFP_NOWARN);
>
> - if (bnode) {
> - raw_spin_lock_irqsave(&krcp->lock, flags);
> - pushed = put_cached_bnode(krcp, bnode);
> - raw_spin_unlock_irqrestore(&krcp->lock, flags);
> + if (!bnode)
> + break;
>
> - if (!pushed) {
> - free_page((unsigned long) bnode);
> - break;
> - }
> + raw_spin_lock_irqsave(&krcp->lock, flags);
> + pushed = put_cached_bnode(krcp, bnode);
> + raw_spin_unlock_irqrestore(&krcp->lock, flags);
> +
> + if (!pushed) {
> + free_page((unsigned long) bnode);
> + break;
> }
> }
>
> --
> 2.30.2
>
OK. You would like to break the loop once an allocation does not succeed.
To me it also makes sense, i mean there is no reason to repeat it several
times that can lead to worqueue hogging.
Reviewed-by: Uladzislau Rezki (Sony) <urezki@xxxxxxxxx>
Thanks!
--
Uladzislau Rezki
