Re: [Xen-devel] [PATCH] pciback: deferred handling of pci configuration space accesses

[Ryan <hap9@xxxxxxxxxxxxxx>]

On Tue, 2006-04-25 at 15:09 +0100, Keir Fraser wrote:
> On 25 Apr 2006, at 14:32, Ryan wrote:
> 
> > I can't put the atomic_inc after I clear the _PCIF_active bit as then
> > the frontend could try and immediately turn around and send a second
> > legitimate request that would fail because op_in_progress may not have
> > decremented yet.
> 
> Okay, I think I see what you mean. But I think the code would be 
> clearer *and* robust against spurious interrupts (event-channel 
> callbacks) by doing the following:
> 
>   * turn op_in_progress into a proper boolean, but with type 'unsigned 
> long'.
> 
>   * in the IRQ handler do:
>       if (test_bit(_XEN_PCIF_active, &pdev->...) &&
>           !test_and_set_bit(0, &pdev->op_in_progress))
>          schedule_work(...);
> 
>   * at the very end of pciback_handle_event() do:
>       smp_wmb(); /* finish the response /then/ flag the IRQ handler */
>       pdev->in_progress = 0;
>       smp_mb(); /* flag the IRQ handler /then/ check for more work */
>       if (test_bit(_XEN_PCIF_active, &pdev->...) &&
>           !test_and_set_bit(0, &pdev->op_in_progress))
>           schedule_work(...);
> 
> I think this is better than the current implementation of 
> op_in_progress as a counter with zero meaning in-progress, giving clear 
> one-shot-setting semantics for op_in_progress for each queued PCI 
> request.

I agree: setting/clearing a bit is definitely clearer than the atomic
type I was using. I've fixed that in the attached patch.

I also see how your method is an improvement over mine in that the
_XEN_PCIF_active flag isn't cleared when the backend is still
processing. I was willing to accept that as a consequence of the
frontend misbehaving but your method is even more robust and will
guarantee that the request will only run once and that the flag will be
set/cleared appropriately. I've fixed up my code and am attaching a
patch.

I also added a comment about the need to defer (to avoid sleeping in
atomic context).

--

Some of the Linux PCI functions called by the virtual configuration
space handlers were making calls into ACPI code which uses semaphores.
Since semaphores can not be locked while atomic (because they could
sleep), I changed the way the PCI backend responds to requests from the
frontend. Previously, the virtual configuration space handlers ran in
the same context as the event channel interrupt handler (which was often
atomic if not always atomic). Now the interrupt handler schedules a
callback function (a bottom half) in the system work queue (keventd)
that will get called in process context at a slightly later time. This
allows the handlers in the virtual configuration space to run in process
context and to call any core PCI function regardless of whether it will
sleep or not.


Signed-off-by: Ryan Wilson <hap9@xxxxxxxxxxxxxx>

Attachment: pciback_op_workqueue.patch
Description: Text Data

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel