On 12/8/20 10:45 AM, SeongJae Park wrote:
> From: SeongJae Park <sjpark@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> ---
> 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);
>
>
