[PATCH] add stats info for FC rports

[PATCH] add stats info for FC rports

[Hillf Danton]

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);

Re: [Open-FCoE] [PATCH] add stats info for FC rports

[Joe Eykholt]

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

Re: [Open-FCoE] [PATCH] add stats info for FC rports

[Hillf Danton]

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

Re: [Open-FCoE] [PATCH] add stats info for FC rports

[Joe Eykholt]

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
>>

Re: [Open-FCoE] [PATCH] add stats info for FC rports

[Hillf Danton]

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

Re: [Open-FCoE] [PATCH] add stats info for FC rports

[Hillf Danton]

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);