Re: [RFC PATCH 1/2] aoe: add reference count in aoeif for tracking the using of net_device

[joeyli]

Hi Damien, 

Thanks for your review, first!

On Wed, Oct 02, 2024 at 02:35:33PM +0900, Damien Le Moal wrote:
> On 10/2/24 1:06 PM, Chun-Yi Lee wrote:
> > This is a patch for debugging. For tracking the reference count of using
> > net_device in aoeif, this patch adds a nd_pcpu_refcnt field in aoeif
> > structure. Two wrappers, nd_dev_hold() and nd_dev_put() are used to
> > call dev_hold(nd)/dev_put(nd) and maintain ifp->nd_pcpu_refcnt at the
> > same time.
> > 
> > Defined DEBUG to the top of the aoe.h can enable the tracking function.
> > The nd_pcpu_refcnt will be printed to debugfs:
> 
> Why not make that a config option ? That would avoid having to edit the code to
> enable debugging...
>

This debug patch is only for tracking the reference count of net_device but
no other debugging feature. I don't want to add one more config option for
this small feature. On the other hand, the tracking requirment of reference
count is more for developer but not for administrator. That's why I did not
add a new option.
 
> > 
> > rttavg: 249029 rttdev: 1781043
> > nskbpool: 0
> > kicked: 0
> > maxbcnt: 1024
> > ref: 0
> > falloc: 36
> > ffree: 0000000013c0033f
> > 52540054c48e:0:16:16
> >         ssthresh:8
> >         taint:0
> >         r:1270
> >         w:8
> >         enp1s0:1	<-- the aoeif->nd_pcpu_refcnt is behind nd->name
> > 
[...snip]
> > +	ifp = ifi? ifi:get_aoeif(nd);
> > +	if (ifp) {
> > +		this_cpu_inc(*ifp->nd_pcpu_refcnt);
> > +		pr_debug("aoe: %s dev_hold %s->refcnt: %d, aoeif->nd_refcnt: %d\n",
> > +			 str, nd->name, netdev_refcnt_read(nd),
> > +			 aoeif_nd_refcnt_read(ifp));
> > +	} else
> > +		pr_debug("aoe: %s dev_hold %s->refcnt: %d\n",
> > +			 str, nd->name, netdev_refcnt_read(nd));
> 
> Missing curly brackets around the else statement.
>

Thanks for your reminder! I will add it in next version.
 
> > +}
> > +#define nd_dev_hold(msg, ifi) __nd_dev_hold(__FUNCTION__, (msg), (ifi))
> > +
> > +static inline void __nd_dev_put(const char *str, struct net_device *nd, struct aoeif *ifi)
> > +{
> > +	struct aoeif *ifp;
> > +
> > +	if (!nd)
> > +		return;
> > +	dev_put(nd);
> > +	ifp = ifi? ifi:get_aoeif(nd);
> > +	if (ifp) {
> > +		this_cpu_dec(*ifp->nd_pcpu_refcnt);
> > +		pr_debug("aoe: %s dev_put %s->refcnt: %d, aoeif->nd_refcnt: %d\n",
> > +			 str, nd->name, netdev_refcnt_read(nd),
> > +			 aoeif_nd_refcnt_read(ifp));
> > +	} else
> > +		pr_debug("aoe: %s dev_put %s->refcnt: %d\n",
> > +			 str, nd->name, netdev_refcnt_read(nd));
> 
> Same here.
>

Thanks! I will add it.
 
> > +}
> > +#define nd_dev_put(msg, ifi) __nd_dev_put(__FUNCTION__, (msg), (ifi))
> > +#else
> > +static inline void nd_dev_put(struct net_device *nd, struct aoeif *ifi)
> > +{
> > +	dev_hold(nd);
> > +}
> > +static inline void nd_dev_hold(struct net_device *nd, struct aoeif *ifi)
> > +{
> > +       dev_put(nd);
> > +}
> > +static inline void aoeif_nd_refcnt_free(const struct aoeif *ifp) {}
> > +#endif // DEBUG
> > diff --git a/drivers/block/aoe/aoeblk.c b/drivers/block/aoe/aoeblk.c
> > index 2028795ec61c..19d62ccca1e9 100644
> > --- a/drivers/block/aoe/aoeblk.c
> > +++ b/drivers/block/aoe/aoeblk.c
> > @@ -142,7 +142,12 @@ static int aoe_debugfs_show(struct seq_file *s, void *ignored)
> >  		ifp = (*t)->ifs;
> >  		ife = ifp + ARRAY_SIZE((*t)->ifs);
> >  		for (; ifp->nd && ifp < ife; ifp++) {
> > +#ifdef DEBUG
> > +			seq_printf(s, "%c%s:%d", c, ifp->nd->name,
> > +					aoeif_nd_refcnt_read(ifp));
> 
> I personnally find it better looking to align the arguments instead of adding a
> random tab...
>

Thanks! I will modify the second line to align with the first argument.
 
> > +#else
> >  			seq_printf(s, "%c%s", c, ifp->nd->name);
> > +#endif
> >  			c = ',';
> >  		}
> >  		seq_puts(s, "\n");
> > diff --git a/drivers/block/aoe/aoedev.c b/drivers/block/aoe/aoedev.c
> > index 3523dd82d7a0..9781488b286b 100644
> > --- a/drivers/block/aoe/aoedev.c
> > +++ b/drivers/block/aoe/aoedev.c
> > @@ -529,3 +529,23 @@ aoedev_init(void)
> >  {
> >  	return 0;
> >  }
> > +
> > +struct aoeif *
> > +get_aoeif(struct net_device *nd)
> 
> Why the line split after "*" ?
> 

I followed the same coding style in aoedev.c:

/* find it or allocate it */
struct aoedev *
aoedev_by_aoeaddr(ulong maj, int min, int do_alloc)
{
        struct aoedev *d;
...

If kernel coding style does not specify this. I prefer follow the original
style in the same driver.

Let me know if I missed anything, please. Then I will change the style. 


Thanks a lot!
Joey Lee