[PATCH 0/4] drivers/xen/xenbus/xenbus_client.c: Several improvements for it

[PATCH 0/4] drivers/xen/xenbus/xenbus_client.c: Several improvements for it

[Chen Gang]

They are all for 'xenbus_client.c', please help check when you have time,
thanks.

Chen Gang (4):
  drivers/xen/xenbus/xenbus_client.c: Remove redundancy assignment to 'addr'
  drivers/xen/xenbus/xenbus_client.c: Correct the comments for xenbus_grant_ring()
  drivers/xen/xenbus/xenbus_client.c: Remove BUG_ON() when error string trucated
  drivers/xen/xenbus/xenbus_client.c: Improve the failure processing for __xenbus_switch_state()

 drivers/xen/xenbus/xenbus_client.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

-- 
1.9.3

[PATCH 1/4] drivers/xen/xenbus/xenbus_client.c: Remove redundancy asignment to 'addr'

[Chen Gang]

When failure occurs, 'node' is already set to NULL, and it is enough
for next checking (which will return in time), so need not set 'addr'.

Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 drivers/xen/xenbus/xenbus_client.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c
index 439c9dc..277e74d 100644
--- a/drivers/xen/xenbus/xenbus_client.c
+++ b/drivers/xen/xenbus/xenbus_client.c
@@ -636,7 +636,7 @@ static int xenbus_unmap_ring_vfree_hvm(struct xenbus_device *dev, void *vaddr)
 			goto found;
 		}
 	}
-	node = addr = NULL;
+	node = NULL;
  found:
 	spin_unlock(&xenbus_valloc_lock);
 
-- 
1.9.3

Re: [PATCH 1/4] drivers/xen/xenbus/xenbus_client.c: Remove redundancy asignment to 'addr'

[Chen Gang]

Oh, sorry, for subject, need use 'assignment' instead of 'asignment'.


On 09/14/2014 06:49 PM, Chen Gang wrote:
> When failure occurs, 'node' is already set to NULL, and it is enough
> for next checking (which will return in time), so need not set 'addr'.
> 
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> ---
>  drivers/xen/xenbus/xenbus_client.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c
> index 439c9dc..277e74d 100644
> --- a/drivers/xen/xenbus/xenbus_client.c
> +++ b/drivers/xen/xenbus/xenbus_client.c
> @@ -636,7 +636,7 @@ static int xenbus_unmap_ring_vfree_hvm(struct xenbus_device *dev, void *vaddr)
>  			goto found;
>  		}
>  	}
> -	node = addr = NULL;
> +	node = NULL;
>   found:
>  	spin_unlock(&xenbus_valloc_lock);
>  
> 


-- 
Chen Gang

Open share and attitude like air water and life which God blessed

Re: [Xen-devel] [PATCH 1/4] drivers/xen/xenbus/xenbus_client.c: Remove redundancy asignment to 'addr'

[David Vrabel]

On 14/09/14 11:49, Chen Gang wrote:
> When failure occurs, 'node' is already set to NULL, and it is enough
> for next checking (which will return in time), so need not set 'addr'.

I'm not going to apply this one.  The redundant assignment is harmless
and improve clarity slightly, IMO.

David

Re: [Xen-devel] [PATCH 1/4] drivers/xen/xenbus/xenbus_client.c: Remove redundancy asignment to 'addr'

[Chen Gang]

On 09/15/2014 11:41 PM, David Vrabel wrote:
> On 14/09/14 11:49, Chen Gang wrote:
>> When failure occurs, 'node' is already set to NULL, and it is enough
>> for next checking (which will return in time), so need not set 'addr'.
> 
> I'm not going to apply this one.  The redundant assignment is harmless
> and improve clarity slightly, IMO.
> 

Hmm... maybe, it may depend on personal hobby, for me, only focusing one
'control' value (in our case is 'node') is more clearer than focusing
two 'control' values ('node' and 'addr').

In kernel conding styles, I feel (but I have no any proofs for it), it
is focus on performance, if one value need not be assigned, it need be
skipped.

But all together, I need respect the related maintainer's taste, if
he/she sticks to his/her taste.


Thanks.
-- 
Chen Gang

Open share and attitude like air water and life which God blessed

[PATCH 2/4] drivers/xen/xenbus/xenbus_client.c: Correct the comments for xenbus_grant_ring()

[Chen Gang]

Both zero and positive number can indicate success, so the original
comments need be improved.

Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 drivers/xen/xenbus/xenbus_client.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c
index 277e74d..e2cf681 100644
--- a/drivers/xen/xenbus/xenbus_client.c
+++ b/drivers/xen/xenbus/xenbus_client.c
@@ -361,8 +361,8 @@ static void xenbus_switch_fatal(struct xenbus_device *dev, int depth, int err,
  * @ring_mfn: mfn of ring to grant
 
  * Grant access to the given @ring_mfn to the peer of the given device.  Return
- * 0 on success, or -errno on error.  On error, the device will switch to
- * XenbusStateClosing, and the error will be saved in the store.
+ * 0 or positive number on success, or -errno on error.  On error, the device
+ * will switch to XenbusStateClosing, and the error will be saved in the store.
  */
 int xenbus_grant_ring(struct xenbus_device *dev, unsigned long ring_mfn)
 {
-- 
1.9.3

Re: [Xen-devel] [PATCH 2/4] drivers/xen/xenbus/xenbus_client.c: Correct the comments for xenbus_grant_ring()

[David Vrabel]

On 14/09/14 11:50, Chen Gang wrote:
> Both zero and positive number can indicate success, so the original
> comments need be improved.
> 
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> ---
>  drivers/xen/xenbus/xenbus_client.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c
> index 277e74d..e2cf681 100644
> --- a/drivers/xen/xenbus/xenbus_client.c
> +++ b/drivers/xen/xenbus/xenbus_client.c
> @@ -361,8 +361,8 @@ static void xenbus_switch_fatal(struct xenbus_device *dev, int depth, int err,
>   * @ring_mfn: mfn of ring to grant
>  
>   * Grant access to the given @ring_mfn to the peer of the given device.  Return
> - * 0 on success, or -errno on error.  On error, the device will switch to
> - * XenbusStateClosing, and the error will be saved in the store.
> + * 0 or positive number on success, or -errno on error.  On error, the device
> + * will switch to XenbusStateClosing, and the error will be saved in the store.

If you're going to update this comment, please state what it is actually
returning: i.e., a grant reference or -ve on error.

David

Re: [Xen-devel] [PATCH 2/4] drivers/xen/xenbus/xenbus_client.c: Correct the comments for xenbus_grant_ring()

[Chen Gang]

On 09/15/2014 11:27 PM, David Vrabel wrote:
> On 14/09/14 11:50, Chen Gang wrote:
>> Both zero and positive number can indicate success, so the original
>> comments need be improved.
>>
>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
>> ---
>>  drivers/xen/xenbus/xenbus_client.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c
>> index 277e74d..e2cf681 100644
>> --- a/drivers/xen/xenbus/xenbus_client.c
>> +++ b/drivers/xen/xenbus/xenbus_client.c
>> @@ -361,8 +361,8 @@ static void xenbus_switch_fatal(struct xenbus_device *dev, int depth, int err,
>>   * @ring_mfn: mfn of ring to grant
>>  
>>   * Grant access to the given @ring_mfn to the peer of the given device.  Return
>> - * 0 on success, or -errno on error.  On error, the device will switch to
>> - * XenbusStateClosing, and the error will be saved in the store.
>> + * 0 or positive number on success, or -errno on error.  On error, the device
>> + * will switch to XenbusStateClosing, and the error will be saved in the store.
> 
> If you're going to update this comment, please state what it is actually
> returning: i.e., a grant reference or -ve on error.
> 

OK, thanks, I shall update it when send patch v2 for it.

Thanks.
-- 
Chen Gang

Open share and attitude like air water and life which God blessed

[PATCH 3/4] drivers/xen/xenbus/xenbus_client.c: Remove BUG_ON() when error string truncated

[Chen Gang]

xenbus_va_dev_error() is for printing error, so when error string is
too long to be truncated, need not BUG_ON(), still return truncation
string is OK.

Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 drivers/xen/xenbus/xenbus_client.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c
index e2cf681..620ffd2 100644
--- a/drivers/xen/xenbus/xenbus_client.c
+++ b/drivers/xen/xenbus/xenbus_client.c
@@ -259,7 +259,6 @@ static char *error_path(struct xenbus_device *dev)
 static void xenbus_va_dev_error(struct xenbus_device *dev, int err,
 				const char *fmt, va_list ap)
 {
-	int ret;
 	unsigned int len;
 	char *printf_buffer = NULL;
 	char *path_buffer = NULL;
@@ -270,9 +269,7 @@ static void xenbus_va_dev_error(struct xenbus_device *dev, int err,
 		goto fail;
 
 	len = sprintf(printf_buffer, "%i ", -err);
-	ret = vsnprintf(printf_buffer+len, PRINTF_BUFFER_SIZE-len, fmt, ap);
-
-	BUG_ON(len + ret > PRINTF_BUFFER_SIZE-1);
+	vsnprintf(printf_buffer+len, PRINTF_BUFFER_SIZE-len, fmt, ap);
 
 	dev_err(&dev->dev, "%s\n", printf_buffer);
 
-- 
1.9.3

[PATCH 4/4] drivers/xen/xenbus/xenbus_client.c: Improve the failure processing for __xenbus_switch_state()

[Chen Gang]

When failure occurs, need return failure code instead of 0, or the upper
caller will misunderstand.

Also when retry for EAGAIN reason, better to schedule out for a while,
so can let others have chance to continue their tasks (especially,
their tasks are related EAGAIN under UP kernel).

Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 drivers/xen/xenbus/xenbus_client.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c
index 620ffd2..fc8699b 100644
--- a/drivers/xen/xenbus/xenbus_client.c
+++ b/drivers/xen/xenbus/xenbus_client.c
@@ -36,6 +36,7 @@
 #include <linux/spinlock.h>
 #include <linux/vmalloc.h>
 #include <linux/export.h>
+#include <linux/delay.h>
 #include <asm/xen/hypervisor.h>
 #include <asm/xen/page.h>
 #include <xen/interface/xen.h>
@@ -196,7 +197,7 @@ again:
 	err = xenbus_transaction_start(&xbt);
 	if (err) {
 		xenbus_switch_fatal(dev, depth, err, "starting transaction");
-		return 0;
+		return err;
 	}
 
 	err = xenbus_scanf(xbt, dev->nodename, "state", "%d", &current_state);
@@ -213,13 +214,15 @@ again:
 abort:
 	err = xenbus_transaction_end(xbt, abort);
 	if (err) {
-		if (err == -EAGAIN && !abort)
+		if (err == -EAGAIN && !abort) {
+			msleep(20);
 			goto again;
+		}
 		xenbus_switch_fatal(dev, depth, err, "ending transaction");
 	} else
 		dev->state = state;
 
-	return 0;
+	return err;
 }
 
 /**
-- 
1.9.3

Re: [PATCH 4/4] drivers/xen/xenbus/xenbus_client.c: Improve the failure processing for __xenbus_switch_state()

[David Vrabel]

On 14/09/14 11:52, Chen Gang wrote:
> When failure occurs, need return failure code instead of 0, or the upper
> caller will misunderstand.
> 
> Also when retry for EAGAIN reason, better to schedule out for a while,
> so can let others have chance to continue their tasks (especially,
> their tasks are related EAGAIN under UP kernel).

Is this fixing a real world problem you have seen?

xenbus_scanf() and xenbus_printf() already sleep while waiting for the
response and delaying isn't going to reduce the likelihood of the
transaction being aborted on the retry.

David

Re: [PATCH 4/4] drivers/xen/xenbus/xenbus_client.c: Improve the failure processing for __xenbus_switch_state()

[Chen Gang]

On 09/15/2014 10:39 PM, David Vrabel wrote:
> On 14/09/14 11:52, Chen Gang wrote:
>> When failure occurs, need return failure code instead of 0, or the upper
>> caller will misunderstand.
>>
>> Also when retry for EAGAIN reason, better to schedule out for a while,
>> so can let others have chance to continue their tasks (especially,
>> their tasks are related EAGAIN under UP kernel).
> 
> Is this fixing a real world problem you have seen?
> 

Not real world, only reading by source code, and some of upper level
callers really check the return value, indirectly (they may
misunderstand).

> xenbus_scanf() and xenbus_printf() already sleep while waiting for the
> response and delaying isn't going to reduce the likelihood of the
> transaction being aborted on the retry.
> 

OK, thanks, what you said sound reasonable to me, I shall remove the
waiting code when send patch v2 for it.


I shall try to send patch v2 within this week end (2014-09-21), if it is
too late to bare, please let me know (I shall try in time).

Thanks.
-- 
Chen Gang

Open share and attitude like air water and life which God blessed

Re: [PATCH 4/4] drivers/xen/xenbus/xenbus_client.c: Improve the failure processing for __xenbus_switch_state()

[Boris Ostrovsky]

On 09/14/2014 06:52 AM, Chen Gang wrote:
> When failure occurs, need return failure code instead of 0, or the upper
> caller will misunderstand.
>
> Also when retry for EAGAIN reason, better to schedule out for a while,
> so can let others have chance to continue their tasks (especially,
> their tasks are related EAGAIN under UP kernel).
>
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> ---
>   drivers/xen/xenbus/xenbus_client.c | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c
> index 620ffd2..fc8699b 100644
> --- a/drivers/xen/xenbus/xenbus_client.c
> +++ b/drivers/xen/xenbus/xenbus_client.c
> @@ -36,6 +36,7 @@
>   #include <linux/spinlock.h>
>   #include <linux/vmalloc.h>
>   #include <linux/export.h>
> +#include <linux/delay.h>
>   #include <asm/xen/hypervisor.h>
>   #include <asm/xen/page.h>
>   #include <xen/interface/xen.h>
> @@ -196,7 +197,7 @@ again:
>   	err = xenbus_transaction_start(&xbt);
>   	if (err) {
>   		xenbus_switch_fatal(dev, depth, err, "starting transaction");
> -		return 0;
> +		return err;
>   	}
>   
>   	err = xenbus_scanf(xbt, dev->nodename, "state", "%d", &current_state);
> @@ -213,13 +214,15 @@ again:
>   abort:
>   	err = xenbus_transaction_end(xbt, abort);
>   	if (err) {
> -		if (err == -EAGAIN && !abort)
> +		if (err == -EAGAIN && !abort) {
> +			msleep(20);

I'd suggest you use schedule() here.

-boris

>   			goto again;
> +		}
>   		xenbus_switch_fatal(dev, depth, err, "ending transaction");
>   	} else
>   		dev->state = state;
>   
> -	return 0;
> +	return err;
>   }
>   
>   /**

Re: [PATCH 4/4] drivers/xen/xenbus/xenbus_client.c: Improve the failure processing for __xenbus_switch_state()

[Chen Gang]

On 9/16/14 9:07, Boris Ostrovsky wrote:
> 
> On 09/14/2014 06:52 AM, Chen Gang wrote:
>> When failure occurs, need return failure code instead of 0, or the upper
>> caller will misunderstand.
>>
>> Also when retry for EAGAIN reason, better to schedule out for a while,
>> so can let others have chance to continue their tasks (especially,
>> their tasks are related EAGAIN under UP kernel).
>>
>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
>> ---
>>   drivers/xen/xenbus/xenbus_client.c | 9 ++++++---
>>   1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c
>> index 620ffd2..fc8699b 100644
>> --- a/drivers/xen/xenbus/xenbus_client.c
>> +++ b/drivers/xen/xenbus/xenbus_client.c
>> @@ -36,6 +36,7 @@
>>   #include <linux/spinlock.h>
>>   #include <linux/vmalloc.h>
>>   #include <linux/export.h>
>> +#include <linux/delay.h>
>>   #include <asm/xen/hypervisor.h>
>>   #include <asm/xen/page.h>
>>   #include <xen/interface/xen.h>
>> @@ -196,7 +197,7 @@ again:
>>       err = xenbus_transaction_start(&xbt);
>>       if (err) {
>>           xenbus_switch_fatal(dev, depth, err, "starting transaction");
>> -        return 0;
>> +        return err;
>>       }
>>         err = xenbus_scanf(xbt, dev->nodename, "state", "%d", &current_state);
>> @@ -213,13 +214,15 @@ again:
>>   abort:
>>       err = xenbus_transaction_end(xbt, abort);
>>       if (err) {
>> -        if (err == -EAGAIN && !abort)
>> +        if (err == -EAGAIN && !abort) {
>> +            msleep(20);
> 
> I'd suggest you use schedule() here.
> 

For me, it is really better than msleep(20).

But just like the related maintainer said, we do not need it, since it
has already "have a rest" inside xenbus_transaction_end().


Thanks.
-- 
Chen Gang

Open, share, and attitude like air, water, and life which God blessed

Re: [Xen-devel] [PATCH 0/4] drivers/xen/xenbus/xenbus_client.c: Several improvements for it

[David Vrabel]

On 14/09/14 11:48, Chen Gang wrote:
> They are all for 'xenbus_client.c', please help check when you have time,
> thanks.
> 
> Chen Gang (4):
>   drivers/xen/xenbus/xenbus_client.c: Remove redundancy assignment to 'addr'
>   drivers/xen/xenbus/xenbus_client.c: Correct the comments for xenbus_grant_ring()
>   drivers/xen/xenbus/xenbus_client.c: Remove BUG_ON() when error string trucated
>   drivers/xen/xenbus/xenbus_client.c: Improve the failure processing for __xenbus_switch_state()

The prefix should be the subsystem not the file (files move around).
i.e., xen/xenbus.

David

Re: [Xen-devel] [PATCH 0/4] drivers/xen/xenbus/xenbus_client.c: Several improvements for it

[Chen Gang]

On 09/15/2014 11:31 PM, David Vrabel wrote:
> On 14/09/14 11:48, Chen Gang wrote:
>> They are all for 'xenbus_client.c', please help check when you have time,
>> thanks.
>>
>> Chen Gang (4):
>>   drivers/xen/xenbus/xenbus_client.c: Remove redundancy assignment to 'addr'
>>   drivers/xen/xenbus/xenbus_client.c: Correct the comments for xenbus_grant_ring()
>>   drivers/xen/xenbus/xenbus_client.c: Remove BUG_ON() when error string trucated
>>   drivers/xen/xenbus/xenbus_client.c: Improve the failure processing for __xenbus_switch_state()
> 
> The prefix should be the subsystem not the file (files move around).
> i.e., xen/xenbus.
> 

OK, thanks, I shall notice about it when send patch v2 for it.

Thanks.
-- 
Chen Gang

Open share and attitude like air water and life which God blessed