All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net] net-memcg: avoid stalls when under memory pressure
@ 2022-10-21 16:03 ` Jakub Kicinski
  0 siblings, 0 replies; 14+ messages in thread
From: Jakub Kicinski @ 2022-10-21 16:03 UTC (permalink / raw)
  To: edumazet
  Cc: netdev, davem, pabeni, cgroups, roman.gushchin, Jakub Kicinski,
	Shakeel Butt, weiwan, ncardwell, ycheng

As Shakeel explains the commit under Fixes had the unintended
side-effect of no longer pre-loading the cached memory allowance.
Even tho we previously dropped the first packet received when
over memory limit - the consecutive ones would get thru by using
the cache. The charging was happening in batches of 128kB, so
we'd let in 128kB (truesize) worth of packets per one drop.

After the change we no longer force charge, there will be no
cache filling side effects. This causes significant drops and
connection stalls for workloads which use a lot of page cache,
since we can't reclaim page cache under GFP_NOWAIT.

Some of the latency can be recovered by improving SACK reneg
handling but nowhere near enough to get back to the pre-5.15
performance (the application I'm experimenting with still
sees 5-10x worst latency).

Apply the suggested workaround of using GFP_ATOMIC. We will now
be more permissive than previously as we'll drop _no_ packets
in softirq when under pressure. But I can't think of any good
and simple way to address that within networking.

Link: https://lore.kernel.org/all/20221012163300.795e7b86@kernel.org/
Suggested-by: Shakeel Butt <shakeelb@google.com>
Fixes: 4b1327be9fe5 ("net-memcg: pass in gfp_t mask to mem_cgroup_charge_skmem()")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: weiwan@google.com
CC: shakeelb@google.com
CC: ncardwell@google.com
CC: ycheng@google.com
---
 include/net/sock.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/net/sock.h b/include/net/sock.h
index 9e464f6409a7..22f8bab583dd 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -2585,7 +2585,7 @@ static inline gfp_t gfp_any(void)
 
 static inline gfp_t gfp_memcg_charge(void)
 {
-	return in_softirq() ? GFP_NOWAIT : GFP_KERNEL;
+	return in_softirq() ? GFP_ATOMIC : GFP_KERNEL;
 }
 
 static inline long sock_rcvtimeo(const struct sock *sk, bool noblock)
-- 
2.37.3


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

* [PATCH net] net-memcg: avoid stalls when under memory pressure
@ 2022-10-21 16:03 ` Jakub Kicinski
  0 siblings, 0 replies; 14+ messages in thread
From: Jakub Kicinski @ 2022-10-21 16:03 UTC (permalink / raw)
  To: edumazet-hpIqsD4AKlfQT0dZR+AlfA
  Cc: netdev-u79uwXL29TY76Z2rM5mHXA, davem-fT/PcQaiUtIeIZ0/mPfg9Q,
	pabeni-H+wXaHxf7aLQT0dZR+AlfA, cgroups-u79uwXL29TY76Z2rM5mHXA,
	roman.gushchin-fxUVXftIFDnyG1zEObXtfA, Jakub Kicinski,
	Shakeel Butt, weiwan-hpIqsD4AKlfQT0dZR+AlfA,
	ncardwell-hpIqsD4AKlfQT0dZR+AlfA, ycheng-hpIqsD4AKlfQT0dZR+AlfA

As Shakeel explains the commit under Fixes had the unintended
side-effect of no longer pre-loading the cached memory allowance.
Even tho we previously dropped the first packet received when
over memory limit - the consecutive ones would get thru by using
the cache. The charging was happening in batches of 128kB, so
we'd let in 128kB (truesize) worth of packets per one drop.

After the change we no longer force charge, there will be no
cache filling side effects. This causes significant drops and
connection stalls for workloads which use a lot of page cache,
since we can't reclaim page cache under GFP_NOWAIT.

Some of the latency can be recovered by improving SACK reneg
handling but nowhere near enough to get back to the pre-5.15
performance (the application I'm experimenting with still
sees 5-10x worst latency).

Apply the suggested workaround of using GFP_ATOMIC. We will now
be more permissive than previously as we'll drop _no_ packets
in softirq when under pressure. But I can't think of any good
and simple way to address that within networking.

Link: https://lore.kernel.org/all/20221012163300.795e7b86-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org/
Suggested-by: Shakeel Butt <shakeelb-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Fixes: 4b1327be9fe5 ("net-memcg: pass in gfp_t mask to mem_cgroup_charge_skmem()")
Signed-off-by: Jakub Kicinski <kuba-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
CC: weiwan-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org
CC: shakeelb-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org
CC: ncardwell-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org
CC: ycheng-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org
---
 include/net/sock.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/net/sock.h b/include/net/sock.h
index 9e464f6409a7..22f8bab583dd 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -2585,7 +2585,7 @@ static inline gfp_t gfp_any(void)
 
 static inline gfp_t gfp_memcg_charge(void)
 {
-	return in_softirq() ? GFP_NOWAIT : GFP_KERNEL;
+	return in_softirq() ? GFP_ATOMIC : GFP_KERNEL;
 }
 
 static inline long sock_rcvtimeo(const struct sock *sk, bool noblock)
-- 
2.37.3


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

* Re: [PATCH net] net-memcg: avoid stalls when under memory pressure
  2022-10-21 16:03 ` Jakub Kicinski
  (?)
@ 2022-10-21 16:25 ` Shakeel Butt
  2022-10-21 16:28     ` Eric Dumazet
  -1 siblings, 1 reply; 14+ messages in thread
From: Shakeel Butt @ 2022-10-21 16:25 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: edumazet, netdev, davem, pabeni, cgroups, roman.gushchin, weiwan,
	ncardwell, ycheng

On Fri, Oct 21, 2022 at 9:03 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> As Shakeel explains the commit under Fixes had the unintended
> side-effect of no longer pre-loading the cached memory allowance.
> Even tho we previously dropped the first packet received when
> over memory limit - the consecutive ones would get thru by using
> the cache. The charging was happening in batches of 128kB, so
> we'd let in 128kB (truesize) worth of packets per one drop.
>
> After the change we no longer force charge, there will be no
> cache filling side effects. This causes significant drops and
> connection stalls for workloads which use a lot of page cache,
> since we can't reclaim page cache under GFP_NOWAIT.
>
> Some of the latency can be recovered by improving SACK reneg
> handling but nowhere near enough to get back to the pre-5.15
> performance (the application I'm experimenting with still
> sees 5-10x worst latency).
>
> Apply the suggested workaround of using GFP_ATOMIC. We will now
> be more permissive than previously as we'll drop _no_ packets
> in softirq when under pressure. But I can't think of any good
> and simple way to address that within networking.
>
> Link: https://lore.kernel.org/all/20221012163300.795e7b86@kernel.org/
> Suggested-by: Shakeel Butt <shakeelb@google.com>
> Fixes: 4b1327be9fe5 ("net-memcg: pass in gfp_t mask to mem_cgroup_charge_skmem()")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: weiwan@google.com
> CC: shakeelb@google.com
> CC: ncardwell@google.com
> CC: ycheng@google.com
> ---
>  include/net/sock.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/net/sock.h b/include/net/sock.h
> index 9e464f6409a7..22f8bab583dd 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -2585,7 +2585,7 @@ static inline gfp_t gfp_any(void)
>
>  static inline gfp_t gfp_memcg_charge(void)
>  {
> -       return in_softirq() ? GFP_NOWAIT : GFP_KERNEL;
> +       return in_softirq() ? GFP_ATOMIC : GFP_KERNEL;
>  }
>

How about just using gfp_any() and we can remove gfp_memcg_charge()?

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

* Re: [PATCH net] net-memcg: avoid stalls when under memory pressure
@ 2022-10-21 16:28     ` Eric Dumazet
  0 siblings, 0 replies; 14+ messages in thread
From: Eric Dumazet @ 2022-10-21 16:28 UTC (permalink / raw)
  To: Shakeel Butt
  Cc: Jakub Kicinski, netdev, davem, pabeni, cgroups, roman.gushchin,
	weiwan, ncardwell, ycheng

On Fri, Oct 21, 2022 at 9:26 AM Shakeel Butt <shakeelb@google.com> wrote:
>
> On Fri, Oct 21, 2022 at 9:03 AM Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > As Shakeel explains the commit under Fixes had the unintended
> > side-effect of no longer pre-loading the cached memory allowance.
> > Even tho we previously dropped the first packet received when
> > over memory limit - the consecutive ones would get thru by using
> > the cache. The charging was happening in batches of 128kB, so
> > we'd let in 128kB (truesize) worth of packets per one drop.
> >
> > After the change we no longer force charge, there will be no
> > cache filling side effects. This causes significant drops and
> > connection stalls for workloads which use a lot of page cache,
> > since we can't reclaim page cache under GFP_NOWAIT.
> >
> > Some of the latency can be recovered by improving SACK reneg
> > handling but nowhere near enough to get back to the pre-5.15
> > performance (the application I'm experimenting with still
> > sees 5-10x worst latency).
> >
> > Apply the suggested workaround of using GFP_ATOMIC. We will now
> > be more permissive than previously as we'll drop _no_ packets
> > in softirq when under pressure. But I can't think of any good
> > and simple way to address that within networking.
> >
> > Link: https://lore.kernel.org/all/20221012163300.795e7b86@kernel.org/
> > Suggested-by: Shakeel Butt <shakeelb@google.com>
> > Fixes: 4b1327be9fe5 ("net-memcg: pass in gfp_t mask to mem_cgroup_charge_skmem()")
> > Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> > ---
> > CC: weiwan@google.com
> > CC: shakeelb@google.com
> > CC: ncardwell@google.com
> > CC: ycheng@google.com
> > ---
> >  include/net/sock.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/include/net/sock.h b/include/net/sock.h
> > index 9e464f6409a7..22f8bab583dd 100644
> > --- a/include/net/sock.h
> > +++ b/include/net/sock.h
> > @@ -2585,7 +2585,7 @@ static inline gfp_t gfp_any(void)
> >
> >  static inline gfp_t gfp_memcg_charge(void)
> >  {
> > -       return in_softirq() ? GFP_NOWAIT : GFP_KERNEL;
> > +       return in_softirq() ? GFP_ATOMIC : GFP_KERNEL;
> >  }
> >
>
> How about just using gfp_any() and we can remove gfp_memcg_charge()?

How about keeping gfp_memcg_charge() and adding a comment on its intent ?

gfp_any() is very generic :/

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

* Re: [PATCH net] net-memcg: avoid stalls when under memory pressure
@ 2022-10-21 16:28     ` Eric Dumazet
  0 siblings, 0 replies; 14+ messages in thread
From: Eric Dumazet @ 2022-10-21 16:28 UTC (permalink / raw)
  To: Shakeel Butt
  Cc: Jakub Kicinski, netdev-u79uwXL29TY76Z2rM5mHXA,
	davem-fT/PcQaiUtIeIZ0/mPfg9Q, pabeni-H+wXaHxf7aLQT0dZR+AlfA,
	cgroups-u79uwXL29TY76Z2rM5mHXA,
	roman.gushchin-fxUVXftIFDnyG1zEObXtfA,
	weiwan-hpIqsD4AKlfQT0dZR+AlfA, ncardwell-hpIqsD4AKlfQT0dZR+AlfA,
	ycheng-hpIqsD4AKlfQT0dZR+AlfA

On Fri, Oct 21, 2022 at 9:26 AM Shakeel Butt <shakeelb-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
>
> On Fri, Oct 21, 2022 at 9:03 AM Jakub Kicinski <kuba-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> >
> > As Shakeel explains the commit under Fixes had the unintended
> > side-effect of no longer pre-loading the cached memory allowance.
> > Even tho we previously dropped the first packet received when
> > over memory limit - the consecutive ones would get thru by using
> > the cache. The charging was happening in batches of 128kB, so
> > we'd let in 128kB (truesize) worth of packets per one drop.
> >
> > After the change we no longer force charge, there will be no
> > cache filling side effects. This causes significant drops and
> > connection stalls for workloads which use a lot of page cache,
> > since we can't reclaim page cache under GFP_NOWAIT.
> >
> > Some of the latency can be recovered by improving SACK reneg
> > handling but nowhere near enough to get back to the pre-5.15
> > performance (the application I'm experimenting with still
> > sees 5-10x worst latency).
> >
> > Apply the suggested workaround of using GFP_ATOMIC. We will now
> > be more permissive than previously as we'll drop _no_ packets
> > in softirq when under pressure. But I can't think of any good
> > and simple way to address that within networking.
> >
> > Link: https://lore.kernel.org/all/20221012163300.795e7b86-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org/
> > Suggested-by: Shakeel Butt <shakeelb-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> > Fixes: 4b1327be9fe5 ("net-memcg: pass in gfp_t mask to mem_cgroup_charge_skmem()")
> > Signed-off-by: Jakub Kicinski <kuba-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > ---
> > CC: weiwan-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org
> > CC: shakeelb-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org
> > CC: ncardwell-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org
> > CC: ycheng-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org
> > ---
> >  include/net/sock.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/include/net/sock.h b/include/net/sock.h
> > index 9e464f6409a7..22f8bab583dd 100644
> > --- a/include/net/sock.h
> > +++ b/include/net/sock.h
> > @@ -2585,7 +2585,7 @@ static inline gfp_t gfp_any(void)
> >
> >  static inline gfp_t gfp_memcg_charge(void)
> >  {
> > -       return in_softirq() ? GFP_NOWAIT : GFP_KERNEL;
> > +       return in_softirq() ? GFP_ATOMIC : GFP_KERNEL;
> >  }
> >
>
> How about just using gfp_any() and we can remove gfp_memcg_charge()?

How about keeping gfp_memcg_charge() and adding a comment on its intent ?

gfp_any() is very generic :/

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

* Re: [PATCH net] net-memcg: avoid stalls when under memory pressure
@ 2022-10-21 16:34       ` Shakeel Butt
  0 siblings, 0 replies; 14+ messages in thread
From: Shakeel Butt @ 2022-10-21 16:34 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: Jakub Kicinski, netdev, davem, pabeni, cgroups, roman.gushchin,
	weiwan, ncardwell, ycheng

On Fri, Oct 21, 2022 at 9:28 AM Eric Dumazet <edumazet@google.com> wrote:
>
> On Fri, Oct 21, 2022 at 9:26 AM Shakeel Butt <shakeelb@google.com> wrote:
> >
> > On Fri, Oct 21, 2022 at 9:03 AM Jakub Kicinski <kuba@kernel.org> wrote:
> > >
> > > As Shakeel explains the commit under Fixes had the unintended
> > > side-effect of no longer pre-loading the cached memory allowance.
> > > Even tho we previously dropped the first packet received when
> > > over memory limit - the consecutive ones would get thru by using
> > > the cache. The charging was happening in batches of 128kB, so
> > > we'd let in 128kB (truesize) worth of packets per one drop.
> > >
> > > After the change we no longer force charge, there will be no
> > > cache filling side effects. This causes significant drops and
> > > connection stalls for workloads which use a lot of page cache,
> > > since we can't reclaim page cache under GFP_NOWAIT.
> > >
> > > Some of the latency can be recovered by improving SACK reneg
> > > handling but nowhere near enough to get back to the pre-5.15
> > > performance (the application I'm experimenting with still
> > > sees 5-10x worst latency).
> > >
> > > Apply the suggested workaround of using GFP_ATOMIC. We will now
> > > be more permissive than previously as we'll drop _no_ packets
> > > in softirq when under pressure. But I can't think of any good
> > > and simple way to address that within networking.
> > >
> > > Link: https://lore.kernel.org/all/20221012163300.795e7b86@kernel.org/
> > > Suggested-by: Shakeel Butt <shakeelb@google.com>
> > > Fixes: 4b1327be9fe5 ("net-memcg: pass in gfp_t mask to mem_cgroup_charge_skmem()")
> > > Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> > > ---
> > > CC: weiwan@google.com
> > > CC: shakeelb@google.com
> > > CC: ncardwell@google.com
> > > CC: ycheng@google.com
> > > ---
> > >  include/net/sock.h | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/include/net/sock.h b/include/net/sock.h
> > > index 9e464f6409a7..22f8bab583dd 100644
> > > --- a/include/net/sock.h
> > > +++ b/include/net/sock.h
> > > @@ -2585,7 +2585,7 @@ static inline gfp_t gfp_any(void)
> > >
> > >  static inline gfp_t gfp_memcg_charge(void)
> > >  {
> > > -       return in_softirq() ? GFP_NOWAIT : GFP_KERNEL;
> > > +       return in_softirq() ? GFP_ATOMIC : GFP_KERNEL;
> > >  }
> > >
> >
> > How about just using gfp_any() and we can remove gfp_memcg_charge()?
>
> How about keeping gfp_memcg_charge() and adding a comment on its intent ?
>
> gfp_any() is very generic :/

SGTM.

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

* Re: [PATCH net] net-memcg: avoid stalls when under memory pressure
@ 2022-10-21 16:34       ` Shakeel Butt
  0 siblings, 0 replies; 14+ messages in thread
From: Shakeel Butt @ 2022-10-21 16:34 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: Jakub Kicinski, netdev-u79uwXL29TY76Z2rM5mHXA,
	davem-fT/PcQaiUtIeIZ0/mPfg9Q, pabeni-H+wXaHxf7aLQT0dZR+AlfA,
	cgroups-u79uwXL29TY76Z2rM5mHXA,
	roman.gushchin-fxUVXftIFDnyG1zEObXtfA,
	weiwan-hpIqsD4AKlfQT0dZR+AlfA, ncardwell-hpIqsD4AKlfQT0dZR+AlfA,
	ycheng-hpIqsD4AKlfQT0dZR+AlfA

On Fri, Oct 21, 2022 at 9:28 AM Eric Dumazet <edumazet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
>
> On Fri, Oct 21, 2022 at 9:26 AM Shakeel Butt <shakeelb-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
> >
> > On Fri, Oct 21, 2022 at 9:03 AM Jakub Kicinski <kuba-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> > >
> > > As Shakeel explains the commit under Fixes had the unintended
> > > side-effect of no longer pre-loading the cached memory allowance.
> > > Even tho we previously dropped the first packet received when
> > > over memory limit - the consecutive ones would get thru by using
> > > the cache. The charging was happening in batches of 128kB, so
> > > we'd let in 128kB (truesize) worth of packets per one drop.
> > >
> > > After the change we no longer force charge, there will be no
> > > cache filling side effects. This causes significant drops and
> > > connection stalls for workloads which use a lot of page cache,
> > > since we can't reclaim page cache under GFP_NOWAIT.
> > >
> > > Some of the latency can be recovered by improving SACK reneg
> > > handling but nowhere near enough to get back to the pre-5.15
> > > performance (the application I'm experimenting with still
> > > sees 5-10x worst latency).
> > >
> > > Apply the suggested workaround of using GFP_ATOMIC. We will now
> > > be more permissive than previously as we'll drop _no_ packets
> > > in softirq when under pressure. But I can't think of any good
> > > and simple way to address that within networking.
> > >
> > > Link: https://lore.kernel.org/all/20221012163300.795e7b86-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org/
> > > Suggested-by: Shakeel Butt <shakeelb-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> > > Fixes: 4b1327be9fe5 ("net-memcg: pass in gfp_t mask to mem_cgroup_charge_skmem()")
> > > Signed-off-by: Jakub Kicinski <kuba-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > > ---
> > > CC: weiwan-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org
> > > CC: shakeelb-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org
> > > CC: ncardwell-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org
> > > CC: ycheng-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org
> > > ---
> > >  include/net/sock.h | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/include/net/sock.h b/include/net/sock.h
> > > index 9e464f6409a7..22f8bab583dd 100644
> > > --- a/include/net/sock.h
> > > +++ b/include/net/sock.h
> > > @@ -2585,7 +2585,7 @@ static inline gfp_t gfp_any(void)
> > >
> > >  static inline gfp_t gfp_memcg_charge(void)
> > >  {
> > > -       return in_softirq() ? GFP_NOWAIT : GFP_KERNEL;
> > > +       return in_softirq() ? GFP_ATOMIC : GFP_KERNEL;
> > >  }
> > >
> >
> > How about just using gfp_any() and we can remove gfp_memcg_charge()?
>
> How about keeping gfp_memcg_charge() and adding a comment on its intent ?
>
> gfp_any() is very generic :/

SGTM.

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

* Re: [PATCH net] net-memcg: avoid stalls when under memory pressure
@ 2022-10-21 17:40         ` Jakub Kicinski
  0 siblings, 0 replies; 14+ messages in thread
From: Jakub Kicinski @ 2022-10-21 17:40 UTC (permalink / raw)
  To: Shakeel Butt
  Cc: Eric Dumazet, netdev, davem, pabeni, cgroups, roman.gushchin,
	weiwan, ncardwell, ycheng

On Fri, 21 Oct 2022 09:34:20 -0700 Shakeel Butt wrote:
> > > How about just using gfp_any() and we can remove gfp_memcg_charge()?  
> >
> > How about keeping gfp_memcg_charge() and adding a comment on its intent ?
> >
> > gfp_any() is very generic :/  

That was my thinking, and I'm not sure what I could put in a comment.
Wouldn't it be some mix of words 'flags', 'memory', 'cgroup' and
'charge'... which is just spelling out the name of the function? 

I mean:

	/* Alloc flags for passing to cgroup socket memory charging */

does not add much value, right? 

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

* Re: [PATCH net] net-memcg: avoid stalls when under memory pressure
@ 2022-10-21 17:40         ` Jakub Kicinski
  0 siblings, 0 replies; 14+ messages in thread
From: Jakub Kicinski @ 2022-10-21 17:40 UTC (permalink / raw)
  To: Shakeel Butt
  Cc: Eric Dumazet, netdev-u79uwXL29TY76Z2rM5mHXA,
	davem-fT/PcQaiUtIeIZ0/mPfg9Q, pabeni-H+wXaHxf7aLQT0dZR+AlfA,
	cgroups-u79uwXL29TY76Z2rM5mHXA,
	roman.gushchin-fxUVXftIFDnyG1zEObXtfA,
	weiwan-hpIqsD4AKlfQT0dZR+AlfA, ncardwell-hpIqsD4AKlfQT0dZR+AlfA,
	ycheng-hpIqsD4AKlfQT0dZR+AlfA

On Fri, 21 Oct 2022 09:34:20 -0700 Shakeel Butt wrote:
> > > How about just using gfp_any() and we can remove gfp_memcg_charge()?  
> >
> > How about keeping gfp_memcg_charge() and adding a comment on its intent ?
> >
> > gfp_any() is very generic :/  

That was my thinking, and I'm not sure what I could put in a comment.
Wouldn't it be some mix of words 'flags', 'memory', 'cgroup' and
'charge'... which is just spelling out the name of the function? 

I mean:

	/* Alloc flags for passing to cgroup socket memory charging */

does not add much value, right? 

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

* Re: [PATCH net] net-memcg: avoid stalls when under memory pressure
  2022-10-21 17:40         ` Jakub Kicinski
  (?)
@ 2022-10-21 18:53         ` Shakeel Butt
  -1 siblings, 0 replies; 14+ messages in thread
From: Shakeel Butt @ 2022-10-21 18:53 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Eric Dumazet, netdev, davem, pabeni, cgroups, roman.gushchin,
	weiwan, ncardwell, ycheng

On Fri, Oct 21, 2022 at 10:40 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Fri, 21 Oct 2022 09:34:20 -0700 Shakeel Butt wrote:
> > > > How about just using gfp_any() and we can remove gfp_memcg_charge()?
> > >
> > > How about keeping gfp_memcg_charge() and adding a comment on its intent ?
> > >
> > > gfp_any() is very generic :/
>
> That was my thinking, and I'm not sure what I could put in a comment.
> Wouldn't it be some mix of words 'flags', 'memory', 'cgroup' and
> 'charge'... which is just spelling out the name of the function?
>
> I mean:
>
>         /* Alloc flags for passing to cgroup socket memory charging */
>
> does not add much value, right?

Yeah I agree. Let's just go with the original patch.

You can add:

Acked-by: Shakeel Butt <shakeelb@google.com>

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

* Re: [PATCH net] net-memcg: avoid stalls when under memory pressure
@ 2022-10-24 16:02   ` Roman Gushchin
  0 siblings, 0 replies; 14+ messages in thread
From: Roman Gushchin @ 2022-10-24 16:02 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: edumazet, netdev, davem, pabeni, cgroups, Shakeel Butt, weiwan,
	ncardwell, ycheng

On Fri, Oct 21, 2022 at 09:03:04AM -0700, Jakub Kicinski wrote:
> As Shakeel explains the commit under Fixes had the unintended
> side-effect of no longer pre-loading the cached memory allowance.
> Even tho we previously dropped the first packet received when
> over memory limit - the consecutive ones would get thru by using
> the cache. The charging was happening in batches of 128kB, so
> we'd let in 128kB (truesize) worth of packets per one drop.
> 
> After the change we no longer force charge, there will be no
> cache filling side effects. This causes significant drops and
> connection stalls for workloads which use a lot of page cache,
> since we can't reclaim page cache under GFP_NOWAIT.
> 
> Some of the latency can be recovered by improving SACK reneg
> handling but nowhere near enough to get back to the pre-5.15
> performance (the application I'm experimenting with still
> sees 5-10x worst latency).
> 
> Apply the suggested workaround of using GFP_ATOMIC. We will now
> be more permissive than previously as we'll drop _no_ packets
> in softirq when under pressure. But I can't think of any good
> and simple way to address that within networking.
> 
> Link: https://lore.kernel.org/all/20221012163300.795e7b86@kernel.org/
> Suggested-by: Shakeel Butt <shakeelb@google.com>
> Fixes: 4b1327be9fe5 ("net-memcg: pass in gfp_t mask to mem_cgroup_charge_skmem()")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Acked-by: Roman Gushchin <roman.gushchin@linux.dev>

Thanks!

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

* Re: [PATCH net] net-memcg: avoid stalls when under memory pressure
@ 2022-10-24 16:02   ` Roman Gushchin
  0 siblings, 0 replies; 14+ messages in thread
From: Roman Gushchin @ 2022-10-24 16:02 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: edumazet-hpIqsD4AKlfQT0dZR+AlfA, netdev-u79uwXL29TY76Z2rM5mHXA,
	davem-fT/PcQaiUtIeIZ0/mPfg9Q, pabeni-H+wXaHxf7aLQT0dZR+AlfA,
	cgroups-u79uwXL29TY76Z2rM5mHXA, Shakeel Butt,
	weiwan-hpIqsD4AKlfQT0dZR+AlfA, ncardwell-hpIqsD4AKlfQT0dZR+AlfA,
	ycheng-hpIqsD4AKlfQT0dZR+AlfA

On Fri, Oct 21, 2022 at 09:03:04AM -0700, Jakub Kicinski wrote:
> As Shakeel explains the commit under Fixes had the unintended
> side-effect of no longer pre-loading the cached memory allowance.
> Even tho we previously dropped the first packet received when
> over memory limit - the consecutive ones would get thru by using
> the cache. The charging was happening in batches of 128kB, so
> we'd let in 128kB (truesize) worth of packets per one drop.
> 
> After the change we no longer force charge, there will be no
> cache filling side effects. This causes significant drops and
> connection stalls for workloads which use a lot of page cache,
> since we can't reclaim page cache under GFP_NOWAIT.
> 
> Some of the latency can be recovered by improving SACK reneg
> handling but nowhere near enough to get back to the pre-5.15
> performance (the application I'm experimenting with still
> sees 5-10x worst latency).
> 
> Apply the suggested workaround of using GFP_ATOMIC. We will now
> be more permissive than previously as we'll drop _no_ packets
> in softirq when under pressure. But I can't think of any good
> and simple way to address that within networking.
> 
> Link: https://lore.kernel.org/all/20221012163300.795e7b86-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org/
> Suggested-by: Shakeel Butt <shakeelb-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> Fixes: 4b1327be9fe5 ("net-memcg: pass in gfp_t mask to mem_cgroup_charge_skmem()")
> Signed-off-by: Jakub Kicinski <kuba-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

Acked-by: Roman Gushchin <roman.gushchin-fxUVXftIFDnyG1zEObXtfA@public.gmane.org>

Thanks!

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

* Re: [PATCH net] net-memcg: avoid stalls when under memory pressure
@ 2022-10-24 18:20   ` patchwork-bot+netdevbpf-DgEjT+Ai2ygdnm+yROfE0A
  0 siblings, 0 replies; 14+ messages in thread
From: patchwork-bot+netdevbpf @ 2022-10-24 18:20 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: edumazet, netdev, davem, pabeni, cgroups, roman.gushchin,
	shakeelb, weiwan, ncardwell, ycheng

Hello:

This patch was applied to netdev/net.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Fri, 21 Oct 2022 09:03:04 -0700 you wrote:
> As Shakeel explains the commit under Fixes had the unintended
> side-effect of no longer pre-loading the cached memory allowance.
> Even tho we previously dropped the first packet received when
> over memory limit - the consecutive ones would get thru by using
> the cache. The charging was happening in batches of 128kB, so
> we'd let in 128kB (truesize) worth of packets per one drop.
> 
> [...]

Here is the summary with links:
  - [net] net-memcg: avoid stalls when under memory pressure
    https://git.kernel.org/netdev/net/c/720ca52bcef2

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



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

* Re: [PATCH net] net-memcg: avoid stalls when under memory pressure
@ 2022-10-24 18:20   ` patchwork-bot+netdevbpf-DgEjT+Ai2ygdnm+yROfE0A
  0 siblings, 0 replies; 14+ messages in thread
From: patchwork-bot+netdevbpf-DgEjT+Ai2ygdnm+yROfE0A @ 2022-10-24 18:20 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: edumazet-hpIqsD4AKlfQT0dZR+AlfA, netdev-u79uwXL29TY76Z2rM5mHXA,
	davem-fT/PcQaiUtIeIZ0/mPfg9Q, pabeni-H+wXaHxf7aLQT0dZR+AlfA,
	cgroups-u79uwXL29TY76Z2rM5mHXA,
	roman.gushchin-fxUVXftIFDnyG1zEObXtfA,
	shakeelb-hpIqsD4AKlfQT0dZR+AlfA, weiwan-hpIqsD4AKlfQT0dZR+AlfA,
	ncardwell-hpIqsD4AKlfQT0dZR+AlfA, ycheng-hpIqsD4AKlfQT0dZR+AlfA

Hello:

This patch was applied to netdev/net.git (master)
by Jakub Kicinski <kuba-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>:

On Fri, 21 Oct 2022 09:03:04 -0700 you wrote:
> As Shakeel explains the commit under Fixes had the unintended
> side-effect of no longer pre-loading the cached memory allowance.
> Even tho we previously dropped the first packet received when
> over memory limit - the consecutive ones would get thru by using
> the cache. The charging was happening in batches of 128kB, so
> we'd let in 128kB (truesize) worth of packets per one drop.
> 
> [...]

Here is the summary with links:
  - [net] net-memcg: avoid stalls when under memory pressure
    https://git.kernel.org/netdev/net/c/720ca52bcef2

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



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

end of thread, other threads:[~2022-10-24 19:59 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-21 16:03 [PATCH net] net-memcg: avoid stalls when under memory pressure Jakub Kicinski
2022-10-21 16:03 ` Jakub Kicinski
2022-10-21 16:25 ` Shakeel Butt
2022-10-21 16:28   ` Eric Dumazet
2022-10-21 16:28     ` Eric Dumazet
2022-10-21 16:34     ` Shakeel Butt
2022-10-21 16:34       ` Shakeel Butt
2022-10-21 17:40       ` Jakub Kicinski
2022-10-21 17:40         ` Jakub Kicinski
2022-10-21 18:53         ` Shakeel Butt
2022-10-24 16:02 ` Roman Gushchin
2022-10-24 16:02   ` Roman Gushchin
2022-10-24 18:20 ` patchwork-bot+netdevbpf
2022-10-24 18:20   ` patchwork-bot+netdevbpf-DgEjT+Ai2ygdnm+yROfE0A

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.