* [PATCH 0/1] net: Reduce rcu_barrier() contentions from 'unshare(CLONE_NEWNET)' @ 2020-12-08 9:45 SeongJae Park 2020-12-08 9:45 ` [PATCH 1/1] net/ipv4/inet_fragment: Batch fqdir destroy works SeongJae Park 0 siblings, 1 reply; 6+ messages in thread From: SeongJae Park @ 2020-12-08 9:45 UTC (permalink / raw) To: davem; +Cc: SeongJae Park, kuba, kuznet, paulmck, netdev, rcu, linux-kernel From: SeongJae Park <sjpark@amazon.de> On a few of our systems, I found frequent 'unshare(CLONE_NEWNET)' calls make the number of active slab objects including 'sock_inode_cache' type rapidly and continuously increase. As a result, memory pressure occurs. 'cleanup_net()' and 'fqdir_work_fn()' are functions that deallocate the relevant memory objects. They are asynchronously invoked by the work queues and internally use 'rcu_barrier()' to ensure safe destructions. 'cleanup_net()' works in a batched maneer in a single thread worker, while 'fqdir_work_fn()' works for each 'fqdir_exit()' call in the 'system_wq'. Therefore, 'fqdir_work_fn()' called frequently under the workload and made the contention for 'rcu_barrier()' high. In more detail, the global mutex, 'rcu_state.barrier_mutex' became the bottleneck. I tried making 'fqdir_work_fn()' batched and confirmed it works. The following patch is for the change. I think this is the right solution for point fix of this issue, but someone might blame different parts. 1. User: Frequent 'unshare()' calls From some point of view, such frequent 'unshare()' calls might seem only insane. 2. Global mutex in 'rcu_barrier()' Because of the global mutex, 'rcu_barrier()' callers could wait long even after the callbacks started before the call finished. Therefore, similar issues could happen in another 'rcu_barrier()' usages. Maybe we can use some wait queue like mechanism to notify the waiters when the desired time came. I personally believe applying the point fix for now and making 'rcu_barrier()' improvement in longterm make sense. If I'm missing something or you have different opinions, please feel free to let me know. SeongJae Park (1): net/ipv4/inet_fragment: Batch fqdir destroy works include/net/inet_frag.h | 2 +- net/ipv4/inet_fragment.c | 28 ++++++++++++++++++++-------- 2 files changed, 21 insertions(+), 9 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/1] net/ipv4/inet_fragment: Batch fqdir destroy works 2020-12-08 9:45 [PATCH 0/1] net: Reduce rcu_barrier() contentions from 'unshare(CLONE_NEWNET)' SeongJae Park @ 2020-12-08 9:45 ` SeongJae Park 2020-12-09 23:16 ` Jakub Kicinski 2020-12-10 0:17 ` Eric Dumazet 0 siblings, 2 replies; 6+ messages in thread From: SeongJae Park @ 2020-12-08 9:45 UTC (permalink / raw) To: davem; +Cc: SeongJae Park, kuba, kuznet, paulmck, netdev, rcu, linux-kernel From: SeongJae Park <sjpark@amazon.de> In 'fqdir_exit()', a work for destruction of the 'fqdir' is enqueued. The work function, 'fqdir_work_fn()', calls 'rcu_barrier()'. In case of intensive 'fqdir_exit()' (e.g., frequent 'unshare(CLONE_NEWNET)' systemcalls), this increased contention could result in unacceptably high latency of 'rcu_barrier()'. This commit avoids such contention by doing the destruction in batched manner, as similar to that of 'cleanup_net()'. Signed-off-by: SeongJae Park <sjpark@amazon.de> --- include/net/inet_frag.h | 2 +- net/ipv4/inet_fragment.c | 28 ++++++++++++++++++++-------- 2 files changed, 21 insertions(+), 9 deletions(-) diff --git a/include/net/inet_frag.h b/include/net/inet_frag.h index bac79e817776..558893d8810c 100644 --- a/include/net/inet_frag.h +++ b/include/net/inet_frag.h @@ -20,7 +20,7 @@ struct fqdir { /* Keep atomic mem on separate cachelines in structs that include it */ atomic_long_t mem ____cacheline_aligned_in_smp; - struct work_struct destroy_work; + struct llist_node destroy_list; }; /** diff --git a/net/ipv4/inet_fragment.c b/net/ipv4/inet_fragment.c index 10d31733297d..796b559137c5 100644 --- a/net/ipv4/inet_fragment.c +++ b/net/ipv4/inet_fragment.c @@ -145,12 +145,19 @@ static void inet_frags_free_cb(void *ptr, void *arg) inet_frag_destroy(fq); } +static LLIST_HEAD(destroy_list); + static void fqdir_work_fn(struct work_struct *work) { - struct fqdir *fqdir = container_of(work, struct fqdir, destroy_work); - struct inet_frags *f = fqdir->f; + struct llist_node *kill_list; + struct fqdir *fqdir; + struct inet_frags *f; + + /* Atomically snapshot the list of fqdirs to destroy */ + kill_list = llist_del_all(&destroy_list); - rhashtable_free_and_destroy(&fqdir->rhashtable, inet_frags_free_cb, NULL); + llist_for_each_entry(fqdir, kill_list, destroy_list) + rhashtable_free_and_destroy(&fqdir->rhashtable, inet_frags_free_cb, NULL); /* We need to make sure all ongoing call_rcu(..., inet_frag_destroy_rcu) * have completed, since they need to dereference fqdir. @@ -158,10 +165,13 @@ static void fqdir_work_fn(struct work_struct *work) */ rcu_barrier(); - if (refcount_dec_and_test(&f->refcnt)) - complete(&f->completion); + llist_for_each_entry(fqdir, kill_list, destroy_list) { + f = fqdir->f; + if (refcount_dec_and_test(&f->refcnt)) + complete(&f->completion); - kfree(fqdir); + kfree(fqdir); + } } int fqdir_init(struct fqdir **fqdirp, struct inet_frags *f, struct net *net) @@ -184,10 +194,12 @@ int fqdir_init(struct fqdir **fqdirp, struct inet_frags *f, struct net *net) } EXPORT_SYMBOL(fqdir_init); +static DECLARE_WORK(fqdir_destroy_work, fqdir_work_fn); + void fqdir_exit(struct fqdir *fqdir) { - INIT_WORK(&fqdir->destroy_work, fqdir_work_fn); - queue_work(system_wq, &fqdir->destroy_work); + if (llist_add(&fqdir->destroy_list, &destroy_list)) + queue_work(system_wq, &fqdir_destroy_work); } EXPORT_SYMBOL(fqdir_exit); -- 2.17.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] net/ipv4/inet_fragment: Batch fqdir destroy works 2020-12-08 9:45 ` [PATCH 1/1] net/ipv4/inet_fragment: Batch fqdir destroy works SeongJae Park @ 2020-12-09 23:16 ` Jakub Kicinski 2020-12-10 6:43 ` SeongJae Park 2020-12-10 0:17 ` Eric Dumazet 1 sibling, 1 reply; 6+ messages in thread From: Jakub Kicinski @ 2020-12-09 23:16 UTC (permalink / raw) To: SeongJae Park Cc: davem, SeongJae Park, kuznet, paulmck, netdev, rcu, linux-kernel On Tue, 8 Dec 2020 10:45:29 +0100 SeongJae Park wrote: > From: SeongJae Park <sjpark@amazon.de> > > In 'fqdir_exit()', a work for destruction of the 'fqdir' is enqueued. > The work function, 'fqdir_work_fn()', calls 'rcu_barrier()'. In case of > intensive 'fqdir_exit()' (e.g., frequent 'unshare(CLONE_NEWNET)' > systemcalls), this increased contention could result in unacceptably > high latency of 'rcu_barrier()'. This commit avoids such contention by > doing the destruction in batched manner, as similar to that of > 'cleanup_net()'. > > Signed-off-by: SeongJae Park <sjpark@amazon.de> Looks fine to me, but you haven't CCed Florian or Eric who where the last two people to touch this function. Please repost CCing them and fixing the nit below, thanks! > static void fqdir_work_fn(struct work_struct *work) > { > - struct fqdir *fqdir = container_of(work, struct fqdir, destroy_work); > - struct inet_frags *f = fqdir->f; > + struct llist_node *kill_list; > + struct fqdir *fqdir; > + struct inet_frags *f; nit: reorder fqdir and f to keep reverse xmas tree variable ordering. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] net/ipv4/inet_fragment: Batch fqdir destroy works 2020-12-09 23:16 ` Jakub Kicinski @ 2020-12-10 6:43 ` SeongJae Park 0 siblings, 0 replies; 6+ messages in thread From: SeongJae Park @ 2020-12-10 6:43 UTC (permalink / raw) To: Jakub Kicinski Cc: SeongJae Park, davem, SeongJae Park, kuznet, paulmck, netdev, rcu, linux-kernel On Wed, 9 Dec 2020 15:16:59 -0800 Jakub Kicinski <kuba@kernel.org> wrote: > On Tue, 8 Dec 2020 10:45:29 +0100 SeongJae Park wrote: > > From: SeongJae Park <sjpark@amazon.de> > > > > In 'fqdir_exit()', a work for destruction of the 'fqdir' is enqueued. > > The work function, 'fqdir_work_fn()', calls 'rcu_barrier()'. In case of > > intensive 'fqdir_exit()' (e.g., frequent 'unshare(CLONE_NEWNET)' > > systemcalls), this increased contention could result in unacceptably > > high latency of 'rcu_barrier()'. This commit avoids such contention by > > doing the destruction in batched manner, as similar to that of > > 'cleanup_net()'. > > > > Signed-off-by: SeongJae Park <sjpark@amazon.de> > > Looks fine to me, but you haven't CCed Florian or Eric who where the > last two people to touch this function. Please repost CCing them and > fixing the nit below, thanks! Thank you for let me know that. I will send the next version so. > > > static void fqdir_work_fn(struct work_struct *work) > > { > > - struct fqdir *fqdir = container_of(work, struct fqdir, destroy_work); > > - struct inet_frags *f = fqdir->f; > > + struct llist_node *kill_list; > > + struct fqdir *fqdir; > > + struct inet_frags *f; > > nit: reorder fqdir and f to keep reverse xmas tree variable ordering. Hehe, ok, I will. :) Thanks, SeongJae Park ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] net/ipv4/inet_fragment: Batch fqdir destroy works 2020-12-08 9:45 ` [PATCH 1/1] net/ipv4/inet_fragment: Batch fqdir destroy works SeongJae Park 2020-12-09 23:16 ` Jakub Kicinski @ 2020-12-10 0:17 ` Eric Dumazet 2020-12-10 7:27 ` SeongJae Park 1 sibling, 1 reply; 6+ messages in thread From: Eric Dumazet @ 2020-12-10 0:17 UTC (permalink / raw) To: SeongJae Park, davem Cc: SeongJae Park, kuba, kuznet, paulmck, netdev, rcu, linux-kernel On 12/8/20 10:45 AM, SeongJae Park wrote: > From: SeongJae Park <sjpark@amazon.de> > > In 'fqdir_exit()', a work for destruction of the 'fqdir' is enqueued. > The work function, 'fqdir_work_fn()', calls 'rcu_barrier()'. In case of > intensive 'fqdir_exit()' (e.g., frequent 'unshare(CLONE_NEWNET)' > systemcalls), this increased contention could result in unacceptably > high latency of 'rcu_barrier()'. This commit avoids such contention by > doing the destruction in batched manner, as similar to that of > 'cleanup_net()'. Any numbers to share ? I have never seen an issue. > > Signed-off-by: SeongJae Park <sjpark@amazon.de> > --- > include/net/inet_frag.h | 2 +- > net/ipv4/inet_fragment.c | 28 ++++++++++++++++++++-------- > 2 files changed, 21 insertions(+), 9 deletions(-) > > diff --git a/include/net/inet_frag.h b/include/net/inet_frag.h > index bac79e817776..558893d8810c 100644 > --- a/include/net/inet_frag.h > +++ b/include/net/inet_frag.h > @@ -20,7 +20,7 @@ struct fqdir { > > /* Keep atomic mem on separate cachelines in structs that include it */ > atomic_long_t mem ____cacheline_aligned_in_smp; > - struct work_struct destroy_work; > + struct llist_node destroy_list; > }; > > /** > diff --git a/net/ipv4/inet_fragment.c b/net/ipv4/inet_fragment.c > index 10d31733297d..796b559137c5 100644 > --- a/net/ipv4/inet_fragment.c > +++ b/net/ipv4/inet_fragment.c > @@ -145,12 +145,19 @@ static void inet_frags_free_cb(void *ptr, void *arg) > inet_frag_destroy(fq); > } > > +static LLIST_HEAD(destroy_list); > + > static void fqdir_work_fn(struct work_struct *work) > { > - struct fqdir *fqdir = container_of(work, struct fqdir, destroy_work); > - struct inet_frags *f = fqdir->f; > + struct llist_node *kill_list; > + struct fqdir *fqdir; > + struct inet_frags *f; > + > + /* Atomically snapshot the list of fqdirs to destroy */ > + kill_list = llist_del_all(&destroy_list); > > - rhashtable_free_and_destroy(&fqdir->rhashtable, inet_frags_free_cb, NULL); > + llist_for_each_entry(fqdir, kill_list, destroy_list) > + rhashtable_free_and_destroy(&fqdir->rhashtable, inet_frags_free_cb, NULL); > OK, it seems rhashtable_free_and_destroy() has cond_resched() so we are not going to hold this cpu for long periods. > /* We need to make sure all ongoing call_rcu(..., inet_frag_destroy_rcu) > * have completed, since they need to dereference fqdir. > @@ -158,10 +165,13 @@ static void fqdir_work_fn(struct work_struct *work) > */ > rcu_barrier(); > > - if (refcount_dec_and_test(&f->refcnt)) > - complete(&f->completion); > + llist_for_each_entry(fqdir, kill_list, destroy_list) { Don't we need the llist_for_each_entry_safe() variant here ??? > + f = fqdir->f; > + if (refcount_dec_and_test(&f->refcnt)) > + complete(&f->completion); > > - kfree(fqdir); > + kfree(fqdir); > + } > } > > int fqdir_init(struct fqdir **fqdirp, struct inet_frags *f, struct net *net) > @@ -184,10 +194,12 @@ int fqdir_init(struct fqdir **fqdirp, struct inet_frags *f, struct net *net) > } > EXPORT_SYMBOL(fqdir_init); > > +static DECLARE_WORK(fqdir_destroy_work, fqdir_work_fn); > + > void fqdir_exit(struct fqdir *fqdir) > { > - INIT_WORK(&fqdir->destroy_work, fqdir_work_fn); > - queue_work(system_wq, &fqdir->destroy_work); > + if (llist_add(&fqdir->destroy_list, &destroy_list)) > + queue_work(system_wq, &fqdir_destroy_work); > } > EXPORT_SYMBOL(fqdir_exit); > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] net/ipv4/inet_fragment: Batch fqdir destroy works 2020-12-10 0:17 ` Eric Dumazet @ 2020-12-10 7:27 ` SeongJae Park 0 siblings, 0 replies; 6+ messages in thread From: SeongJae Park @ 2020-12-10 7:27 UTC (permalink / raw) To: Eric Dumazet Cc: SeongJae Park, davem, SeongJae Park, kuba, kuznet, paulmck, netdev, rcu, linux-kernel On Thu, 10 Dec 2020 01:17:58 +0100 Eric Dumazet <eric.dumazet@gmail.com> wrote: > > > On 12/8/20 10:45 AM, SeongJae Park wrote: > > From: SeongJae Park <sjpark@amazon.de> > > > > In 'fqdir_exit()', a work for destruction of the 'fqdir' is enqueued. > > The work function, 'fqdir_work_fn()', calls 'rcu_barrier()'. In case of > > intensive 'fqdir_exit()' (e.g., frequent 'unshare(CLONE_NEWNET)' > > systemcalls), this increased contention could result in unacceptably > > high latency of 'rcu_barrier()'. This commit avoids such contention by > > doing the destruction in batched manner, as similar to that of > > 'cleanup_net()'. > > Any numbers to share ? I have never seen an issue. On our 40 CPU cores / 70GB DRAM machine, 15GB of available memory was reduced within 2 minutes while my artificial reproducer runs. The reproducer merely repeats 'unshare(CLONE_NEWNET)' in a loop for 50,000 times. The reproducer is not only artificial but resembles the behavior of our real workloads. While the reproducer runs, 'cleanup_net()' was called only 4 times. First two calls quickly finished, but third call took about 30 seconds, and the fourth call didn't finished until the reproducer finishes. We also confirmed the third and fourth calls just waiting for 'rcu_barrier()'. I think you've not seen this issue before because we are doing very intensive 'unshare()' calls. Also, this is not reproducible on every hardware. On my 6 CPU machine, the problem didn't reproduce. > > > > > Signed-off-by: SeongJae Park <sjpark@amazon.de> > > --- > > include/net/inet_frag.h | 2 +- > > net/ipv4/inet_fragment.c | 28 ++++++++++++++++++++-------- > > 2 files changed, 21 insertions(+), 9 deletions(-) > > > > diff --git a/include/net/inet_frag.h b/include/net/inet_frag.h > > index bac79e817776..558893d8810c 100644 > > --- a/include/net/inet_frag.h > > +++ b/include/net/inet_frag.h > > @@ -20,7 +20,7 @@ struct fqdir { > > > > /* Keep atomic mem on separate cachelines in structs that include it */ > > atomic_long_t mem ____cacheline_aligned_in_smp; > > - struct work_struct destroy_work; > > + struct llist_node destroy_list; > > }; > > > > /** > > diff --git a/net/ipv4/inet_fragment.c b/net/ipv4/inet_fragment.c > > index 10d31733297d..796b559137c5 100644 > > --- a/net/ipv4/inet_fragment.c > > +++ b/net/ipv4/inet_fragment.c > > @@ -145,12 +145,19 @@ static void inet_frags_free_cb(void *ptr, void *arg) > > inet_frag_destroy(fq); > > } > > > > +static LLIST_HEAD(destroy_list); > > + > > static void fqdir_work_fn(struct work_struct *work) > > { > > - struct fqdir *fqdir = container_of(work, struct fqdir, destroy_work); > > - struct inet_frags *f = fqdir->f; > > + struct llist_node *kill_list; > > + struct fqdir *fqdir; > > + struct inet_frags *f; > > + > > + /* Atomically snapshot the list of fqdirs to destroy */ > > + kill_list = llist_del_all(&destroy_list); > > > > - rhashtable_free_and_destroy(&fqdir->rhashtable, inet_frags_free_cb, NULL); > > + llist_for_each_entry(fqdir, kill_list, destroy_list) > > + rhashtable_free_and_destroy(&fqdir->rhashtable, inet_frags_free_cb, NULL); > > > > > OK, it seems rhashtable_free_and_destroy() has cond_resched() so we are not going > to hold this cpu for long periods. > > > /* We need to make sure all ongoing call_rcu(..., inet_frag_destroy_rcu) > > * have completed, since they need to dereference fqdir. > > @@ -158,10 +165,13 @@ static void fqdir_work_fn(struct work_struct *work) > > */ > > rcu_barrier(); > > > > - if (refcount_dec_and_test(&f->refcnt)) > > - complete(&f->completion); > > + llist_for_each_entry(fqdir, kill_list, destroy_list) { > > Don't we need the llist_for_each_entry_safe() variant here ??? Oh, indeed. I will do so in the next version. > > > + f = fqdir->f; > > + if (refcount_dec_and_test(&f->refcnt)) > > + complete(&f->completion); > > > > - kfree(fqdir); > > + kfree(fqdir); > > + } Thanks, SeongJae Park ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-12-10 7:28 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-12-08 9:45 [PATCH 0/1] net: Reduce rcu_barrier() contentions from 'unshare(CLONE_NEWNET)' SeongJae Park 2020-12-08 9:45 ` [PATCH 1/1] net/ipv4/inet_fragment: Batch fqdir destroy works SeongJae Park 2020-12-09 23:16 ` Jakub Kicinski 2020-12-10 6:43 ` SeongJae Park 2020-12-10 0:17 ` Eric Dumazet 2020-12-10 7:27 ` SeongJae Park
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).