[PATCH] libxl/PCI: defer backend wait upon attaching to PV guest

[PATCH] libxl/PCI: defer backend wait upon attaching to PV guest

[Jan Beulich]

Attempting to wait when the backend hasn't been created yet can't work:
the function will complain "Backend ... does not exist". Move the
waiting past the creation of the backend (and that of other related
nodes), hoping that there are no other dependencies that would now be
broken.

Fixes: 0fdb48ffe7a1 ("libxl: Make sure devices added by pci-attach are reflected in the config")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Just to make it explicit: I have no idea why the waiting is needed in
the first place. It's been there from the very introduction of PCI
passthrough support (commit b0a1af61678b). I therefore can't exclude
that an even better fix would be to simply omit the 2nd hunk here.

--- a/tools/libs/light/libxl_pci.c
+++ b/tools/libs/light/libxl_pci.c
@@ -157,11 +157,6 @@ static int libxl__device_pci_add_xenstor
     if (domtype == LIBXL_DOMAIN_TYPE_INVALID)
         return ERROR_FAIL;
 
-    if (!starting && domtype == LIBXL_DOMAIN_TYPE_PV) {
-        if (libxl__wait_for_backend(gc, be_path, GCSPRINTF("%d", XenbusStateConnected)) < 0)
-            return ERROR_FAIL;
-    }
-
     back = flexarray_make(gc, 16, 1);
 
     LOGD(DEBUG, domid, "Adding new pci device to xenstore");
@@ -213,6 +208,9 @@ static int libxl__device_pci_add_xenstor
         if (rc < 0) goto out;
     }
 
+    if (!starting && domtype == LIBXL_DOMAIN_TYPE_PV)
+        rc = libxl__wait_for_backend(gc, be_path, GCSPRINTF("%d", XenbusStateConnected));
+
 out:
     libxl__xs_transaction_abort(gc, &t);
     if (lock) libxl__unlock_file(lock);

Re: [PATCH] libxl/PCI: defer backend wait upon attaching to PV guest

[Jason Andryuk]

On Tue, Dec 14, 2021 at 2:50 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> Attempting to wait when the backend hasn't been created yet can't work:
> the function will complain "Backend ... does not exist". Move the
> waiting past the creation of the backend (and that of other related
> nodes), hoping that there are no other dependencies that would now be
> broken.
>
> Fixes: 0fdb48ffe7a1 ("libxl: Make sure devices added by pci-attach are reflected in the config")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Just to make it explicit: I have no idea why the waiting is needed in
> the first place. It's been there from the very introduction of PCI
> passthrough support (commit b0a1af61678b). I therefore can't exclude
> that an even better fix would be to simply omit the 2nd hunk here.

The first time a device is attached, the backend does not exist, and
the wait is not needed.  However, when a second device is attached,
the backend does exist.  Since pciback goes through Reconfiguring and
Reconfigured, I believe the wait exists to let the frontend/backend
settle back to Connected before modifying the xenstore entries to add
the additional device.  I could be wrong, but that is my best answer
for why someone went to the trouble of adding a wait in the first
place.

Prior to 0fdb48ffe7a1, the backend was created before the watch:
     num_devs = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/num_devs", be_path));
-    if (!num_devs)
-        return libxl__create_pci_backend(gc, domid, pci, 1);

     libxl_domain_type domtype = libxl__domain_type(gc, domid);
     if (domtype == LIBXL_DOMAIN_TYPE_INVALID)
         return ERROR_FAIL;

     if (!starting && domtype == LIBXL_DOMAIN_TYPE_PV) {
         if (libxl__wait_for_backend(gc, be_path, GCSPRINTF("%d",
XenbusStateConnected)) < 0)
             return ERROR_FAIL;
     }

Here and elsewhere, num_devs has been used to identify pre-existing
backends.  That's why I went with the following to address this:
-    if (!starting && domtype == LIBXL_DOMAIN_TYPE_PV) {
-        if (libxl__wait_for_backend(gc, be_path, GCSPRINTF("%d",
XenbusStateConnected)) < 0)
+    /* wait is only needed if the backend already exists (num_devs != NULL) */
+    if (num_devs && !starting && domtype == LIBXL_DOMAIN_TYPE_PV) {
+        if (libxl__wait_for_backend(gc, be_path,
+                                    GCSPRINTF("%d", XenbusStateConnected)) < 0)

Regards,
Jason

> --- a/tools/libs/light/libxl_pci.c
> +++ b/tools/libs/light/libxl_pci.c
> @@ -157,11 +157,6 @@ static int libxl__device_pci_add_xenstor
>      if (domtype == LIBXL_DOMAIN_TYPE_INVALID)
>          return ERROR_FAIL;
>
> -    if (!starting && domtype == LIBXL_DOMAIN_TYPE_PV) {
> -        if (libxl__wait_for_backend(gc, be_path, GCSPRINTF("%d", XenbusStateConnected)) < 0)
> -            return ERROR_FAIL;
> -    }
> -
>      back = flexarray_make(gc, 16, 1);
>
>      LOGD(DEBUG, domid, "Adding new pci device to xenstore");
> @@ -213,6 +208,9 @@ static int libxl__device_pci_add_xenstor
>          if (rc < 0) goto out;
>      }
>
> +    if (!starting && domtype == LIBXL_DOMAIN_TYPE_PV)
> +        rc = libxl__wait_for_backend(gc, be_path, GCSPRINTF("%d", XenbusStateConnected));
> +
>  out:
>      libxl__xs_transaction_abort(gc, &t);
>      if (lock) libxl__unlock_file(lock);
>
>

Re: [PATCH] libxl/PCI: defer backend wait upon attaching to PV guest

[Jan Beulich]

On 14.12.2021 14:34, Jason Andryuk wrote:
> On Tue, Dec 14, 2021 at 2:50 AM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> Attempting to wait when the backend hasn't been created yet can't work:
>> the function will complain "Backend ... does not exist". Move the
>> waiting past the creation of the backend (and that of other related
>> nodes), hoping that there are no other dependencies that would now be
>> broken.
>>
>> Fixes: 0fdb48ffe7a1 ("libxl: Make sure devices added by pci-attach are reflected in the config")
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> Just to make it explicit: I have no idea why the waiting is needed in
>> the first place. It's been there from the very introduction of PCI
>> passthrough support (commit b0a1af61678b). I therefore can't exclude
>> that an even better fix would be to simply omit the 2nd hunk here.
> 
> The first time a device is attached, the backend does not exist, and
> the wait is not needed.  However, when a second device is attached,
> the backend does exist.  Since pciback goes through Reconfiguring and
> Reconfigured, I believe the wait exists to let the frontend/backend
> settle back to Connected before modifying the xenstore entries to add
> the additional device.  I could be wrong, but that is my best answer
> for why someone went to the trouble of adding a wait in the first
> place.

If things are as you describe them, then the change here is wrong: The
waiting gets moved from before the creation of the new device's nodes
to immediately after. Yet then I also can't see how else I should
address the issue at hand, so I'd have to defer to someone else; this
may involve undoing / redoing some of what the commit referenced by
the Fixes: tag did.

However, since all new nodes get added in a single transaction, I
can't see why waiting for the completion of a prior reconfigure would
be necessary: That'll either notice (and process) the new nodes, or
it won't. If it does, the next reconfigure would simply be a no-op.

Jan

Ping: [PATCH] libxl/PCI: defer backend wait upon attaching to PV guest

[Jan Beulich]

On 14.12.2021 08:49, Jan Beulich wrote:
> Attempting to wait when the backend hasn't been created yet can't work:
> the function will complain "Backend ... does not exist". Move the
> waiting past the creation of the backend (and that of other related
> nodes), hoping that there are no other dependencies that would now be
> broken.
> 
> Fixes: 0fdb48ffe7a1 ("libxl: Make sure devices added by pci-attach are reflected in the config")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Just to make it explicit: I have no idea why the waiting is needed in
> the first place. It's been there from the very introduction of PCI
> passthrough support (commit b0a1af61678b). I therefore can't exclude
> that an even better fix would be to simply omit the 2nd hunk here.

Anyone, be it an ack or an alternative?

Jan

> --- a/tools/libs/light/libxl_pci.c
> +++ b/tools/libs/light/libxl_pci.c
> @@ -157,11 +157,6 @@ static int libxl__device_pci_add_xenstor
>      if (domtype == LIBXL_DOMAIN_TYPE_INVALID)
>          return ERROR_FAIL;
>  
> -    if (!starting && domtype == LIBXL_DOMAIN_TYPE_PV) {
> -        if (libxl__wait_for_backend(gc, be_path, GCSPRINTF("%d", XenbusStateConnected)) < 0)
> -            return ERROR_FAIL;
> -    }
> -
>      back = flexarray_make(gc, 16, 1);
>  
>      LOGD(DEBUG, domid, "Adding new pci device to xenstore");
> @@ -213,6 +208,9 @@ static int libxl__device_pci_add_xenstor
>          if (rc < 0) goto out;
>      }
>  
> +    if (!starting && domtype == LIBXL_DOMAIN_TYPE_PV)
> +        rc = libxl__wait_for_backend(gc, be_path, GCSPRINTF("%d", XenbusStateConnected));
> +
>  out:
>      libxl__xs_transaction_abort(gc, &t);
>      if (lock) libxl__unlock_file(lock);
> 
> 

Re: Ping: [PATCH] libxl/PCI: defer backend wait upon attaching to PV guest

[Durrant, Paul]

On 04/01/2022 07:53, Jan Beulich wrote:
> On 14.12.2021 08:49, Jan Beulich wrote:
>> Attempting to wait when the backend hasn't been created yet can't work:
>> the function will complain "Backend ... does not exist". Move the
>> waiting past the creation of the backend (and that of other related
>> nodes), hoping that there are no other dependencies that would now be
>> broken.
>>
>> Fixes: 0fdb48ffe7a1 ("libxl: Make sure devices added by pci-attach are reflected in the config")
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> Just to make it explicit: I have no idea why the waiting is needed in
>> the first place. It's been there from the very introduction of PCI
>> passthrough support (commit b0a1af61678b). I therefore can't exclude
>> that an even better fix would be to simply omit the 2nd hunk here.
> 
> Anyone, be it an ack or an alternative?
> 

You can add my R-b FWIW, but of course I am not a maintainer.

   Paul

> Jan
> 
>> --- a/tools/libs/light/libxl_pci.c
>> +++ b/tools/libs/light/libxl_pci.c
>> @@ -157,11 +157,6 @@ static int libxl__device_pci_add_xenstor
>>       if (domtype == LIBXL_DOMAIN_TYPE_INVALID)
>>           return ERROR_FAIL;
>>   
>> -    if (!starting && domtype == LIBXL_DOMAIN_TYPE_PV) {
>> -        if (libxl__wait_for_backend(gc, be_path, GCSPRINTF("%d", XenbusStateConnected)) < 0)
>> -            return ERROR_FAIL;
>> -    }
>> -
>>       back = flexarray_make(gc, 16, 1);
>>   
>>       LOGD(DEBUG, domid, "Adding new pci device to xenstore");
>> @@ -213,6 +208,9 @@ static int libxl__device_pci_add_xenstor
>>           if (rc < 0) goto out;
>>       }
>>   
>> +    if (!starting && domtype == LIBXL_DOMAIN_TYPE_PV)
>> +        rc = libxl__wait_for_backend(gc, be_path, GCSPRINTF("%d", XenbusStateConnected));
>> +
>>   out:
>>       libxl__xs_transaction_abort(gc, &t);
>>       if (lock) libxl__unlock_file(lock);
>>
>>
> 

Re: [PATCH] libxl/PCI: defer backend wait upon attaching to PV guest

[Anthony PERARD]

On Tue, Dec 14, 2021 at 02:52:43PM +0100, Jan Beulich wrote:
> On 14.12.2021 14:34, Jason Andryuk wrote:
> > On Tue, Dec 14, 2021 at 2:50 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> Attempting to wait when the backend hasn't been created yet can't work:
> >> the function will complain "Backend ... does not exist". Move the
> >> waiting past the creation of the backend (and that of other related
> >> nodes), hoping that there are no other dependencies that would now be
> >> broken.
> >>
> >> Fixes: 0fdb48ffe7a1 ("libxl: Make sure devices added by pci-attach are reflected in the config")
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >> ---
> >> Just to make it explicit: I have no idea why the waiting is needed in
> >> the first place. It's been there from the very introduction of PCI
> >> passthrough support (commit b0a1af61678b). I therefore can't exclude
> >> that an even better fix would be to simply omit the 2nd hunk here.
> > 
> > The first time a device is attached, the backend does not exist, and
> > the wait is not needed.  However, when a second device is attached,
> > the backend does exist.  Since pciback goes through Reconfiguring and
> > Reconfigured, I believe the wait exists to let the frontend/backend
> > settle back to Connected before modifying the xenstore entries to add
> > the additional device.  I could be wrong, but that is my best answer
> > for why someone went to the trouble of adding a wait in the first
> > place.
> 
> If things are as you describe them, then the change here is wrong: The
> waiting gets moved from before the creation of the new device's nodes
> to immediately after. Yet then I also can't see how else I should
> address the issue at hand, so I'd have to defer to someone else; this
> may involve undoing / redoing some of what the commit referenced by
> the Fixes: tag did.
> 
> However, since all new nodes get added in a single transaction, I
> can't see why waiting for the completion of a prior reconfigure would
> be necessary: That'll either notice (and process) the new nodes, or
> it won't. If it does, the next reconfigure would simply be a no-op.

Well, the current code is checking that the backend is in a known state:
"Connected". Without this, the backend could be in any state like
"Closing" or other error, not just reconfiguring. We probably want to
keep checking that the backend can expect more devices.

Looking at Linux PCI PV backend implementation, I think linux reads
"num_devs", takes time to do configuration of new devs, then set "state"
to "reconfigured". So if libxl set's "num_devs" and "states" while
Linux takes time to config new devs, Linux will never check "num_devs"
again and ignore new added devices. So I guess it doesn't matter if we
wait before or after to read "state"=="connected".

There is no real documentation on this PV PCI passthrough, so it is hard
to tell what libxl can do. The pci backend xenstore path isn't even in
"xenstore-paths.pandoc".

But overall, maybe Jason's proposed change would be better, that is to
wait on the backend before adding a new device but only when there's
already a device which mean the backend would exist. (It would be better
to me as it doesn't change when the waiting is done.)

Thanks,

-- 
Anthony PERARD

Re: [PATCH] libxl/PCI: defer backend wait upon attaching to PV guest

[Jan Beulich]

On 07.01.2022 16:20, Anthony PERARD wrote:
> On Tue, Dec 14, 2021 at 02:52:43PM +0100, Jan Beulich wrote:
>> On 14.12.2021 14:34, Jason Andryuk wrote:
>>> On Tue, Dec 14, 2021 at 2:50 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>
>>>> Attempting to wait when the backend hasn't been created yet can't work:
>>>> the function will complain "Backend ... does not exist". Move the
>>>> waiting past the creation of the backend (and that of other related
>>>> nodes), hoping that there are no other dependencies that would now be
>>>> broken.
>>>>
>>>> Fixes: 0fdb48ffe7a1 ("libxl: Make sure devices added by pci-attach are reflected in the config")
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>> ---
>>>> Just to make it explicit: I have no idea why the waiting is needed in
>>>> the first place. It's been there from the very introduction of PCI
>>>> passthrough support (commit b0a1af61678b). I therefore can't exclude
>>>> that an even better fix would be to simply omit the 2nd hunk here.
>>>
>>> The first time a device is attached, the backend does not exist, and
>>> the wait is not needed.  However, when a second device is attached,
>>> the backend does exist.  Since pciback goes through Reconfiguring and
>>> Reconfigured, I believe the wait exists to let the frontend/backend
>>> settle back to Connected before modifying the xenstore entries to add
>>> the additional device.  I could be wrong, but that is my best answer
>>> for why someone went to the trouble of adding a wait in the first
>>> place.
>>
>> If things are as you describe them, then the change here is wrong: The
>> waiting gets moved from before the creation of the new device's nodes
>> to immediately after. Yet then I also can't see how else I should
>> address the issue at hand, so I'd have to defer to someone else; this
>> may involve undoing / redoing some of what the commit referenced by
>> the Fixes: tag did.
>>
>> However, since all new nodes get added in a single transaction, I
>> can't see why waiting for the completion of a prior reconfigure would
>> be necessary: That'll either notice (and process) the new nodes, or
>> it won't. If it does, the next reconfigure would simply be a no-op.
> 
> Well, the current code is checking that the backend is in a known state:
> "Connected". Without this, the backend could be in any state like
> "Closing" or other error, not just reconfiguring. We probably want to
> keep checking that the backend can expect more devices.

Perhaps; I wonder though whether that's enough. The backend may also not
expect (or successfully deal with) new devices for other reasons. IOW
some kind of check of the success of the "addition" would seem to be
needed anyway.

> Looking at Linux PCI PV backend implementation, I think linux reads
> "num_devs", takes time to do configuration of new devs, then set "state"
> to "reconfigured". So if libxl set's "num_devs" and "states" while
> Linux takes time to config new devs, Linux will never check "num_devs"
> again and ignore new added devices. So I guess it doesn't matter if we
> wait before or after to read "state"=="connected".
> 
> There is no real documentation on this PV PCI passthrough, so it is hard
> to tell what libxl can do. The pci backend xenstore path isn't even in
> "xenstore-paths.pandoc".
> 
> But overall, maybe Jason's proposed change would be better, that is to
> wait on the backend before adding a new device but only when there's
> already a device which mean the backend would exist. (It would be better
> to me as it doesn't change when the waiting is done.)

It's hard for me to tell without having seen Jason's full patch. I also
understand it has been submitted earlier than mine, so I wonder what its
status is.

Jan