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

[Jan Beulich <jbeulich@xxxxxxxx>]

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