[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v6 03/13] vpci: move lock outside of struct vpci
On 08.02.22 12:50, Roger Pau Monné wrote: > On Tue, Feb 08, 2022 at 07:35:34AM +0000, Oleksandr Andrushchenko wrote: >> >> On 07.02.22 18:44, Oleksandr Andrushchenko wrote: >>> On 07.02.22 18:37, Jan Beulich wrote: >>>> On 07.02.2022 17:21, Oleksandr Andrushchenko wrote: >>>>> On 07.02.22 18:15, Jan Beulich wrote: >>>>>> On 07.02.2022 17:07, Oleksandr Andrushchenko wrote: >>>>>>> On 07.02.22 17:26, Jan Beulich wrote: >>>>>>>> 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? >>>>>> This sounds overly complicated. You can derive locally in vpci_write(), >>>>>> from just its "reg" and "size" parameters, whether the lock needs taking >>>>>> in write mode. >>>>> Yes, I started writing a reply with that. So, the summary (ROM >>>>> position depends on header type): >>>>> if ( (reg == PCI_COMMAND) || (reg == ROM) ) >>>>> { >>>>> read PCI_COMMAND and see if memory or IO decoding are enabled. >>>>> if ( enabled ) >>>>> write_lock(d->vpci_lock) >>>>> else >>>>> read_lock(d->vpci_lock) >>>>> } >>>> Hmm, yes, you can actually get away without using "size", since both >>>> command register and ROM BAR are 32-bit aligned registers, and 64-bit >>>> accesses get split in vpci_ecam_write(). >>> But, OS may want reading a single byte of ROM BAR, so I think >>> I'll need to check if reg+size fall into PCI_COMAND and ROM BAR >>> ranges >>>> For the command register the memory- / IO-decoding-enabled check may >>>> end up a little more complicated, as the value to be written also >>>> matters. Maybe read the command register only for the ROM BAR write, >>>> using the write lock uniformly for all command register writes? >>> Sounds good for the start. >>> Another concern is that if we go with a read_lock and then in the >>> underlying code we disable memory decoding and try doing >>> something and calling cmd_write handler for any reason then.... >>> >>> I mean that the check in the vpci_write is somewhat we can tolerate, >>> but then it is must be considered that no code in the read path >>> is allowed to perform write path functions. Which brings a pretty >>> valid use-case: say in read mode we detect an unrecoverable error >>> and need to remove the device: >>> vpci_process_pending -> ERROR -> vpci_remove_device or similar. >>> >>> What do we do then? It is all going to be fragile... >> I have tried to summarize the options we have wrt locking >> and would love to hear from @Roger and @Jan. >> >> In every variant there is a task of dealing with the overlap >> detection in modify_bars, so this is the only place as of now >> which needs special treatment. >> >> Existing limitations: there is no way to upgrade a read lock to a write >> lock, so paths which may require write lock protection need to use >> write lock from the very beginning. Workarounds can be applied. >> >> 1. Per-domain rw lock, aka d->vpci_lock >> ============================================================== >> Note: with per-domain rw lock it is possible to do without introducing >> per-device locks, so pdev->vpci->lock can be removed and no pdev->vpci_lock >> should be required. > Er, no, I think you still need a per-device lock unless you intent to > take the per-domain rwlock in write mode every time you modify data > in vpci. This is exactly the assumption stated below. I am trying to discuss all the possible options, so this one is also listed > I still think you need pdev->vpci->lock. It's possible this > approach doesn't require moving the lock outside of the vpci struct. > >> This is only going to work in case if vpci_write always takes the write lock >> and vpci_read takes a read lock and no path in vpci_read is allowed to >> perform write path operations. > I think that's likely too strong? > > You could get away with both vpci_{read,write} only taking the read > lock and use a per-device vpci lock? But as discussed before: - if pdev->vpci_lock is used this still leads to ABBA - we should know about if to take the write lock beforehand > > Otherwise you are likely to introduce contention in msix_write if a > guest makes heavy use of the MSI-X entry mask bit. > >> vpci_process_pending uses write lock as it have vpci_remove_device in its >> error path. >> >> Pros: >> - no per-device vpci lock is needed? >> - solves overlap code ABBA in modify_bars >> >> Cons: >> - all writes are serialized >> - need to carefully select read paths, so they are guaranteed not to lead >> to lock upgrade use-cases >> >> 1.1. Semi read lock upgrade in modify bars >> -------------------------------------------------------------- >> In this case both vpci_read and vpci_write take a read lock and when it comes >> to modify_bars: >> >> 1. read_unlock(d->vpci_lock) >> 2. write_lock(d->vpci_lock) >> 3. Check that pdev->vpci is still available and is the same object: >> if (pdev->vpci && (pdev->vpci == old_vpci) ) >> { >> /* vpci structure is valid and can be used. */ >> } >> else >> { >> /* vpci has gone, return an error. */ >> } >> >> Pros: >> - no per-device vpci lock is needed? >> - solves overlap code ABBA in modify_bars >> - readers and writers are NOT serialized >> - NO need to carefully select read paths, so they are guaranteed not to lead >> to lock upgrade use-cases >> >> Cons: >> - ??? >> >> 2. per-device lock (pdev->vpci_lock) + d->overlap_chk_lock >> ============================================================== >> In order to solve overlap ABBA, we introduce a per-domain helper >> lock to protect the overlapping code in modify_bars: >> >> old_vpci = pdev->vpci; >> spin_unlock(pdev->vpci_lock); >> spin_lock(pdev->domain->overlap_chk_lock); > Since you drop the pdev lock you get a window here where either vpci > or even pdev itself could be removed under your feet, so using > pdev->vpci_lock like you do below could dereference a stale pdev. pdev is anyways not protected with pcidevs lock here, so even now it is possible to have pdev disapear in between. We do not use pcidevs_lock in MMIO handlers... > >> spin_lock(pdev->vpci_lock); >> if ( pdev->vpci && (pdev->vpci == old_vpci) ) >> for_each_pdev ( pdev->domain, tmp ) >> { >> if ( tmp != pdev ) >> { >> spin_lock(tmp->vpci_lock); >> if ( tmp->vpci ) >> ... >> } >> } >> >> Pros: >> - all accesses are independent, only the same device access is serialized >> - no need to care about readers and writers wrt read lock upgrade issues >> >> Cons: >> - helper spin lock >> >> 3. Move overlap detection into process pending >> ============================================================== >> There is a Roger's patch [1] which adds a possibility for >> vpci_process_pending >> to perform different tasks rather than just map/unmap. With this patch >> extended >> in a way that it can hold a request queue it is possible to delay execution >> of the overlap code until no pdev->vpci_lock is held, but before returning to >> a guest after vpci_{read|write} or similar. >> >> Pros: >> - no need to emulate read lock upgrade >> - fully parallel read/write >> - queue in the vpci_process_pending will later on be used by SR-IOV, >> so this is going to help the future code >> Cons: >> - ??? > Maybe? It's hard to devise how that would end up looking like, and > whether it won't still require such kind of double locking. We would > still need to prevent doing a rangeset_remove_range for the device we > are trying to setup the mapping for, at which point we still need to > lock the current device plus the device we are iterating against? > > Since the code in vpci_process_pending is always executed in guest > vCPU context requiring all guest vCPUs to be paused when doing a > device addition or removal would prevent devices from going away, but > we could still have issues with concurrent accesses from other vCPUs. Yes, I understand that this may not be easily done, but this is still an option, > >> 4. Re-write overlap detection code >> ============================================================== >> It is possible to re-write overlap detection code, so the information about >> the >> mapped/unmapped regions is not read from vpci->header->bars[i] of each >> device, >> but instead there is a per-domain structure which holds the regions and >> implements reference counting. >> >> Pros: >> - solves ABBA >> >> Cons: >> - very complex code is expected >> >> 5. You name it >> ============================================================== >> >> From all the above I would recommend we go with option 2 which seems to >> reliably >> solve ABBA and does not bring cons of the other approaches. > 6. per-domain rwlock + per-device vpci lock > > Introduce vpci_header_write_lock(start, {end, size}) helper: return > whether a range requires the per-domain lock in write mode. This will > only return true if the range overlaps with the BAR ROM or the command > register. > > In vpci_{read,write}: > > if ( vpci_header_write_lock(...) ) > /* Gain exclusive access to all of the domain pdevs vpci. */ > write_lock(d->vpci); > else > { > read_lock(d->vpci); > spin_lock(vpci->lock); > } > ... > > The vpci assign/deassign functions would need to be modified to write > lock the per-domain rwlock. The MSI-X table MMIO handler will also > need to read lock the per domain vpci lock. Ok, so it seems you are in favor of this implementation and I have no objection as well. The only limitation we should be aware of is that once a path has acquired the read lock it is not possible to do any write path operations in there. vpci_process_pending will acquire write lock though as it can lead to vpci_remove_device on its error path. So, I am going to implement pdev->vpci->lock + d->vpci_lock > > I think it's either something along the lines of my suggestion above, > or maybe option 3, albeit you would have to investigate how to > implement option 3. > > Thanks, Roger. @Roger, @Jan! Thank you!!
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |