On Tue, May 28, 2024 at 03:21:02PM +0200, Uladzislau Rezki wrote:
> On Tue, May 28, 2024 at 02:08:20PM +0200, Julia Lawall wrote:
> >
> >
> > On Tue, 28 May 2024, Uladzislau Rezki wrote:
> >
> > > Hello.
> > >
> > > >
> > > >
> > > > On Mon, 27 May 2024, Paul E. McKenney wrote:
> > > >
> > > > > On Mon, May 27, 2024 at 10:13:40AM +0200, Julia Lawall wrote:
> > > > > >
> > > > > >
> > > > > > On Mon, 27 May 2024, Vlastimil Babka wrote:
> > > > > >
> > > > > > > Hi,
> > > > > > >
> > > > > > > one bit from LSF/MM discussions is that there might be call_rcu users with a
> > > > > > > callback that only does a kmem_cache_free() to a specific cache. Since SLOB
> > > > > > > was removed, it's always ok to use kfree() and thus also kfree_rcu() on
> > > > > > > allocations from kmem_cache_alloc() in addition to kmalloc(). Thus, such
> > > > > > > call_rcu() users might be simplified to kfree_rcu(). I found some cases
> > > > > > > semi-manually, but I'd expect coccinelle could help here so if anyone wants
> > > > > > > to take this task, feel free to.
> > > > > >
> > > > > > Thanks for the suggestion! I will try to look into it.
> > > > >
> > > > > Thank you both!
> > > >
> > > > I found the following functions. Do you want some other information, such
> > > > as where they are called from?
> > > >
> > > > Ignore the -s at the beginning of some lines. Those are for emphasis. not
> > > > to suggest to remove the code.
> > > >
> > > > I checked that the functions are only used in calls to call_rcu.
> > > >
> > > > Without more effort, Coccinelle only looks for functions defined in the
> > > > same file. Here are the functions that are passed to call_rcu where the
> > > > function is not defined in the same file:
> > > >
> > > > need definition for audit_free_rule_rcu
> > > > need definition for __i915_gem_free_object_rcu
> > > > need definition for io_eventfd_ops
> > > > need definition for ip_vs_dest_dst_rcu_free
> > > > need definition for __put_task_struct_rcu_cb
> > > > need definition for radix_tree_node_rcu_free
> > > >
> > > > They all do something more, although radix_tree_node_rcu_free doesn't do
> > > > much more (some memsets).
> > > >
> > > > julia
> > > >
> > > > diff -u -p /home/jll/linux/drivers/net/wireguard/allowedips.c /tmp/nothing/drivers/net/wireguard/allowedips.c
> > > > --- /home/jll/linux/drivers/net/wireguard/allowedips.c
> > > > +++ /tmp/nothing/drivers/net/wireguard/allowedips.c
> > > > @@ -50,7 +50,6 @@ static void push_rcu(struct allowedips_n
> > > >
> > > > static void node_free_rcu(struct rcu_head *rcu)
> > > > {
> > > > - kmem_cache_free(node_cache, container_of(rcu, struct allowedips_node, rcu));
> > > > }
> > > >
> > > > static void root_free_rcu(struct rcu_head *rcu)
> > > > diff -u -p /home/jll/linux/fs/ecryptfs/dentry.c /tmp/nothing/fs/ecryptfs/dentry.c
> > > > --- /home/jll/linux/fs/ecryptfs/dentry.c
> > > > +++ /tmp/nothing/fs/ecryptfs/dentry.c
> > > > @@ -53,8 +53,6 @@ struct kmem_cache *ecryptfs_dentry_info_
> > > >
> > > > static void ecryptfs_dentry_free_rcu(struct rcu_head *head)
> > > > {
> > > > - kmem_cache_free(ecryptfs_dentry_info_cache,
> > > > - container_of(head, struct ecryptfs_dentry_info, rcu));
> > > > }
> > > >
> > > > /**
> > > > diff -u -p /home/jll/linux/kernel/fork.c /tmp/nothing/kernel/fork.c
> > > > --- /home/jll/linux/kernel/fork.c
> > > > +++ /tmp/nothing/kernel/fork.c
> > > > @@ -378,7 +378,6 @@ static struct kmem_cache *thread_stack_c
> > > >
> > > > static void thread_stack_free_rcu(struct rcu_head *rh)
> > > > {
> > > > - kmem_cache_free(thread_stack_cache, rh);
> > > > }
> > > >
> > > > static void thread_stack_delayed_free(struct task_struct *tsk)
> > > > diff -u -p /home/jll/linux/kernel/workqueue.c /tmp/nothing/kernel/workqueue.c
> > > > --- /home/jll/linux/kernel/workqueue.c
> > > > +++ /tmp/nothing/kernel/workqueue.c
> > > > @@ -5024,8 +5024,6 @@ fail:
> > > >
> > > > static void rcu_free_pwq(struct rcu_head *rcu)
> > > > {
> > > > - kmem_cache_free(pwq_cache,
> > > > - container_of(rcu, struct pool_workqueue, rcu));
> > > > }
> > > >
> > > > /*
> > > > diff -u -p /home/jll/linux/net/ipv4/inetpeer.c /tmp/nothing/net/ipv4/inetpeer.c
> > > > --- /home/jll/linux/net/ipv4/inetpeer.c
> > > > +++ /tmp/nothing/net/ipv4/inetpeer.c
> > > > @@ -130,7 +130,6 @@ static struct inet_peer *lookup(const st
> > > >
> > > > static void inetpeer_free_rcu(struct rcu_head *head)
> > > > {
> > > > - kmem_cache_free(peer_cachep, container_of(head, struct inet_peer, rcu));
> > > > }
> > > >
> > > > /* perform garbage collect on all items stacked during a lookup */
> > > > diff -u -p /home/jll/linux/net/ipv6/xfrm6_tunnel.c /tmp/nothing/net/ipv6/xfrm6_tunnel.c
> > > > --- /home/jll/linux/net/ipv6/xfrm6_tunnel.c
> > > > +++ /tmp/nothing/net/ipv6/xfrm6_tunnel.c
> > > > @@ -180,8 +180,6 @@ EXPORT_SYMBOL(xfrm6_tunnel_alloc_spi);
> > > >
> > > > static void x6spi_destroy_rcu(struct rcu_head *head)
> > > > {
> > > > - kmem_cache_free(xfrm6_tunnel_spi_kmem,
> > > > - container_of(head, struct xfrm6_tunnel_spi, rcu_head));
> > > > }
> > > >
> > > > static void xfrm6_tunnel_free_spi(struct net *net, xfrm_address_t *saddr)
> > > > diff -u -p /home/jll/linux/security/security.c /tmp/nothing/security/security.c
> > > > --- /home/jll/linux/security/security.c
> > > > +++ /tmp/nothing/security/security.c
> > > > @@ -1599,7 +1599,6 @@ static void inode_free_by_rcu(struct rcu
> > > > /*
> > > > * The rcu head is at the start of the inode blob
> > > > */
> > > > - kmem_cache_free(lsm_inode_cache, head);
> > > > }
> > > >
> > > > /**
> > > >
> > > See below some extra functions which can be eliminated. How i found them:
> > >
> > > find ./ -name "*.c" -o -name "*.h" | xargs grep -rn call_rcu -B 10 | grep kmem_cache_free
> > >
> > > <snip>
> > > static void nfsd4_free_file_rcu(struct rcu_head *rcu);
> >
> > Thanks. I tried to find this kind of issue, but it turned up nothing.
> > There must be some mistake, and I will try again.
> >
> You are welcome. Also there are several places like below:
>
> <snip>
> static void blk_free_queue_rcu(struct rcu_head *rcu_head)
> {
> struct request_queue *q = container_of(rcu_head,
> struct request_queue, rcu_head);
>
> percpu_ref_exit(&q->q_usage_counter);
> kmem_cache_free(blk_requestq_cachep, q);
> }
>
> static void blk_free_queue(struct request_queue *q)
> {
> blk_free_queue_stats(q->stats);
> if (queue_is_mq(q))
> blk_mq_release(q);
>
> ida_free(&blk_queue_ida, q->id);
> call_rcu(&q->rcu_head, blk_free_queue_rcu);
> }
> <snip>
>
> and potentially it can be also replaced:
>
> <snip>
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 82c3ae22d76d..898d6990600d 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -257,15 +257,6 @@ void blk_clear_pm_only(struct request_queue *q)
> }
> EXPORT_SYMBOL_GPL(blk_clear_pm_only);
>
> -static void blk_free_queue_rcu(struct rcu_head *rcu_head)
> -{
> - struct request_queue *q = container_of(rcu_head,
> - struct request_queue, rcu_head);
> -
> - percpu_ref_exit(&q->q_usage_counter);
> - kmem_cache_free(blk_requestq_cachep, q);
> -}
> -
> static void blk_free_queue(struct request_queue *q)
> {
> blk_free_queue_stats(q->stats);
> @@ -273,7 +264,8 @@ static void blk_free_queue(struct request_queue *q)
> blk_mq_release(q);
>
> ida_free(&blk_queue_ida, q->id);
> - call_rcu(&q->rcu_head, blk_free_queue_rcu);
> + percpu_ref_exit(&q->q_usage_counter);
> + kfree_rcu(q, rcu_head);
> }
>
> /**
> <snip>
This should be functionally fine, but could inflict serious memory
contention on the last few readers. Which, for all I know, might
be just fine.
As you say:
> but a maintainer of "blk-core.c" should check it if a usage counter can
> be updated before a completion of GP.
>
> Probably we can skip as of now such cases, so i do not have a strong
> opinion here.
Thanx, Paul
