* [PATCH] add stats info for FC rports
@ 2010-12-08 13:27 Hillf Danton
2010-12-08 22:06 ` [Open-FCoE] " Joe Eykholt
0 siblings, 1 reply; 6+ messages in thread
From: Hillf Danton @ 2010-12-08 13:27 UTC (permalink / raw)
To: devel; +Cc: linux-scsi
Stats info is added, in the define of local port, to monitor the usage
of remote ports.
Then it is simple to answer how many rport data leaked in the life
cycle of lport.
This work is motivated to capture mm leak of rports of
FC_FID_DIR_SERV, and thanks for dropping a reply to this message if
captured.
Signed-off-by: Hillf Danton <dhillf@gmail.com>
---
--- a/include/scsi/libfc.h 2010-11-01 19:54:12.000000000 +0800
+++ b/include/scsi/libfc.h 2010-12-08 21:05:46.000000000 +0800
@@ -803,6 +803,9 @@ struct fc_lport {
void *scsi_priv;
struct fc_disc disc;
+ /* stats info of the usage of rports */
+ unsigned long nr_rdata;
+
/* Virtual port information */
struct list_head vports;
struct fc_vport *vport;
--- a/drivers/scsi/libfc/fc_rport.c 2010-11-01 19:54:12.000000000 +0800
+++ b/drivers/scsi/libfc/fc_rport.c 2010-12-08 21:16:40.000000000 +0800
@@ -128,7 +128,7 @@ static struct fc_rport_priv *fc_rport_cr
rdata = kzalloc(sizeof(*rdata) + lport->rport_priv_size, GFP_KERNEL);
if (!rdata)
return NULL;
-
+ lport->nr_rdata++;
rdata->ids.node_name = -1;
rdata->ids.port_name = -1;
rdata->ids.port_id = port_id;
@@ -159,6 +159,7 @@ static void fc_rport_free_rcu(struct rcu
struct fc_rport_priv *rdata;
rdata = container_of(rcu, struct fc_rport_priv, rcu);
+ rdata->local_port->nr_rdata--;
kfree(rdata);
}
@@ -1880,7 +1881,7 @@ int fc_rport_init(struct fc_lport *lport
if (!lport->tt.rport_destroy)
lport->tt.rport_destroy = fc_rport_destroy;
-
+ lport->nr_rdata = 0;
return 0;
}
EXPORT_SYMBOL(fc_rport_init);
--- a/drivers/scsi/libfc/fc_fcp.c 2010-11-01 19:54:12.000000000 +0800
+++ b/drivers/scsi/libfc/fc_fcp.c 2010-12-08 21:19:20.000000000 +0800
@@ -2184,6 +2184,11 @@ void fc_fcp_destroy(struct fc_lport *lpo
{
struct fc_fcp_internal *si = fc_get_scsi_internal(lport);
+ if (lport->nr_rdata != 0)
+ printk(KERN_WARNING "libfc: %lu Leaked rports when "
+ "destroying local port (%6.6x)\n",
+ lport->nr_rdata, lport->port_id);
+
if (!list_empty(&si->scsi_pkt_queue))
printk(KERN_ERR "libfc: Leaked SCSI packets when destroying "
"port (%6.6x)\n", lport->port_id);
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Open-FCoE] [PATCH] add stats info for FC rports 2010-12-08 13:27 [PATCH] add stats info for FC rports Hillf Danton @ 2010-12-08 22:06 ` Joe Eykholt 2010-12-09 14:18 ` Hillf Danton 0 siblings, 1 reply; 6+ messages in thread From: Joe Eykholt @ 2010-12-08 22:06 UTC (permalink / raw) To: Hillf Danton; +Cc: devel, linux-scsi On 12/8/10 5:27 AM, Hillf Danton wrote: > Stats info is added, in the define of local port, to monitor the usage > of remote ports. > > Then it is simple to answer how many rport data leaked in the life > cycle of lport. > > This work is motivated to capture mm leak of rports of > FC_FID_DIR_SERV, and thanks for dropping a reply to this message if > captured. This sounds like code that could be done locally and then tossed out once the leak is found. Also, there is already a list of remote ports, I think, so the count is redundant info. > Signed-off-by: Hillf Danton <dhillf@gmail.com> > --- > > --- a/include/scsi/libfc.h 2010-11-01 19:54:12.000000000 +0800 > +++ b/include/scsi/libfc.h 2010-12-08 21:05:46.000000000 +0800 > @@ -803,6 +803,9 @@ struct fc_lport { > void *scsi_priv; > struct fc_disc disc; > > + /* stats info of the usage of rports */ > + unsigned long nr_rdata; > + > /* Virtual port information */ > struct list_head vports; > struct fc_vport *vport; > --- a/drivers/scsi/libfc/fc_rport.c 2010-11-01 19:54:12.000000000 +0800 > +++ b/drivers/scsi/libfc/fc_rport.c 2010-12-08 21:16:40.000000000 +0800 > @@ -128,7 +128,7 @@ static struct fc_rport_priv *fc_rport_cr > rdata = kzalloc(sizeof(*rdata) + lport->rport_priv_size, GFP_KERNEL); > if (!rdata) > return NULL; > - > + lport->nr_rdata++; > rdata->ids.node_name = -1; > rdata->ids.port_name = -1; > rdata->ids.port_id = port_id; > @@ -159,6 +159,7 @@ static void fc_rport_free_rcu(struct rcu > struct fc_rport_priv *rdata; > > rdata = container_of(rcu, struct fc_rport_priv, rcu); > + rdata->local_port->nr_rdata--; What about locking? I don't think you should do this in the rcu callback because the local port may no longer exist. Instead, I would do it in fc_rport_destroy. > kfree(rdata); > } > > @@ -1880,7 +1881,7 @@ int fc_rport_init(struct fc_lport *lport > > if (!lport->tt.rport_destroy) > lport->tt.rport_destroy = fc_rport_destroy; > - > + lport->nr_rdata = 0; Everything in the lport starts as zero, so no need to insert this explicitly. > return 0; > } > EXPORT_SYMBOL(fc_rport_init); > --- a/drivers/scsi/libfc/fc_fcp.c 2010-11-01 19:54:12.000000000 +0800 > +++ b/drivers/scsi/libfc/fc_fcp.c 2010-12-08 21:19:20.000000000 +0800 > @@ -2184,6 +2184,11 @@ void fc_fcp_destroy(struct fc_lport *lpo > { > struct fc_fcp_internal *si = fc_get_scsi_internal(lport); > > + if (lport->nr_rdata != 0) > + printk(KERN_WARNING "libfc: %lu Leaked rports when " > + "destroying local port (%6.6x)\n", > + lport->nr_rdata, lport->port_id); > + The equivalent of this check should go into fc_rport.c, not here. > if (!list_empty(&si->scsi_pkt_queue)) > printk(KERN_ERR "libfc: Leaked SCSI packets when destroying " > "port (%6.6x)\n", lport->port_id); > _______________________________________________ > devel mailing list > devel@open-fcoe.org > http://www.open-fcoe.org/mailman/listinfo/devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Open-FCoE] [PATCH] add stats info for FC rports 2010-12-08 22:06 ` [Open-FCoE] " Joe Eykholt @ 2010-12-09 14:18 ` Hillf Danton 2010-12-09 19:17 ` Joe Eykholt 0 siblings, 1 reply; 6+ messages in thread From: Hillf Danton @ 2010-12-09 14:18 UTC (permalink / raw) To: Joe Eykholt; +Cc: devel, linux-scsi On Thu, Dec 9, 2010 at 6:06 AM, Joe Eykholt <jeykholt@cisco.com> wrote: > > > On 12/8/10 5:27 AM, Hillf Danton wrote: >> Stats info is added, in the define of local port, to monitor the usage >> of remote ports. >> >> Then it is simple to answer how many rport data leaked in the life >> cycle of lport. >> >> This work is motivated to capture mm leak of rports of >> FC_FID_DIR_SERV, and thanks for dropping a reply to this message if >> captured. > > This sounds like code that could be done locally and then > tossed out once the leak is found. Also, there is already a list > of remote ports, I think, so the count is redundant info. It is fine to read your comments, Joe, in winter evening. Though the list is defined, rport of FC_FID_DIR_SERV is not added onto it, at least when created, which is hard to trace then. And FC_FID_DIR_SERV is the major concern of this work. If captured, fix should have been delivered. > >> Signed-off-by: Hillf Danton <dhillf@gmail.com> >> --- >> >> --- a/include/scsi/libfc.h 2010-11-01 19:54:12.000000000 +0800 >> +++ b/include/scsi/libfc.h 2010-12-08 21:05:46.000000000 +0800 >> @@ -803,6 +803,9 @@ struct fc_lport { >> void *scsi_priv; >> struct fc_disc disc; >> >> + /* stats info of the usage of rports */ >> + unsigned long nr_rdata; >> + >> /* Virtual port information */ >> struct list_head vports; >> struct fc_vport *vport; >> --- a/drivers/scsi/libfc/fc_rport.c 2010-11-01 19:54:12.000000000 +0800 >> +++ b/drivers/scsi/libfc/fc_rport.c 2010-12-08 21:16:40.000000000 +0800 >> @@ -128,7 +128,7 @@ static struct fc_rport_priv *fc_rport_cr >> rdata = kzalloc(sizeof(*rdata) + lport->rport_priv_size, GFP_KERNEL); >> if (!rdata) >> return NULL; >> - >> + lport->nr_rdata++; >> rdata->ids.node_name = -1; >> rdata->ids.port_name = -1; >> rdata->ids.port_id = port_id; >> @@ -159,6 +159,7 @@ static void fc_rport_free_rcu(struct rcu >> struct fc_rport_priv *rdata; >> >> rdata = container_of(rcu, struct fc_rport_priv, rcu); >> + rdata->local_port->nr_rdata--; > > What about locking? I don't think you should do this in the rcu Locking here looks unnecessary, since lport could not goto Mars. If could, lot of orphans have to stay on Earth with no more care. > callback because the local port may no longer exist. Instead, I > would do it in fc_rport_destroy. > >> kfree(rdata); >> } >> >> @@ -1880,7 +1881,7 @@ int fc_rport_init(struct fc_lport *lport >> >> if (!lport->tt.rport_destroy) >> lport->tt.rport_destroy = fc_rport_destroy; >> - >> + lport->nr_rdata = 0; > > Everything in the lport starts as zero, so no need to insert this explicitly. When initing lport, clarity could be redundant. > >> return 0; >> } >> EXPORT_SYMBOL(fc_rport_init); >> --- a/drivers/scsi/libfc/fc_fcp.c 2010-11-01 19:54:12.000000000 +0800 >> +++ b/drivers/scsi/libfc/fc_fcp.c 2010-12-08 21:19:20.000000000 +0800 >> @@ -2184,6 +2184,11 @@ void fc_fcp_destroy(struct fc_lport *lpo >> { >> struct fc_fcp_internal *si = fc_get_scsi_internal(lport); >> >> + if (lport->nr_rdata != 0) >> + printk(KERN_WARNING "libfc: %lu Leaked rports when " >> + "destroying local port (%6.6x)\n", >> + lport->nr_rdata, lport->port_id); >> + > > The equivalent of this check should go into fc_rport.c, not here. When destroying lport, it is the right place to report leakage. Cheers Hillf > >> if (!list_empty(&si->scsi_pkt_queue)) >> printk(KERN_ERR "libfc: Leaked SCSI packets when destroying " >> "port (%6.6x)\n", lport->port_id); >> _______________________________________________ >> devel mailing list >> devel@open-fcoe.org >> http://www.open-fcoe.org/mailman/listinfo/devel > -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Open-FCoE] [PATCH] add stats info for FC rports 2010-12-09 14:18 ` Hillf Danton @ 2010-12-09 19:17 ` Joe Eykholt 2010-12-10 13:35 ` Hillf Danton 0 siblings, 1 reply; 6+ messages in thread From: Joe Eykholt @ 2010-12-09 19:17 UTC (permalink / raw) To: Hillf Danton; +Cc: devel, linux-scsi On 12/9/10 6:18 AM, Hillf Danton wrote: > On Thu, Dec 9, 2010 at 6:06 AM, Joe Eykholt <jeykholt@cisco.com> wrote: >> >> >> On 12/8/10 5:27 AM, Hillf Danton wrote: >>> Stats info is added, in the define of local port, to monitor the usage >>> of remote ports. >>> >>> Then it is simple to answer how many rport data leaked in the life >>> cycle of lport. >>> >>> This work is motivated to capture mm leak of rports of >>> FC_FID_DIR_SERV, and thanks for dropping a reply to this message if >>> captured. >> >> This sounds like code that could be done locally and then >> tossed out once the leak is found. Also, there is already a list >> of remote ports, I think, so the count is redundant info. > > It is fine to read your comments, Joe, in winter evening. > > Though the list is defined, rport of FC_FID_DIR_SERV is not added > onto it, at least when created, which is hard to trace then. > > And FC_FID_DIR_SERV is the major concern of this work. > If captured, fix should have been delivered. > >> >>> Signed-off-by: Hillf Danton <dhillf@gmail.com> >>> --- >>> >>> --- a/include/scsi/libfc.h 2010-11-01 19:54:12.000000000 +0800 >>> +++ b/include/scsi/libfc.h 2010-12-08 21:05:46.000000000 +0800 >>> @@ -803,6 +803,9 @@ struct fc_lport { >>> void *scsi_priv; >>> struct fc_disc disc; >>> >>> + /* stats info of the usage of rports */ >>> + unsigned long nr_rdata; >>> + >>> /* Virtual port information */ >>> struct list_head vports; >>> struct fc_vport *vport; >>> --- a/drivers/scsi/libfc/fc_rport.c 2010-11-01 19:54:12.000000000 +0800 >>> +++ b/drivers/scsi/libfc/fc_rport.c 2010-12-08 21:16:40.000000000 +0800 >>> @@ -128,7 +128,7 @@ static struct fc_rport_priv *fc_rport_cr >>> rdata = kzalloc(sizeof(*rdata) + lport->rport_priv_size, GFP_KERNEL); >>> if (!rdata) >>> return NULL; >>> - >>> + lport->nr_rdata++; >>> rdata->ids.node_name = -1; >>> rdata->ids.port_name = -1; >>> rdata->ids.port_id = port_id; >>> @@ -159,6 +159,7 @@ static void fc_rport_free_rcu(struct rcu >>> struct fc_rport_priv *rdata; >>> >>> rdata = container_of(rcu, struct fc_rport_priv, rcu); >>> + rdata->local_port->nr_rdata--; >> >> What about locking? I don't think you should do this in the rcu > > Locking here looks unnecessary, since lport could not goto Mars. > If could, lot of orphans have to stay on Earth with no more care. The lport could go away before the rport is freed by rcu. Also, two rports could be freed in rcu callbacks at the same time, so two decrements of nr_rdata could be concurrent and you would get nondeterministic results since the decrement isn't atomic. >> callback because the local port may no longer exist. Instead, I >> would do it in fc_rport_destroy. >> >>> kfree(rdata); >>> } >>> >>> @@ -1880,7 +1881,7 @@ int fc_rport_init(struct fc_lport *lport >>> >>> if (!lport->tt.rport_destroy) >>> lport->tt.rport_destroy = fc_rport_destroy; >>> - >>> + lport->nr_rdata = 0; >> >> Everything in the lport starts as zero, so no need to insert this explicitly. > > When initing lport, clarity could be redundant. So, you agree or not? >>> return 0; >>> } >>> EXPORT_SYMBOL(fc_rport_init); >>> --- a/drivers/scsi/libfc/fc_fcp.c 2010-11-01 19:54:12.000000000 +0800 >>> +++ b/drivers/scsi/libfc/fc_fcp.c 2010-12-08 21:19:20.000000000 +0800 >>> @@ -2184,6 +2184,11 @@ void fc_fcp_destroy(struct fc_lport *lpo >>> { >>> struct fc_fcp_internal *si = fc_get_scsi_internal(lport); >>> >>> + if (lport->nr_rdata != 0) >>> + printk(KERN_WARNING "libfc: %lu Leaked rports when " >>> + "destroying local port (%6.6x)\n", >>> + lport->nr_rdata, lport->port_id); >>> + >> >> The equivalent of this check should go into fc_rport.c, not here. > > When destroying lport, it is the right place to report leakage. Yes, but there's a call from lport destroy into rport, and that's the place to report rport leakage, not in the FCP module. For one thing, not all LLDs use that fcp module. > Cheers > Hillf > >> >>> if (!list_empty(&si->scsi_pkt_queue)) >>> printk(KERN_ERR "libfc: Leaked SCSI packets when destroying " >>> "port (%6.6x)\n", lport->port_id); >>> _______________________________________________ >>> devel mailing list >>> devel@open-fcoe.org >>> http://www.open-fcoe.org/mailman/listinfo/devel >> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Open-FCoE] [PATCH] add stats info for FC rports 2010-12-09 19:17 ` Joe Eykholt @ 2010-12-10 13:35 ` Hillf Danton 2010-12-10 15:13 ` Hillf Danton 0 siblings, 1 reply; 6+ messages in thread From: Hillf Danton @ 2010-12-10 13:35 UTC (permalink / raw) To: Joe Eykholt; +Cc: devel, linux-scsi On Fri, Dec 10, 2010 at 3:17 AM, Joe Eykholt <jeykholt@cisco.com> wrote: > > > On 12/9/10 6:18 AM, Hillf Danton wrote: >> On Thu, Dec 9, 2010 at 6:06 AM, Joe Eykholt <jeykholt@cisco.com> wrote: >> >> Locking here looks unnecessary, since lport could not goto Mars. >> If could, lot of orphans have to stay on Earth with no more care. > > The lport could go away before the rport is freed by rcu. Yes, rcu was neglected by my fault. > Also, two rports could be freed in rcu callbacks at the same time, > so two decrements of nr_rdata could be concurrent and you would > get nondeterministic results since the decrement isn't atomic. > > >> >> When initing lport, clarity could be redundant. > > So, you agree or not? Agree with no doubt. > >> When destroying lport, it is the right place to report leakage. > > Yes, but there's a call from lport destroy into rport, and that's the > place to report rport leakage, not in the FCP module. For one thing, > not all LLDs use that fcp module. Great point. Redelivery soon out. good weekend Hillf -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Open-FCoE] [PATCH] add stats info for FC rports 2010-12-10 13:35 ` Hillf Danton @ 2010-12-10 15:13 ` Hillf Danton 0 siblings, 0 replies; 6+ messages in thread From: Hillf Danton @ 2010-12-10 15:13 UTC (permalink / raw) To: Joe Eykholt; +Cc: devel, linux-scsi It is the second version with two changes. This version could not be completed without help from Joe, specially on rcu and tearing lport down. Thank you, Joe. Hillf --- --- a/include/scsi/libfc.h 2010-11-01 19:54:12.000000000 +0800 +++ b/include/scsi/libfc.h 2010-12-08 21:05:46.000000000 +0800 @@ -803,6 +803,9 @@ struct fc_lport { void *scsi_priv; struct fc_disc disc; + /* stats info of the usage of rports */ + unsigned long nr_rdata; + /* Virtual port information */ struct list_head vports; struct fc_vport *vport; --- a/drivers/scsi/libfc/fc_rport.c 2010-11-01 19:54:12.000000000 +0800 +++ b/drivers/scsi/libfc/fc_rport.c 2010-12-10 23:10:54.000000000 +0800 @@ -129,6 +129,7 @@ static struct fc_rport_priv *fc_rport_cr if (!rdata) return NULL; + lport->nr_rdata++; rdata->ids.node_name = -1; rdata->ids.port_name = -1; rdata->ids.port_id = port_id; @@ -171,6 +172,7 @@ static void fc_rport_destroy(struct kref struct fc_rport_priv *rdata; rdata = container_of(kref, struct fc_rport_priv, kref); + rdata->local_port->nr_rdata--; call_rcu(&rdata->rcu, fc_rport_free_rcu); } --- a/drivers/scsi/libfc/fc_lport.c 2010-11-01 19:54:12.000000000 +0800 +++ b/drivers/scsi/libfc/fc_lport.c 2010-12-10 23:06:00.000000000 +0800 @@ -629,6 +629,10 @@ int fc_lport_destroy(struct fc_lport *lp lport->tt.fcp_abort_io(lport); lport->tt.disc_stop_final(lport); lport->tt.exch_mgr_reset(lport, 0, 0); + if (lport->nr_rdata != 0) + printk(KERN_WARNING "libfc: %lu Leaked rports when " + "destroying local port (%6.6x)\n", + lport->nr_rdata, lport->port_id); return 0; } EXPORT_SYMBOL(fc_lport_destroy); ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-12-10 15:13 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-12-08 13:27 [PATCH] add stats info for FC rports Hillf Danton 2010-12-08 22:06 ` [Open-FCoE] " Joe Eykholt 2010-12-09 14:18 ` Hillf Danton 2010-12-09 19:17 ` Joe Eykholt 2010-12-10 13:35 ` Hillf Danton 2010-12-10 15:13 ` Hillf Danton
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).