On Tue, Jun 11, 2024 at 10:25:52AM -0700, Paul E. McKenney wrote:
> 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.
>
I can send the patch :)
--
Uladzislau Rezki
