lkml.org 
[lkml]   [2021]   [May]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH 02/16] PCI/P2PDMA: Avoid pci_get_slot() which sleeps
    From
    Date
    On 5/3/21 9:08 AM, Logan Gunthorpe wrote:
    ...
    >> By the way, pre-existing code comment: pci_p2pdma_whitelist[] seems
    >> really short. From a naive point of view, I'd expect that there must be
    >> a lot more CPUs/chipsets that can do pci p2p, what do you think? I
    >> wonder if we have to be so super strict, anyway. It just seems extremely
    >> limited, and I suspect there will be some additions to the list as soon
    >> as we start to use this.
    >
    > Yes, well unfortunately we have no other way to determine what host
    > bridges can communicate with P2P. We settled on a whitelist when the
    > series was first patch. Nobody likes that situation, but nobody has
    > found anything better. We've been hoping standards bodies would give us
    > a flag but I haven't heard anything about that. At least AMD has been
    > able to guarantee us that all CPUs newer than Zen will support so that
    > covers a large swath. It would be nice if we could say something similar
    > for Intel.

    Thanks for explaining the situation!

    >
    >> OK, yes this avoids taking the pci_bus_sem, but it's kind of cheating.
    >> Why is it OK to avoid taking any locks in order to retrieve the
    >> first entry from the list, but in order to retrieve any other entry, you
    >> have to aquire the pci_bus_sem, and get a reference as well? Something
    >> is inconsistent there.
    >>
    >> The new version here also no longer takes a reference on the device,
    >> which is also cheating. But I'm guessing that the unstated assumption
    >> here is that there is always at least one entry in the list. But if
    >> that's true, then it's better to show clearly that assumption, instead
    >> of hiding it in an implicit call that skips both locking and reference
    >> counting.
    >
    > Because we hold a reference to a child device of the bus. So the host
    > bus device can't go away until the child device has been released. An
    > earlier version of the P2PDMA patchset had a lot more extraneous get
    > device calls until someone else pointed this out.
    >
    >> You could add a new function, which is a cut-down version of pci_get_slot(),
    >> like this, and call this from __host_bridge_whitelist():
    >>
    >> /*
    >> * A special purpose variant of pci_get_slot() that doesn't take the pci_bus_sem
    >> * lock, and only looks for the 00.0 bus-device-function. Once the PCI bus is
    >> * up, it is safe to call this, because there will always be a top-level PCI
    >> * root device.
    >> *
    >> * Other assumptions: the root device is the first device in the list, and the
    >> * root device is numbered 00.0.
    >> */
    >> struct pci_dev *pci_get_root_slot(struct pci_bus *bus)
    >> {
    >> struct pci_dev *root;
    >> unsigned devfn = PCI_DEVFN(0, 0);
    >>
    >> root = list_first_entry_or_null(&bus->devices, struct pci_dev,
    >> bus_list);
    >> if (root->devfn == devfn)
    >> goto out;
    >>
    >> root = NULL;
    >> out:
    >> pci_dev_get(root);
    >> return root;
    >> }
    >> EXPORT_SYMBOL(pci_get_root_slot);
    >>
    >> ...I think that's a lot clearer to the reader, about what's going on here.
    >
    > Per above, I think the reference count is unnecessary. But I could wrap
    > it in a static function for clarity. (There's no reason to export this
    > function).
    >

    Yes, please.


    thanks,
    --
    John Hubbard
    NVIDIA

    \
     
     \ /
      Last update: 2021-05-03 20:20    [W:4.291 / U:0.244 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site