On Tue, Jun 11, 2024 at 06:40:49PM +0200, Uladzislau Rezki wrote:
> > >
> > > These look ok to me. In the last two cases, the callback function is also
> > > stored in a data structure, eg:
> > >
> > > static struct mfc6_cache *ip6mr_cache_alloc(void)
> > > {
> > > struct mfc6_cache *c = kmem_cache_zalloc(mrt_cachep, GFP_KERNEL);
> > > if (!c)
> > > return NULL;
> > > c->_c.mfc_un.res.last_assert = jiffies - MFC_ASSERT_THRESH - 1;
> > > c->_c.mfc_un.res.minvif = MAXMIFS;
> > > c->_c.free = ip6mr_cache_free_rcu;
> > > refcount_set(&c->_c.mfc_un.res.refcount, 1);
> > > return c;
> > > }
> > >
> > > Should that be left as it is?
> >
> > Given that ->_c.free isn't used in the RCU callback, I am guessing that
> > this is intended for debugging purposes, so that you can see from a crash
> > dump how this will be freed. But I could be completely off-base here.
> >
> > One approach would be to remove the ->_c.free field and call attention
> > to this in the patches' commit logs.
> >
> > Another would be to instead put the address of the allocation function
> > in ->_c.free, and again call attention to this in the commit logs.
> >
> > Is there a better approach than these three? ;-)
> >
> IMO, "_c.free" should be removed:
Why not send the patch and see what the maintainers say? If they object,
you can always fall back to one of the other two methods, depending on
the nature of their objection.
Thanx, Paul
> diff --git a/include/linux/mroute_base.h b/include/linux/mroute_base.h
> index 9dd4bf157255..45f220f761df 100644
> --- a/include/linux/mroute_base.h
> +++ b/include/linux/mroute_base.h
> @@ -130,7 +130,6 @@ enum {
> * @refcount: reference count for this entry
> * @list: global entry list
> * @rcu: used for entry destruction
> - * @free: Operation used for freeing an entry under RCU
> */
> struct mr_mfc {
> struct rhlist_head mnode;
> @@ -156,13 +155,12 @@ struct mr_mfc {
> } mfc_un;
> struct list_head list;
> struct rcu_head rcu;
> - void (*free)(struct rcu_head *head);
> };
>
> static inline void mr_cache_put(struct mr_mfc *c)
> {
> if (refcount_dec_and_test(&c->mfc_un.res.refcount))
> - call_rcu(&c->rcu, c->free);
> + kfree_rcu(c, rcu);
> }
>
> static inline void mr_cache_hold(struct mr_mfc *c)
> diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
> index 6c750bd13dd8..5d2e339f09cc 100644
> --- a/net/ipv4/ipmr.c
> +++ b/net/ipv4/ipmr.c
> @@ -702,16 +702,9 @@ static int vif_delete(struct mr_table *mrt, int vifi, int notify,
> return 0;
> }
>
> -static void ipmr_cache_free_rcu(struct rcu_head *head)
> -{
> - struct mr_mfc *c = container_of(head, struct mr_mfc, rcu);
> -
> - kmem_cache_free(mrt_cachep, (struct mfc_cache *)c);
> -}
> -
> static void ipmr_cache_free(struct mfc_cache *c)
> {
> - call_rcu(&c->_c.rcu, ipmr_cache_free_rcu);
> + kfree_rcu(c, _c.rcu);
> }
>
> /* Destroy an unresolved cache entry, killing queued skbs
> @@ -959,7 +952,6 @@ static struct mfc_cache *ipmr_cache_alloc(void)
> if (c) {
> c->_c.mfc_un.res.last_assert = jiffies - MFC_ASSERT_THRESH - 1;
> c->_c.mfc_un.res.minvif = MAXVIFS;
> - c->_c.free = ipmr_cache_free_rcu;
> refcount_set(&c->_c.mfc_un.res.refcount, 1);
> }
> return c;
> diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
> index dd342e6ecf3f..1634fa794ea2 100644
> --- a/net/ipv6/ip6mr.c
> +++ b/net/ipv6/ip6mr.c
> @@ -753,16 +753,9 @@ static int mif6_delete(struct mr_table *mrt, int vifi, int notify,
> return 0;
> }
>
> -static inline void ip6mr_cache_free_rcu(struct rcu_head *head)
> -{
> - struct mr_mfc *c = container_of(head, struct mr_mfc, rcu);
> -
> - kmem_cache_free(mrt_cachep, (struct mfc6_cache *)c);
> -}
> -
> static inline void ip6mr_cache_free(struct mfc6_cache *c)
> {
> - call_rcu(&c->_c.rcu, ip6mr_cache_free_rcu);
> + kfree_rcu(c, _c.rcu);
> }
>
> /* Destroy an unresolved cache entry, killing queued skbs
> @@ -985,7 +978,6 @@ static struct mfc6_cache *ip6mr_cache_alloc(void)
> return NULL;
> c->_c.mfc_un.res.last_assert = jiffies - MFC_ASSERT_THRESH - 1;
> c->_c.mfc_un.res.minvif = MAXMIFS;
> - c->_c.free = ip6mr_cache_free_rcu;
> refcount_set(&c->_c.mfc_un.res.refcount, 1);
> return c;
> }
>
> --
> Uladzislau Rezki
