On Tue, Dec 06, 2022 at 09:51:01PM +0100, Johannes Weiner wrote:
> On Tue, Dec 06, 2022 at 08:13:50PM +0100, Eric Dumazet wrote:
> > On Tue, Dec 6, 2022 at 8:00 PM Johannes Weiner <hannes@xxxxxxxxxxx> wrote:
> > > @@ -1701,10 +1701,10 @@ void mem_cgroup_sk_alloc(struct sock *sk);
> > > void mem_cgroup_sk_free(struct sock *sk);
> > > static inline bool mem_cgroup_under_socket_pressure(struct mem_cgroup *memcg)
> > > {
> > > - if (!cgroup_subsys_on_dfl(memory_cgrp_subsys) && memcg->tcpmem_pressure)
> > > + if (!cgroup_subsys_on_dfl(memory_cgrp_subsys) && memcg->socket_pressure)
> >
> > && READ_ONCE(memcg->socket_pressure))
> >
> > > return true;
> > > do {
> > > - if (time_before(jiffies, READ_ONCE(memcg->socket_pressure)))
> > > + if (memcg->socket_pressure)
> >
> > if (READ_ONCE(...))
>
> Good point, I'll add those.
>
> > > @@ -7195,10 +7194,10 @@ bool mem_cgroup_charge_skmem(struct mem_cgroup *memcg, unsigned int nr_pages,
> > > struct page_counter *fail;
> > >
> > > if (page_counter_try_charge(&memcg->tcpmem, nr_pages, &fail)) {
> > > - memcg->tcpmem_pressure = 0;
> >
> > Orthogonal to your patch, but:
> >
> > Maybe avoid touching this cache line too often and use READ/WRITE_ONCE() ?
> >
> > if (READ_ONCE(memcg->socket_pressure))
> > WRITE_ONCE(memcg->socket_pressure, false);
>
> Ah, that's a good idea.
>
> I think it'll be fine in the failure case, since that's associated
> with OOM and total performance breakdown anyway.
>
> But certainly, in the common case of the charge succeeding, we should
> not keep hammering false into that variable over and over.
>
> How about the delta below? I also flipped the branches around to keep
> the common path at the first indentation level, hopefully making that
> a bit clearer too.
>
> Thanks for taking a look, Eric!
>
I still think we should not put a persistent state of socket pressure on
unsuccessful charge which will only get reset on successful charge. I
think the better approach would be to limit the pressure state by time
window same as today but set it on charge path. Something like below:
diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index d3c8203cab6c..7bd88d443c42 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -287,7 +287,6 @@ struct mem_cgroup {
/* Legacy tcp memory accounting */
bool tcpmem_active;
- int tcpmem_pressure;
#ifdef CONFIG_MEMCG_KMEM
int kmemcg_id;
@@ -1712,8 +1711,6 @@ void mem_cgroup_sk_alloc(struct sock *sk);
void mem_cgroup_sk_free(struct sock *sk);
static inline bool mem_cgroup_under_socket_pressure(struct mem_cgroup *memcg)
{
- if (!cgroup_subsys_on_dfl(memory_cgrp_subsys) && memcg->tcpmem_pressure)
- return true;
do {
if (time_before(jiffies, READ_ONCE(memcg->socket_pressure)))
return true;
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 48c44229cf47..290444bcab84 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -5286,7 +5286,6 @@ static struct mem_cgroup *mem_cgroup_alloc(void)
vmpressure_init(&memcg->vmpressure);
INIT_LIST_HEAD(&memcg->event_list);
spin_lock_init(&memcg->event_list_lock);
- memcg->socket_pressure = jiffies;
#ifdef CONFIG_MEMCG_KMEM
memcg->kmemcg_id = -1;
INIT_LIST_HEAD(&memcg->objcg_list);
@@ -7252,10 +7251,12 @@ bool mem_cgroup_charge_skmem(struct mem_cgroup *memcg, unsigned int nr_pages,
struct page_counter *fail;
if (page_counter_try_charge(&memcg->tcpmem, nr_pages, &fail)) {
- memcg->tcpmem_pressure = 0;
+ if (READ_ONCE(memcg->socket_pressure))
+ WRITE_ONCE(memcg->socket_pressure, 0);
return true;
}
- memcg->tcpmem_pressure = 1;
+ if (READ_ONCE(memcg->socket_pressure) < jiffies + HZ)
+ WRITE_ONCE(memcg->socket_pressure, jiffies + HZ);
if (gfp_mask & __GFP_NOFAIL) {
page_counter_charge(&memcg->tcpmem, nr_pages);
return true;
@@ -7263,12 +7264,21 @@ bool mem_cgroup_charge_skmem(struct mem_cgroup *memcg, unsigned int nr_pages,
return false;
}
- if (try_charge(memcg, gfp_mask, nr_pages) == 0) {
- mod_memcg_state(memcg, MEMCG_SOCK, nr_pages);
- return true;
+ if (try_charge(memcg, gfp_mask & ~__GFP_NOFAIL, nr_pages) < 0) {
+ if (READ_ONCE(memcg->socket_pressure) < jiffies + HZ)
+ WRITE_ONCE(memcg->socket_pressure, jiffies + HZ);
+ if (gfp_mask & __GFP_NOFAIL) {
+ try_charge(memcg, gfp_mask, nr_pages);
+ goto out;
+ }
+ return false;
}
- return false;
+ if (READ_ONCE(memcg->socket_pressure))
+ WRITE_ONCE(memcg->socket_pressure, 0);
+out:
+ mod_memcg_state(memcg, MEMCG_SOCK, nr_pages);
+ return true;
}
/**
