Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1575928 > unrolled thread

Re: [PATCH v3 3/3] xen: optimize xenbus driver for multiple concurrent xenstore accesses

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 3/3] xen: optimize xenbus driver for multiple  concurrent xenstore accesses Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2017-02-07 19:00 +0100
    Re: [PATCH v3 3/3] xen: optimize xenbus driver for multiple  concurrent xenstore accesses Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2017-02-07 23:40 +0100
      Re: [PATCH v3 3/3] xen: optimize xenbus driver for multiple  concurrent xenstore accesses Juergen Gross <jgross@suse.com> - 2017-02-08 07:30 +0100

#1575928 — Re: [PATCH v3 3/3] xen: optimize xenbus driver for multiple concurrent xenstore accesses

On 01/24/2017 11:23 AM, Juergen Gross wrote:
> On 24/01/17 14:47, Boris Ostrovsky wrote:
>> On 01/23/2017 01:59 PM, Boris Ostrovsky wrote:
>>> On 01/23/2017 05:09 AM, Juergen Gross wrote:
>>>> Handling of multiple concurrent Xenstore accesses through xenbus driver
>>>> either from the kernel or user land is rather lame today: xenbus is
>>>> capable to have one access active only at one point of time.
>>>>
>>>>
>>
>> This patch appears to break save/restore:
> Hmm, tried multiple times, but I can't reproduce this issue.
>
> Anything special in the setup? I tried a 64 bit pv guest and did
> "xl save".
>
> Do I have to run some load in parallel?

Any luck reproducing this? I am still failing the test on dumpdata but I
couldn't reproduce it on another system.

-boris

[toc] | [next] | [standalone]

#1576127

On 02/07/2017 12:51 PM, Boris Ostrovsky wrote:
> On 01/24/2017 11:23 AM, Juergen Gross wrote:
>> On 24/01/17 14:47, Boris Ostrovsky wrote:
>>> On 01/23/2017 01:59 PM, Boris Ostrovsky wrote:
>>>> On 01/23/2017 05:09 AM, Juergen Gross wrote:
>>>>> Handling of multiple concurrent Xenstore accesses through xenbus driver
>>>>> either from the kernel or user land is rather lame today: xenbus is
>>>>> capable to have one access active only at one point of time.
>>>>>
>>>>>
>>> This patch appears to break save/restore:
>> Hmm, tried multiple times, but I can't reproduce this issue.
>>
>> Anything special in the setup? I tried a 64 bit pv guest and did
>> "xl save".
>>
>> Do I have to run some load in parallel?
> Any luck reproducing this? I am still failing the test on dumpdata but I
> couldn't reproduce it on another system.


The problem appears to be xs_state_users being non-zero when we call
xs_suspend_enter().

From what I understand this is caused by xs_request_exit() not
decrementing it when closing a transaction. This seems to be happening
when XS_TRANSACTION_END transaction returns XS_ERROR (I haven't traced
what causes this error but it doesn't appear to cause any visible harm).

Does the patch below make sense?

diff --git a/drivers/xen/xenbus/xenbus_xs.c b/drivers/xen/xenbus/xenbus_xs.c
index e62cb09..ffd5fac 100644
--- a/drivers/xen/xenbus/xenbus_xs.c
+++ b/drivers/xen/xenbus/xenbus_xs.c
@@ -140,7 +140,7 @@ void xs_request_exit(struct xb_req_data *req)
        spin_lock(&xs_state_lock);
        xs_state_users--;
        if ((req->type == XS_TRANSACTION_START && req->msg.type ==
XS_ERROR) ||
-           req->msg.type == XS_TRANSACTION_END)
+           req->type == XS_TRANSACTION_END)
                xs_state_users--;
        spin_unlock(&xs_state_lock);


I ran a few tests on dumpdata and they completed successfully. I'll keep
this for the overnight runs too, with a different Xen version.

-boris

[toc] | [prev] | [next] | [standalone]

#1576293

On 07/02/17 23:39, Boris Ostrovsky wrote:
> On 02/07/2017 12:51 PM, Boris Ostrovsky wrote:
>> On 01/24/2017 11:23 AM, Juergen Gross wrote:
>>> On 24/01/17 14:47, Boris Ostrovsky wrote:
>>>> On 01/23/2017 01:59 PM, Boris Ostrovsky wrote:
>>>>> On 01/23/2017 05:09 AM, Juergen Gross wrote:
>>>>>> Handling of multiple concurrent Xenstore accesses through xenbus driver
>>>>>> either from the kernel or user land is rather lame today: xenbus is
>>>>>> capable to have one access active only at one point of time.
>>>>>>
>>>>>>
>>>> This patch appears to break save/restore:
>>> Hmm, tried multiple times, but I can't reproduce this issue.
>>>
>>> Anything special in the setup? I tried a 64 bit pv guest and did
>>> "xl save".
>>>
>>> Do I have to run some load in parallel?
>> Any luck reproducing this? I am still failing the test on dumpdata but I
>> couldn't reproduce it on another system.
> 
> 
> The problem appears to be xs_state_users being non-zero when we call
> xs_suspend_enter().
> 
> From what I understand this is caused by xs_request_exit() not
> decrementing it when closing a transaction. This seems to be happening
> when XS_TRANSACTION_END transaction returns XS_ERROR (I haven't traced
> what causes this error but it doesn't appear to cause any visible harm).

Aah, of course: this will happen for a transaction failing due to a
conflict (EAGAIN case).

> Does the patch below make sense?

As the xenbus driver is checking for the transaction id to be valid this
is okay, I think.

I have noticed another problem, though: a user client mixing Xenstore
accesses with and without transactions could be blocked when doing a
non-transactional access hindering it from completing the transaction.

I'll send an updated version including your fix.

> 
> diff --git a/drivers/xen/xenbus/xenbus_xs.c b/drivers/xen/xenbus/xenbus_xs.c
> index e62cb09..ffd5fac 100644
> --- a/drivers/xen/xenbus/xenbus_xs.c
> +++ b/drivers/xen/xenbus/xenbus_xs.c
> @@ -140,7 +140,7 @@ void xs_request_exit(struct xb_req_data *req)
>         spin_lock(&xs_state_lock);
>         xs_state_users--;
>         if ((req->type == XS_TRANSACTION_START && req->msg.type ==
> XS_ERROR) ||
> -           req->msg.type == XS_TRANSACTION_END)
> +           req->type == XS_TRANSACTION_END)
>                 xs_state_users--;
>         spin_unlock(&xs_state_lock);
> 
> 
> I ran a few tests on dumpdata and they completed successfully. I'll keep
> this for the overnight runs too, with a different Xen version.

Thanks.


Juergen

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web