Re: [PATCH v6 03/13] vpci: move lock outside of struct vpci

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 07.02.22 17:26, Jan Beulich wrote:
> On 07.02.2022 16:11, Oleksandr Andrushchenko wrote:
>>
>> On 07.02.22 16:35, Oleksandr Andrushchenko wrote:
>>> On 07.02.22 16:27, Roger Pau Monné wrote:
>>>> On Mon, Feb 07, 2022 at 03:11:03PM +0100, Jan Beulich wrote:
>>>>> On 07.02.2022 14:53, Oleksandr Andrushchenko wrote:
>>>>>> On 07.02.22 14:46, Roger Pau Monné wrote:
>>>>>>> I think the per-domain rwlock seems like a good option. I would do
>>>>>>> that as a pre-patch.
>>>>>> It is. But it seems it won't solve the thing we started this adventure 
>>>>>> for:
>>>>>>
>>>>>> With per-domain read lock and still ABBA in modify_bars (hope the below
>>>>>> is correctly seen with a monospace font):
>>>>>>
>>>>>> cpu0: vpci_write-> d->RLock -> pdev1->lock ->                            
>>>>>>                       rom_write -> modify_bars: tmp (pdev2) ->lock
>>>>>> cpu1:        vpci_write-> d->RLock pdev2->lock -> cmd_write -> 
>>>>>> modify_bars: tmp (pdev1) ->lock
>>>>>>
>>>>>> There is no API to upgrade read lock to write lock in modify_bars which 
>>>>>> could help,
>>>>>> so in both cases vpci_write should take write lock.
>>>>> Hmm, yes, I think you're right: It's not modify_bars() itself which needs
>>>>> to acquire the write lock, but its (perhaps indirect) caller. Effectively
>>>>> vpci_write() would need to take the write lock if the range written
>>>>> overlaps the BARs or the command register.
>>>> I'm confused. If we use a per-domain rwlock approach there would be no
>>>> need to lock tmp again in modify_bars, because we should hold the
>>>> rwlock in write mode, so there's no ABBA?
>>> this is only possible with what you wrote below:
>>>> We will have however to drop the per domain read and vpci locks and
>>>> pick the per-domain lock in write mode.
>>> I think this is going to be unreliable. We need a reliable way to
>>> upgrade read lock to write lock.
>>> Then, we can drop pdev->vpci_lock at all, because we are always
>>> protected with d->rwlock and those who want to free pdev->vpci
>>> will use write lock.
>>>
>>> So, per-domain rwlock with write upgrade implemented minus pdev->vpci
>>> should do the trick
>> Linux doesn't implement write upgrade and it seems for a reason [1]:
>> "Also, you cannot “upgrade” a read-lock to a write-lock, so if you at _any_ 
>> time
>> need to do any changes (even if you don’t do it every time), you have to get
>> the write-lock at the very beginning."
>>
>> So, I am not sure we can have the same for Xen...
>>
>> At the moment I see at least two possible ways to solve the issue:
>> 1. Make vpci_write use write lock, thus make all write accesses synchronized
>> for the given domain, read are fully parallel
> 1b. Make vpci_write use write lock for writes to command register and BARs
> only; keep using the read lock for all other writes.
I am not quite sure how to do that. Do you mean something like:
void vpci_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int size,
                 uint32_t data)
[snip]
     list_for_each_entry ( r, &pdev->vpci->handlers, node )
{
[snip]
     if ( r->needs_write_lock)
         write_lock(d->vpci_lock)
     else
         read_lock(d->vpci_lock)
....

And provide rw as an argument to:

int vpci_add_register(struct vpci *vpci, vpci_read_t *read_handler,
                       vpci_write_t *write_handler, unsigned int offset,
                       unsigned int size, void *data, --->>> bool write_path 
<<<-----)

Is this what you mean?

With the above, if we have d->vpci_lock, I think we can drop
pdev->vpci_lock at all

Thank you,
Oleksandr

P.S. I don't think you mean we just drop the read lock and acquire write lock
as it leads to the mentioned before unreliability.