xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
From: Jan Beulich <jbeulich@suse.com>
To: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Juergen Gross <jgross@suse.com>,
	Konrad Wilk <konrad.wilk@oracle.com>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH 2/3] xen-pciback: reconfigure also from backend watch handler
Date: Mon, 12 Apr 2021 11:44:20 +0200	[thread overview]
Message-ID: <529f18d5-89ae-596e-29a7-d773e5a3c6b2@suse.com> (raw)
In-Reply-To: <a93c66e5-807b-e557-d437-48d7f46f25f7@oracle.com>

On 09.04.2021 23:43, Boris Ostrovsky wrote:
> 
> On 4/7/21 10:37 AM, Jan Beulich wrote:
>> When multiple PCI devices get assigned to a guest right at boot, libxl
>> incrementally populates the backend tree. The writes for the first of
>> the devices trigger the backend watch. In turn xen_pcibk_setup_backend()
>> will set the XenBus state to Initialised, at which point no further
>> reconfigures would happen unless a device got hotplugged. Arrange for
>> reconfigure to also get triggered from the backend watch handler.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> Cc: stable@vger.kernel.org
>> ---
>> I will admit that this isn't entirely race-free (with the guest actually
>> attaching in parallel), but from the looks of it such a race ought to be
>> benign (not the least thanks to the mutex). Ideally the tool stack
>> wouldn't write num_devs until all devices had their information
>> populated. I tried doing so in libxl, but xen_pcibk_setup_backend()
>> calling xenbus_dev_fatal() when not being able to read that node
>> prohibits such an approach (or else libxl and driver changes would need
>> to go into use in lock-step).
>>
>> I wonder why what is being watched isn't just the num_devs node. Right
>> now the watch triggers quite frequently without anything relevant
>> actually having changed (I suppose in at least some cases in response
>> to writes by the backend itself).
>>
>> --- a/drivers/xen/xen-pciback/xenbus.c
>> +++ b/drivers/xen/xen-pciback/xenbus.c
>> @@ -359,7 +359,8 @@ out:
>>  	return err;
>>  }
>>  
>> -static int xen_pcibk_reconfigure(struct xen_pcibk_device *pdev)
>> +static int xen_pcibk_reconfigure(struct xen_pcibk_device *pdev,
>> +				 enum xenbus_state state)
>>  {
>>  	int err = 0;
>>  	int num_devs;
>> @@ -374,8 +375,7 @@ static int xen_pcibk_reconfigure(struct
>>  
>>  	mutex_lock(&pdev->dev_lock);
>>  	/* Make sure we only reconfigure once */
> 
> 
> Is this comment still valid or should it be moved ...
> 
> 
>> -	if (xenbus_read_driver_state(pdev->xdev->nodename) !=
>> -	    XenbusStateReconfiguring)
>> +	if (xenbus_read_driver_state(pdev->xdev->nodename) != state)
>>  		goto out;
>>  
>>  	err = xenbus_scanf(XBT_NIL, pdev->xdev->nodename, "num_devs", "%d",
>> @@ -500,6 +500,9 @@ static int xen_pcibk_reconfigure(struct
>>  		}
>>  	}
>>  
>> +	if (state != XenbusStateReconfiguring)
>> +		goto out;
>> +
> 
> 
> ... here?

Yeah, probably better to move it.

>>  	err = xenbus_switch_state(pdev->xdev, XenbusStateReconfigured);
>>  	if (err) {
>>  		xenbus_dev_fatal(pdev->xdev, err,
>> @@ -525,7 +528,7 @@ static void xen_pcibk_frontend_changed(s
>>  		break;
>>  
>>  	case XenbusStateReconfiguring:
>> -		xen_pcibk_reconfigure(pdev);
>> +		xen_pcibk_reconfigure(pdev, XenbusStateReconfiguring);
>>  		break;
>>  
>>  	case XenbusStateConnected:
>> @@ -664,6 +667,10 @@ static void xen_pcibk_be_watch(struct xe
>>  		xen_pcibk_setup_backend(pdev);
>>  		break;
>>  
>> +	case XenbusStateInitialised:
>> +		xen_pcibk_reconfigure(pdev, XenbusStateInitialised);
> 
> 
> Could you add a comment here that this is needed when multiple devices are added?

Sure.

> It also looks a bit odd that adding a device is now viewed as a reconfiguration. It seems to me that xen_pcibk_setup_backend() and xen_pcibk_reconfigure() really should be merged --- initialization code for both looks pretty much the same.

I thought the same, but didn't want to make more of a change than is
needed to address the problem at hand. Plus of course there's still
the possibility that someone may fix libxl, at which point the change
here (and hence the merging) would become unnecessary.

Jan


  reply	other threads:[~2021-04-12  9:44 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-07 14:35 [PATCH 0/3] xen-pciback: a fix, a workaround, and some simplification Jan Beulich
2021-04-07 14:37 ` [PATCH 1/3] xen-pciback: redo VF placement in the virtual topology Jan Beulich
2021-04-08 22:28   ` Boris Ostrovsky
2021-04-09  8:16     ` Jan Beulich
2021-04-07 14:37 ` [PATCH 2/3] xen-pciback: reconfigure also from backend watch handler Jan Beulich
2021-04-09 21:43   ` Boris Ostrovsky
2021-04-12  9:44     ` Jan Beulich [this message]
2021-04-12 15:55       ` Boris Ostrovsky
2021-04-07 14:37 ` [PATCH 3/3] xen-pciback: simplify vpci's find hook Jan Beulich
2021-04-09 21:45   ` Boris Ostrovsky
2021-04-12  9:50     ` Jan Beulich
2021-04-12 16:05       ` Boris Ostrovsky
2021-04-13  8:09         ` Jan Beulich
2021-04-13 12:54           ` Boris Ostrovsky
2021-04-23  8:05   ` Juergen Gross

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=529f18d5-89ae-596e-29a7-d773e5a3c6b2@suse.com \
    --to=jbeulich@suse.com \
    --cc=boris.ostrovsky@oracle.com \
    --cc=jgross@suse.com \
    --cc=konrad.wilk@oracle.com \
    --cc=xen-devel@lists.xenproject.org \
    --subject='Re: [PATCH 2/3] xen-pciback: reconfigure also from backend watch handler' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).