[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH] vpci: introduce per-domain lock to protect vpci structure
On 14.02.2022 15:26, Oleksandr Andrushchenko wrote: > > > On 14.02.22 16:19, Jan Beulich wrote: >> On 09.02.2022 14:36, Oleksandr Andrushchenko wrote: >>> @@ -410,14 +428,37 @@ static void vpci_write_helper(const struct pci_dev >>> *pdev, >>> r->private); >>> } >>> >>> +static bool vpci_header_write_lock(const struct pci_dev *pdev, >>> + unsigned int start, unsigned int size) >>> +{ >>> + /* >>> + * Writing the command register and ROM BAR register may trigger >>> + * modify_bars to run which in turn may access multiple pdevs while >>> + * checking for the existing BAR's overlap. The overlapping check, if >>> done >>> + * under the read lock, requires vpci->lock to be acquired on both >>> devices >>> + * being compared, which may produce a deadlock. It is not possible to >>> + * upgrade read lock to write lock in such a case. So, in order to >>> prevent >>> + * the deadlock, check which registers are going to be written and >>> acquire >>> + * the lock in the appropriate mode from the beginning. >>> + */ >>> + if ( !vpci_offset_cmp(start, size, PCI_COMMAND, 2) ) >>> + return true; >>> + >>> + if ( !vpci_offset_cmp(start, size, pdev->vpci->header.rom_reg, 4) ) >>> + return true; >>> + >>> + return false; >>> +} >> A function of this name gives (especially at the call site(s)) the >> impression of acquiring a lock. Considering that of the prefixes >> neither "vpci" nor "header" are really relevant here, may I suggest >> to use need_write_lock()? >> >> May I further suggest that you either split the comment or combine >> the two if()-s (perhaps even straight into single return statement)? >> Personally I'd prefer the single return statement approach here ... > That was already questioned by Roger and now it looks like: > > static bool overlap(unsigned int r1_offset, unsigned int r1_size, > unsigned int r2_offset, unsigned int r2_size) > { > /* Return true if there is an overlap. */ > return r1_offset < r2_offset + r2_size && r2_offset < r1_offset + > r1_size; > } > > bool vpci_header_write_lock(const struct pci_dev *pdev, > unsigned int start, unsigned int size) > { > /* > * Writing the command register and ROM BAR register may trigger > * modify_bars to run which in turn may access multiple pdevs while > * checking for the existing BAR's overlap. The overlapping check, if > done > * under the read lock, requires vpci->lock to be acquired on both > devices > * being compared, which may produce a deadlock. It is not possible to > * upgrade read lock to write lock in such a case. So, in order to > prevent > * the deadlock, check which registers are going to be written and > acquire > * the lock in the appropriate mode from the beginning. > */ > if ( overlap(start, size, PCI_COMMAND, 2) || > (pdev->vpci->header.rom_reg && > overlap(start, size, pdev->vpci->header.rom_reg, 4)) ) > return true; > > return false; > } > > vpci_header_write_lock moved to header.c and is not static anymore. > So, sitting in header.c, the name seems to be appropriate now The prefix of the name - yes. But as said, a function of this name looks as if it would acquire a lock. Imo you want to insert "need" or some such. Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |