[PATCH] qla2xxx: Don't memset buffer unless debug level is enabled

[PATCH] qla2xxx: Don't memset buffer unless debug level is enabled

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

During recent perf testing with qla_target.c logic, dl_dbg() appeared
as one of the top CPU consumers (~5% CPU) with small block random I/O
even when no debug levels have been enabled.

This patch moves the explict memset() in four qla_dbc.c functions
within code blocks to where we know it will actually be used to save
the extra wasted cycles.

Cc: Andrew Vasquez <andrew.vasquez@qlogic.com>
Cc: Giridhar Malavali <giridhar.malavali@qlogic.com>
Cc: Roland Dreier <roland@purestorage.com>
Cc: Joern Engel <joern@logfs.org>
Cc: James Bottomley <JBottomley@Parallels.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/scsi/qla2xxx/qla_dbg.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_dbg.c b/drivers/scsi/qla2xxx/qla_dbg.c
index 07372de..cc32ec8 100644
--- a/drivers/scsi/qla2xxx/qla_dbg.c
+++ b/drivers/scsi/qla2xxx/qla_dbg.c
@@ -1676,11 +1676,11 @@ ql_dbg(uint32_t level, scsi_qla_host_t *vha, int32_t id, char *msg, ...) {
 	uint32_t len;
 	struct pci_dev *pdev = NULL;
 
-	memset(pbuf, 0, QL_DBG_BUF_LEN);
-
 	va_start(ap, msg);
 
 	if ((level & ql2xextended_error_logging) == level) {
+		memset(pbuf, 0, QL_DBG_BUF_LEN);
+		
 		if (vha != NULL) {
 			pdev = vha->hw->pdev;
 			/* <module-name> <pci-name> <msg-id>:<host> Message */
@@ -1724,11 +1724,11 @@ ql_dbg_pci(uint32_t level, struct pci_dev *pdev, int32_t id, char *msg, ...) {
 	if (pdev == NULL)
 		return;
 
-	memset(pbuf, 0, QL_DBG_BUF_LEN);
-
 	va_start(ap, msg);
 
 	if ((level & ql2xextended_error_logging) == level) {
+		memset(pbuf, 0, QL_DBG_BUF_LEN);
+		
 		/* <module-name> <dev-name>:<msg-id> Message */
 		sprintf(pbuf, "%s [%s]-%04x: : ", QL_MSGHDR,
 		    dev_name(&(pdev->dev)), id + ql_dbg_offset);
@@ -1763,11 +1763,11 @@ ql_log(uint32_t level, scsi_qla_host_t *vha, int32_t id, char *msg, ...) {
 	uint32_t len;
 	struct pci_dev *pdev = NULL;
 
-	memset(pbuf, 0, QL_DBG_BUF_LEN);
-
 	va_start(ap, msg);
 
 	if (level <= ql_errlev) {
+		memset(pbuf, 0, QL_DBG_BUF_LEN);
+
 		if (vha != NULL) {
 			pdev = vha->hw->pdev;
 			/* <module-name> <msg-id>:<host> Message */
@@ -1823,11 +1823,11 @@ ql_log_pci(uint32_t level, struct pci_dev *pdev, int32_t id, char *msg, ...) {
 	if (pdev == NULL)
 		return;
 
-	memset(pbuf, 0, QL_DBG_BUF_LEN);
-
 	va_start(ap, msg);
 
 	if (level <= ql_errlev) {
+		memset(pbuf, 0, QL_DBG_BUF_LEN);
+
 		/* <module-name> <dev-name>:<msg-id> Message */
 		sprintf(pbuf, "%s [%s]-%04x: : ", QL_MSGHDR,
 		    dev_name(&(pdev->dev)), id);
-- 
1.7.2.5

Re: [PATCH] qla2xxx: Don't memset buffer unless debug level is enabled

[Chad Dupuis]

Looks good. Thanks for the find.

Acked-by: Chad Dupuis <chad.dupuis@qlogic.com>

On Wed, 30 Nov 2011, Nicholas A. Bellinger wrote:

> From: Nicholas Bellinger <nab@linux-iscsi.org>
>
> During recent perf testing with qla_target.c logic, dl_dbg() appeared
> as one of the top CPU consumers (~5% CPU) with small block random I/O
> even when no debug levels have been enabled.
>
> This patch moves the explict memset() in four qla_dbc.c functions
> within code blocks to where we know it will actually be used to save
> the extra wasted cycles.
>
> Cc: Andrew Vasquez <andrew.vasquez@qlogic.com>
> Cc: Giridhar Malavali <giridhar.malavali@qlogic.com>
> Cc: Roland Dreier <roland@purestorage.com>
> Cc: Joern Engel <joern@logfs.org>
> Cc: James Bottomley <JBottomley@Parallels.com>
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> ---
> drivers/scsi/qla2xxx/qla_dbg.c |   16 ++++++++--------
> 1 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/scsi/qla2xxx/qla_dbg.c b/drivers/scsi/qla2xxx/qla_dbg.c
> index 07372de..cc32ec8 100644
> --- a/drivers/scsi/qla2xxx/qla_dbg.c
> +++ b/drivers/scsi/qla2xxx/qla_dbg.c
> @@ -1676,11 +1676,11 @@ ql_dbg(uint32_t level, scsi_qla_host_t *vha, int32_t id, char *msg, ...) {
>       uint32_t len;
>       struct pci_dev *pdev = NULL;
>
> -     memset(pbuf, 0, QL_DBG_BUF_LEN);
> -
>       va_start(ap, msg);
>
>       if ((level & ql2xextended_error_logging) == level) {
> +             memset(pbuf, 0, QL_DBG_BUF_LEN);
> +
>               if (vha != NULL) {
>                       pdev = vha->hw->pdev;
>                       /* <module-name> <pci-name> <msg-id>:<host> Message */
> @@ -1724,11 +1724,11 @@ ql_dbg_pci(uint32_t level, struct pci_dev *pdev, int32_t id, char *msg, ...) {
>       if (pdev == NULL)
>               return;
>
> -     memset(pbuf, 0, QL_DBG_BUF_LEN);
> -
>       va_start(ap, msg);
>
>       if ((level & ql2xextended_error_logging) == level) {
> +             memset(pbuf, 0, QL_DBG_BUF_LEN);
> +
>               /* <module-name> <dev-name>:<msg-id> Message */
>               sprintf(pbuf, "%s [%s]-%04x: : ", QL_MSGHDR,
>                   dev_name(&(pdev->dev)), id + ql_dbg_offset);
> @@ -1763,11 +1763,11 @@ ql_log(uint32_t level, scsi_qla_host_t *vha, int32_t id, char *msg, ...) {
>       uint32_t len;
>       struct pci_dev *pdev = NULL;
>
> -     memset(pbuf, 0, QL_DBG_BUF_LEN);
> -
>       va_start(ap, msg);
>
>       if (level <= ql_errlev) {
> +             memset(pbuf, 0, QL_DBG_BUF_LEN);
> +
>               if (vha != NULL) {
>                       pdev = vha->hw->pdev;
>                       /* <module-name> <msg-id>:<host> Message */
> @@ -1823,11 +1823,11 @@ ql_log_pci(uint32_t level, struct pci_dev *pdev, int32_t id, char *msg, ...) {
>       if (pdev == NULL)
>               return;
>
> -     memset(pbuf, 0, QL_DBG_BUF_LEN);
> -
>       va_start(ap, msg);
>
>       if (level <= ql_errlev) {
> +             memset(pbuf, 0, QL_DBG_BUF_LEN);
> +
>               /* <module-name> <dev-name>:<msg-id> Message */
>               sprintf(pbuf, "%s [%s]-%04x: : ", QL_MSGHDR,
>                   dev_name(&(pdev->dev)), id);
>

This message and any attached documents contain information from QLogic Corporation or its wholly-owned subsidiaries that may be confidential. If you are not the intended recipient, you may not read, copy, distribute, or use this information. If you have received this transmission in error, please notify the sender immediately by reply e-mail and then delete this message.

Re: [PATCH] qla2xxx: Don't memset buffer unless debug level is enabled

[Sebastian Andrzej Siewior]

On 11/30/2011 11:03 PM, Nicholas A. Bellinger wrote:
> diff --git a/drivers/scsi/qla2xxx/qla_dbg.c b/drivers/scsi/qla2xxx/qla_dbg.c
> index 07372de..cc32ec8 100644
> --- a/drivers/scsi/qla2xxx/qla_dbg.c
> +++ b/drivers/scsi/qla2xxx/qla_dbg.c
> @@ -1676,11 +1676,11 @@ ql_dbg(uint32_t level, scsi_qla_host_t *vha, int32_t id, char *msg, ...) {
>   	uint32_t len;
>   	struct pci_dev *pdev = NULL;
>
> -	memset(pbuf, 0, QL_DBG_BUF_LEN);
> -
>   	va_start(ap, msg);
>
>   	if ((level&  ql2xextended_error_logging) == level) {
> +		memset(pbuf, 0, QL_DBG_BUF_LEN);
> +		

512bytes on the stack is brave. Anyway, why do you memset the whole
buffer? As far the string processing is concerned, setting the first
byte to zero is enough.

Sebastian

Re: [PATCH] qla2xxx: Don't memset buffer unless debug level is enabled

[Nicholas A. Bellinger]

On Thu, 2011-12-01 at 09:12 +0100, Sebastian Andrzej Siewior wrote:
> On 11/30/2011 11:03 PM, Nicholas A. Bellinger wrote:
> > diff --git a/drivers/scsi/qla2xxx/qla_dbg.c b/drivers/scsi/qla2xxx/qla_dbg.c
> > index 07372de..cc32ec8 100644
> > --- a/drivers/scsi/qla2xxx/qla_dbg.c
> > +++ b/drivers/scsi/qla2xxx/qla_dbg.c
> > @@ -1676,11 +1676,11 @@ ql_dbg(uint32_t level, scsi_qla_host_t *vha, int32_t id, char *msg, ...) {
> >   	uint32_t len;
> >   	struct pci_dev *pdev = NULL;
> >
> > -	memset(pbuf, 0, QL_DBG_BUF_LEN);
> > -
> >   	va_start(ap, msg);
> >
> >   	if ((level&  ql2xextended_error_logging) == level) {
> > +		memset(pbuf, 0, QL_DBG_BUF_LEN);
> > +		
> 
> 512bytes on the stack is brave.

...

>  Anyway, why do you memset the whole
> buffer? As far the string processing is concerned, setting the first
> byte to zero is enough.
> 

After converting qla_target.c in a few hundred locations to use
qla_dbg() my wrists are still hurting, so you can imagine I'm not real
eager to look into this code.  ;)

Anyways, memset of the whole buffer is overkill.  Since sprintf() is
being used for qla_dbg.c cases, even the 1 byte memset is unnecessary.

--nab

>From 25541e7f09b87de0000b11506d49695183aab4f1 Mon Sep 17 00:00:00 2001
From: Nicholas Bellinger <nab@linux-iscsi.org>
Date: Wed, 30 Nov 2011 13:57:02 -0800
Subject: [PATCH] qla2xxx: Remove memset debug buffer with sprintf usage

During recent perf testing with qla_target.c logic, dl_dbg() appeared
as one of the top CPU consumers (~5% CPU) with small block random I/O
even when no debug levels have been enabled.

This patch drops the explict memset() in four qla_dbc.c functions
as we expect sprintf() to add the trailing NULL when debug is enabled.

Cc: Andrew Vasquez <andrew.vasquez@qlogic.com>
Cc: Giridhar Malavali <giridhar.malavali@qlogic.com>
Cc: Roland Dreier <roland@purestorage.com>
Cc: Joern Engel <joern@logfs.org>
Cc: James Bottomley <JBottomley@Parallels.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/scsi/qla2xxx/qla_dbg.c |    8 --------
 1 files changed, 0 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_dbg.c b/drivers/scsi/qla2xxx/qla_dbg.c
index 07372de..ef31819 100644
--- a/drivers/scsi/qla2xxx/qla_dbg.c
+++ b/drivers/scsi/qla2xxx/qla_dbg.c
@@ -1676,8 +1676,6 @@ ql_dbg(uint32_t level, scsi_qla_host_t *vha, int32_t id, char *msg, ...) {
 	uint32_t len;
 	struct pci_dev *pdev = NULL;
 
-	memset(pbuf, 0, QL_DBG_BUF_LEN);
-
 	va_start(ap, msg);
 
 	if ((level & ql2xextended_error_logging) == level) {
@@ -1724,8 +1722,6 @@ ql_dbg_pci(uint32_t level, struct pci_dev *pdev, int32_t id, char *msg, ...) {
 	if (pdev == NULL)
 		return;
 
-	memset(pbuf, 0, QL_DBG_BUF_LEN);
-
 	va_start(ap, msg);
 
 	if ((level & ql2xextended_error_logging) == level) {
@@ -1763,8 +1759,6 @@ ql_log(uint32_t level, scsi_qla_host_t *vha, int32_t id, char *msg, ...) {
 	uint32_t len;
 	struct pci_dev *pdev = NULL;
 
-	memset(pbuf, 0, QL_DBG_BUF_LEN);
-
 	va_start(ap, msg);
 
 	if (level <= ql_errlev) {
@@ -1823,8 +1817,6 @@ ql_log_pci(uint32_t level, struct pci_dev *pdev, int32_t id, char *msg, ...) {
 	if (pdev == NULL)
 		return;
 
-	memset(pbuf, 0, QL_DBG_BUF_LEN);
-
 	va_start(ap, msg);
 
 	if (level <= ql_errlev) {
-- 
1.7.2.5

root@tifa:/usr/src/lio-core-2.6.git# 

Re: [PATCH] qla2xxx: Don't memset buffer unless debug level is enabled

[Chad Dupuis]

On Thu, 1 Dec 2011, Nicholas A. Bellinger wrote:

> On Thu, 2011-12-01 at 09:12 +0100, Sebastian Andrzej Siewior wrote:
>> On 11/30/2011 11:03 PM, Nicholas A. Bellinger wrote:
>>> diff --git a/drivers/scsi/qla2xxx/qla_dbg.c b/drivers/scsi/qla2xxx/qla_dbg.c
>>> index 07372de..cc32ec8 100644
>>> --- a/drivers/scsi/qla2xxx/qla_dbg.c
>>> +++ b/drivers/scsi/qla2xxx/qla_dbg.c
>>> @@ -1676,11 +1676,11 @@ ql_dbg(uint32_t level, scsi_qla_host_t *vha, int32_t id, char *msg, ...) {
>>>     uint32_t len;
>>>     struct pci_dev *pdev = NULL;
>>>
>>> -   memset(pbuf, 0, QL_DBG_BUF_LEN);
>>> -
>>>     va_start(ap, msg);
>>>
>>>     if ((level&  ql2xextended_error_logging) == level) {
>>> +           memset(pbuf, 0, QL_DBG_BUF_LEN);
>>> +
>>
>> 512bytes on the stack is brave.
>
> ...
>
>>  Anyway, why do you memset the whole
>> buffer? As far the string processing is concerned, setting the first
>> byte to zero is enough.
>>
>
> After converting qla_target.c in a few hundred locations to use
> qla_dbg() my wrists are still hurting, so you can imagine I'm not real
> eager to look into this code.  ;)
>
> Anyways, memset of the whole buffer is overkill.  Since sprintf() is
> being used for qla_dbg.c cases, even the 1 byte memset is unnecessary.

I do want to note that there is a patch posted already that removes the
memset as well:
http://marc.info/?l=linux-scsi&m=132163664809856&w=2.

>
> --nab
>
> From 25541e7f09b87de0000b11506d49695183aab4f1 Mon Sep 17 00:00:00 2001
> From: Nicholas Bellinger <nab@linux-iscsi.org>
> Date: Wed, 30 Nov 2011 13:57:02 -0800
> Subject: [PATCH] qla2xxx: Remove memset debug buffer with sprintf usage
>
> During recent perf testing with qla_target.c logic, dl_dbg() appeared
> as one of the top CPU consumers (~5% CPU) with small block random I/O
> even when no debug levels have been enabled.
>
> This patch drops the explict memset() in four qla_dbc.c functions
> as we expect sprintf() to add the trailing NULL when debug is enabled.
>
> Cc: Andrew Vasquez <andrew.vasquez@qlogic.com>
> Cc: Giridhar Malavali <giridhar.malavali@qlogic.com>
> Cc: Roland Dreier <roland@purestorage.com>
> Cc: Joern Engel <joern@logfs.org>
> Cc: James Bottomley <JBottomley@Parallels.com>
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> ---
> drivers/scsi/qla2xxx/qla_dbg.c |    8 --------
> 1 files changed, 0 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/scsi/qla2xxx/qla_dbg.c b/drivers/scsi/qla2xxx/qla_dbg.c
> index 07372de..ef31819 100644
> --- a/drivers/scsi/qla2xxx/qla_dbg.c
> +++ b/drivers/scsi/qla2xxx/qla_dbg.c
> @@ -1676,8 +1676,6 @@ ql_dbg(uint32_t level, scsi_qla_host_t *vha, int32_t id, char *msg, ...) {
>       uint32_t len;
>       struct pci_dev *pdev = NULL;
>
> -     memset(pbuf, 0, QL_DBG_BUF_LEN);
> -
>       va_start(ap, msg);
>
>       if ((level & ql2xextended_error_logging) == level) {
> @@ -1724,8 +1722,6 @@ ql_dbg_pci(uint32_t level, struct pci_dev *pdev, int32_t id, char *msg, ...) {
>       if (pdev == NULL)
>               return;
>
> -     memset(pbuf, 0, QL_DBG_BUF_LEN);
> -
>       va_start(ap, msg);
>
>       if ((level & ql2xextended_error_logging) == level) {
> @@ -1763,8 +1759,6 @@ ql_log(uint32_t level, scsi_qla_host_t *vha, int32_t id, char *msg, ...) {
>       uint32_t len;
>       struct pci_dev *pdev = NULL;
>
> -     memset(pbuf, 0, QL_DBG_BUF_LEN);
> -
>       va_start(ap, msg);
>
>       if (level <= ql_errlev) {
> @@ -1823,8 +1817,6 @@ ql_log_pci(uint32_t level, struct pci_dev *pdev, int32_t id, char *msg, ...) {
>       if (pdev == NULL)
>               return;
>
> -     memset(pbuf, 0, QL_DBG_BUF_LEN);
> -
>       va_start(ap, msg);
>
>       if (level <= ql_errlev) {
>

This message and any attached documents contain information from QLogic Corporation or its wholly-owned subsidiaries that may be confidential. If you are not the intended recipient, you may not read, copy, distribute, or use this information. If you have received this transmission in error, please notify the sender immediately by reply e-mail and then delete this message.

Re: [PATCH] qla2xxx: Don't memset buffer unless debug level is enabled

[James Bottomley]

On Thu, 2011-12-01 at 13:01 -0500, Chad Dupuis wrote:
> 
> On Thu, 1 Dec 2011, Nicholas A. Bellinger wrote:
> 
> > On Thu, 2011-12-01 at 09:12 +0100, Sebastian Andrzej Siewior wrote:
> >> On 11/30/2011 11:03 PM, Nicholas A. Bellinger wrote:
> >>> diff --git a/drivers/scsi/qla2xxx/qla_dbg.c b/drivers/scsi/qla2xxx/qla_dbg.c
> >>> index 07372de..cc32ec8 100644
> >>> --- a/drivers/scsi/qla2xxx/qla_dbg.c
> >>> +++ b/drivers/scsi/qla2xxx/qla_dbg.c
> >>> @@ -1676,11 +1676,11 @@ ql_dbg(uint32_t level, scsi_qla_host_t *vha, int32_t id, char *msg, ...) {
> >>>     uint32_t len;
> >>>     struct pci_dev *pdev = NULL;
> >>>
> >>> -   memset(pbuf, 0, QL_DBG_BUF_LEN);
> >>> -
> >>>     va_start(ap, msg);
> >>>
> >>>     if ((level&  ql2xextended_error_logging) == level) {
> >>> +           memset(pbuf, 0, QL_DBG_BUF_LEN);
> >>> +
> >>
> >> 512bytes on the stack is brave.
> >
> > ...
> >
> >>  Anyway, why do you memset the whole
> >> buffer? As far the string processing is concerned, setting the first
> >> byte to zero is enough.
> >>
> >
> > After converting qla_target.c in a few hundred locations to use
> > qla_dbg() my wrists are still hurting, so you can imagine I'm not real
> > eager to look into this code.  ;)
> >
> > Anyways, memset of the whole buffer is overkill.  Since sprintf() is
> > being used for qla_dbg.c cases, even the 1 byte memset is unnecessary.
> 
> I do want to note that there is a patch posted already that removes the
> memset as well:
> http://marc.info/?l=linux-scsi&m=132163664809856&w=2.

So since that one came from you as part of a series, I was originally
planning to merge it before you acked this one, which basically makes
this one redundant (and conflicting), doesn't it?  Which patch did you
want me to take, since I can't take both?

James

Re: [PATCH] qla2xxx: Don't memset buffer unless debug level is enabled

[Chad Dupuis]

On Thu, 1 Dec 2011, James Bottomley wrote:

> On Thu, 2011-12-01 at 13:01 -0500, Chad Dupuis wrote:
>>
>> On Thu, 1 Dec 2011, Nicholas A. Bellinger wrote:
>>
>>> On Thu, 2011-12-01 at 09:12 +0100, Sebastian Andrzej Siewior wrote:
>>>> On 11/30/2011 11:03 PM, Nicholas A. Bellinger wrote:
>>>>> diff --git a/drivers/scsi/qla2xxx/qla_dbg.c b/drivers/scsi/qla2xxx/qla_dbg.c
>>>>> index 07372de..cc32ec8 100644
>>>>> --- a/drivers/scsi/qla2xxx/qla_dbg.c
>>>>> +++ b/drivers/scsi/qla2xxx/qla_dbg.c
>>>>> @@ -1676,11 +1676,11 @@ ql_dbg(uint32_t level, scsi_qla_host_t *vha, int32_t id, char *msg, ...) {
>>>>>     uint32_t len;
>>>>>     struct pci_dev *pdev = NULL;
>>>>>
>>>>> -   memset(pbuf, 0, QL_DBG_BUF_LEN);
>>>>> -
>>>>>     va_start(ap, msg);
>>>>>
>>>>>     if ((level&  ql2xextended_error_logging) == level) {
>>>>> +           memset(pbuf, 0, QL_DBG_BUF_LEN);
>>>>> +
>>>>
>>>> 512bytes on the stack is brave.
>>>
>>> ...
>>>
>>>>  Anyway, why do you memset the whole
>>>> buffer? As far the string processing is concerned, setting the first
>>>> byte to zero is enough.
>>>>
>>>
>>> After converting qla_target.c in a few hundred locations to use
>>> qla_dbg() my wrists are still hurting, so you can imagine I'm not real
>>> eager to look into this code.  ;)
>>>
>>> Anyways, memset of the whole buffer is overkill.  Since sprintf() is
>>> being used for qla_dbg.c cases, even the 1 byte memset is unnecessary.
>>
>> I do want to note that there is a patch posted already that removes the
>> memset as well:
>> http://marc.info/?l=linux-scsi&m=132163664809856&w=2.
>
> So since that one came from you as part of a series, I was originally
> planning to merge it before you acked this one, which basically makes
> this one redundant (and conflicting), doesn't it?  Which patch did you
> want me to take, since I can't take both?

Apologies for that. Please take the first one that was part of the series.

>
> James
>
>

This message and any attached documents contain information from QLogic Corporation or its wholly-owned subsidiaries that may be confidential. If you are not the intended recipient, you may not read, copy, distribute, or use this information. If you have received this transmission in error, please notify the sender immediately by reply e-mail and then delete this message.