| Message ID | 20241002040616.25193-2-jlee@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | tracking the references of net_device in aoe | expand |
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... > > 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 > > The value of aoeif->nd_pcpu_refcnt will also be printed when 'rmmod aoe': > > [23412.255237][ T2857] aoe: enp1s0->refcnt: 32, aoeif->nd_refcnt: 0 > > Using kernel dynamic debug can print more detail log but it causes extra > overhead: > > echo -n 'file drivers/block/aoe/* +p' > /sys/kernel/debug/dynamic_debug/control > > [ 6961.938642] aoe: tx dev_put enp1s0->refcnt: 31, aoeif->nd_refcnt: 1 > [ 7023.368814] aoe: aoecmd_cfg_pkts dev_hold lo->refcnt: 30 > [ 7023.370530] aoe: aoecmd_cfg_pkts dev_hold enp1s0->refcnt: 32, aoeif->nd_refcnt: 2 > [ 7023.372977] aoe: tx dev_put lo->refcnt: 29 > [ 7023.375147] aoe: tx dev_put enp1s0->refcnt: 31, aoeif->nd_refcnt: 1 > > Normally, after one operation of aoe, the aoeif->nd_refcnt should be > shown as '1' which means that calls of dev_hold(nd)/dev_put(nd) are > balanced. The final '1' reference of net_device will be removed when > rmmod aoe. > > Signed-off-by: Chun-Yi Lee <jlee@suse.com> > --- > drivers/block/aoe/aoe.h | 84 ++++++++++++++++++++++++++++++++++++++ > drivers/block/aoe/aoeblk.c | 5 +++ > drivers/block/aoe/aoedev.c | 20 +++++++++ > 3 files changed, 109 insertions(+) > > diff --git a/drivers/block/aoe/aoe.h b/drivers/block/aoe/aoe.h > index 749ae1246f4c..a6d954562794 100644 > --- a/drivers/block/aoe/aoe.h > +++ b/drivers/block/aoe/aoe.h > @@ -1,5 +1,6 @@ > /* Copyright (c) 2013 Coraid, Inc. See COPYING for GPL terms. */ > #include <linux/blk-mq.h> > +#include <linux/netdevice.h> > > #define VERSION "85" > #define AOE_MAJOR 152 > @@ -133,6 +134,9 @@ struct aoeif { > struct net_device *nd; > ulong lost; > int bcnt; > +#ifdef DEBUG > + int __percpu *nd_pcpu_refcnt; > +#endif > }; > > struct aoetgt { > @@ -238,6 +242,7 @@ void aoedev_downdev(struct aoedev *d); > int aoedev_flush(const char __user *str, size_t size); > void aoe_failbuf(struct aoedev *, struct buf *); > void aoedev_put(struct aoedev *); > +struct aoeif *get_aoeif(struct net_device *nd); > > int aoenet_init(void); > void aoenet_exit(void); > @@ -246,3 +251,82 @@ int is_aoe_netif(struct net_device *ifp); > int set_aoe_iflist(const char __user *str, size_t size); > > extern struct workqueue_struct *aoe_wq; > + > +#ifdef DEBUG > +static inline int aoeif_nd_refcnt_read(const struct aoeif *ifp) > +{ > + int i, refcnt = 0; > + > + for_each_possible_cpu(i) > + refcnt += *per_cpu_ptr(ifp->nd_pcpu_refcnt, i); > + return refcnt; > +} > + > +static inline void aoeif_nd_refcnt_free(struct aoeif *ifp) > +{ > + int i; > + > + if(!ifp) > + return; > + if (ifp->nd) > + pr_info("aoe: %s->refcnt: %d, aoeif->nd_refcnt: %d\n", > + ifp->nd->name, netdev_refcnt_read(ifp->nd), > + aoeif_nd_refcnt_read(ifp)); > + else > + pr_info("aoe: aoeif->nd_refcnt: %d\n", aoeif_nd_refcnt_read(ifp)); > + > + for_each_possible_cpu(i) > + *per_cpu_ptr(ifp->nd_pcpu_refcnt, i) = 0; > + free_percpu(ifp->nd_pcpu_refcnt); > + ifp->nd_pcpu_refcnt = NULL; > +} > + > +/* ifi aoeif input, nb be set to aoeif or in the future will be set */ > +static inline void __nd_dev_hold(const char *str, struct net_device *nd, struct aoeif *ifi) > +{ > + struct aoeif *ifp; > + > + if (!nd) > + return; > + dev_hold(nd); > + 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. > +} > +#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. > +} > +#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... > +#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 "*" ? > +{ > + struct aoedev *d; > + struct aoetgt *t, **tt, **te; > + struct aoeif *ifp; > + > + for (d=devlist; d; d=d->next) { > + tt = d->targets; > + te = tt + d->ntargets; > + for (; tt < te && (t = *tt); tt++) { > + for (ifp = t->ifs; ifp < &t->ifs[NAOEIFS]; ++ifp) { > + if (ifp->nd && (ifp->nd == nd)) > + return ifp; > + } > + } > + } > + return NULL; > +}
On 10/1/24 10: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. There's no parallel universe in which using a percpu reference over just a refcount_t for something like aoe is warranted.
diff --git a/drivers/block/aoe/aoe.h b/drivers/block/aoe/aoe.h index 749ae1246f4c..a6d954562794 100644 --- a/drivers/block/aoe/aoe.h +++ b/drivers/block/aoe/aoe.h @@ -1,5 +1,6 @@ /* Copyright (c) 2013 Coraid, Inc. See COPYING for GPL terms. */ #include <linux/blk-mq.h> +#include <linux/netdevice.h> #define VERSION "85" #define AOE_MAJOR 152 @@ -133,6 +134,9 @@ struct aoeif { struct net_device *nd; ulong lost; int bcnt; +#ifdef DEBUG + int __percpu *nd_pcpu_refcnt; +#endif }; struct aoetgt { @@ -238,6 +242,7 @@ void aoedev_downdev(struct aoedev *d); int aoedev_flush(const char __user *str, size_t size); void aoe_failbuf(struct aoedev *, struct buf *); void aoedev_put(struct aoedev *); +struct aoeif *get_aoeif(struct net_device *nd); int aoenet_init(void); void aoenet_exit(void); @@ -246,3 +251,82 @@ int is_aoe_netif(struct net_device *ifp); int set_aoe_iflist(const char __user *str, size_t size); extern struct workqueue_struct *aoe_wq; + +#ifdef DEBUG +static inline int aoeif_nd_refcnt_read(const struct aoeif *ifp) +{ + int i, refcnt = 0; + + for_each_possible_cpu(i) + refcnt += *per_cpu_ptr(ifp->nd_pcpu_refcnt, i); + return refcnt; +} + +static inline void aoeif_nd_refcnt_free(struct aoeif *ifp) +{ + int i; + + if(!ifp) + return; + if (ifp->nd) + pr_info("aoe: %s->refcnt: %d, aoeif->nd_refcnt: %d\n", + ifp->nd->name, netdev_refcnt_read(ifp->nd), + aoeif_nd_refcnt_read(ifp)); + else + pr_info("aoe: aoeif->nd_refcnt: %d\n", aoeif_nd_refcnt_read(ifp)); + + for_each_possible_cpu(i) + *per_cpu_ptr(ifp->nd_pcpu_refcnt, i) = 0; + free_percpu(ifp->nd_pcpu_refcnt); + ifp->nd_pcpu_refcnt = NULL; +} + +/* ifi aoeif input, nb be set to aoeif or in the future will be set */ +static inline void __nd_dev_hold(const char *str, struct net_device *nd, struct aoeif *ifi) +{ + struct aoeif *ifp; + + if (!nd) + return; + dev_hold(nd); + 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)); +} +#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)); +} +#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)); +#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) +{ + struct aoedev *d; + struct aoetgt *t, **tt, **te; + struct aoeif *ifp; + + for (d=devlist; d; d=d->next) { + tt = d->targets; + te = tt + d->ntargets; + for (; tt < te && (t = *tt); tt++) { + for (ifp = t->ifs; ifp < &t->ifs[NAOEIFS]; ++ifp) { + if (ifp->nd && (ifp->nd == nd)) + return ifp; + } + } + } + return NULL; +}
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: 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 The value of aoeif->nd_pcpu_refcnt will also be printed when 'rmmod aoe': [23412.255237][ T2857] aoe: enp1s0->refcnt: 32, aoeif->nd_refcnt: 0 Using kernel dynamic debug can print more detail log but it causes extra overhead: echo -n 'file drivers/block/aoe/* +p' > /sys/kernel/debug/dynamic_debug/control [ 6961.938642] aoe: tx dev_put enp1s0->refcnt: 31, aoeif->nd_refcnt: 1 [ 7023.368814] aoe: aoecmd_cfg_pkts dev_hold lo->refcnt: 30 [ 7023.370530] aoe: aoecmd_cfg_pkts dev_hold enp1s0->refcnt: 32, aoeif->nd_refcnt: 2 [ 7023.372977] aoe: tx dev_put lo->refcnt: 29 [ 7023.375147] aoe: tx dev_put enp1s0->refcnt: 31, aoeif->nd_refcnt: 1 Normally, after one operation of aoe, the aoeif->nd_refcnt should be shown as '1' which means that calls of dev_hold(nd)/dev_put(nd) are balanced. The final '1' reference of net_device will be removed when rmmod aoe. Signed-off-by: Chun-Yi Lee <jlee@suse.com> --- drivers/block/aoe/aoe.h | 84 ++++++++++++++++++++++++++++++++++++++ drivers/block/aoe/aoeblk.c | 5 +++ drivers/block/aoe/aoedev.c | 20 +++++++++ 3 files changed, 109 insertions(+)