| Message ID | 20210408170123.8788-5-logang@deltatee.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add new DMA mapping operation for P2PDMA | expand |
On 4/8/21 10:01 AM, Logan Gunthorpe wrote: > All callers of pci_p2pdma_map_type() have a struct dev_pgmap and a > struct device (of the client doing the DMA transfer). Thus move the > conversion to struct pci_devs for the provider and client into this > function. Actually, this is the wrong direction to go! All of these pre-existing pci_*() functions have a small problem already: they are dealing with struct device, instead of struct pci_dev. And so refactoring should be pushing the conversion to pci_dev *up* the calling stack, not lower as the patch here proposes. Also, there is no improvement in clarity by passing in (pgmap, dev) instead of the previous (provider, client). Now you have to do more type checking in the leaf function, which is another indication of a problem. Let's go that direction, please? Just convert to pci_dev much higher in the calling stack, and you'll find that everything fits together better. And it's OK to pass in extra params if that turns out to be necessary, after all. thanks,
On 2021-05-02 2:41 p.m., John Hubbard wrote: > On 4/8/21 10:01 AM, Logan Gunthorpe wrote: >> All callers of pci_p2pdma_map_type() have a struct dev_pgmap and a >> struct device (of the client doing the DMA transfer). Thus move the >> conversion to struct pci_devs for the provider and client into this >> function. > > Actually, this is the wrong direction to go! All of these pre-existing > pci_*() functions have a small problem already: they are dealing with > struct device, instead of struct pci_dev. And so refactoring should be > pushing the conversion to pci_dev *up* the calling stack, not lower as > the patch here proposes. > > Also, there is no improvement in clarity by passing in (pgmap, dev) > instead of the previous (provider, client). Now you have to do more type > checking in the leaf function, which is another indication of a problem. > > Let's go that direction, please? Just convert to pci_dev much higher in > the calling stack, and you'll find that everything fits together better. > And it's OK to pass in extra params if that turns out to be necessary, > after all. No, I disagree with this and it seems a bit confused. This change is allowing callers to call the function with what they have and doing more checks inside the called function. This allows for *less* checks in the leaf function, not more checks. (I mean, look at the patch itself, it puts a bunch of checks in both call sites into the callee and makes the code a lot cleaner -- it's removing more lines than it adds). Similar argument can be made with the pci_p2pdma_distance_many() (which I assume you are referring to). If the function took struct pci_dev instead of struct device, every caller would need to do all checks and conversions to struct pci_dev. That is not an improvement. Logan
On 5/3/21 9:30 AM, Logan Gunthorpe wrote: > > > On 2021-05-02 2:41 p.m., John Hubbard wrote: >> On 4/8/21 10:01 AM, Logan Gunthorpe wrote: >>> All callers of pci_p2pdma_map_type() have a struct dev_pgmap and a >>> struct device (of the client doing the DMA transfer). Thus move the >>> conversion to struct pci_devs for the provider and client into this >>> function. >> >> Actually, this is the wrong direction to go! All of these pre-existing >> pci_*() functions have a small problem already: they are dealing with >> struct device, instead of struct pci_dev. And so refactoring should be >> pushing the conversion to pci_dev *up* the calling stack, not lower as >> the patch here proposes. >> >> Also, there is no improvement in clarity by passing in (pgmap, dev) >> instead of the previous (provider, client). Now you have to do more type >> checking in the leaf function, which is another indication of a problem. >> >> Let's go that direction, please? Just convert to pci_dev much higher in >> the calling stack, and you'll find that everything fits together better. >> And it's OK to pass in extra params if that turns out to be necessary, >> after all. > > No, I disagree with this and it seems a bit confused. This change is I am not confused here, no. Other places, yes, but not at this moment. :) > allowing callers to call the function with what they have and doing more > checks inside the called function. This allows for *less* checks in the > leaf function, not more checks. (I mean, look at the patch itself, it > puts a bunch of checks in both call sites into the callee and makes the > code a lot cleaner -- it's removing more lines than it adds). > > Similar argument can be made with the pci_p2pdma_distance_many() (which > I assume you are referring to). If the function took struct pci_dev > instead of struct device, every caller would need to do all checks and > conversions to struct pci_dev. That is not an improvement. > IMHO, it is better to have all of the pci_*() functions dealing with pci_dev instead of dev, but it is also true that this is a larger change, so I won't press the point too hard right now. The reason I commented was that this refactoring goes in the opposite direction that I would be going in, if I were to start "improving" this part of the kernel, via refactoring. Anyway, I'll leave it alone. thanks,
On 2021-05-03 12:31 p.m., John Hubbard wrote: > On 5/3/21 9:30 AM, Logan Gunthorpe wrote: >> >> >> On 2021-05-02 2:41 p.m., John Hubbard wrote: >>> On 4/8/21 10:01 AM, Logan Gunthorpe wrote: >>>> All callers of pci_p2pdma_map_type() have a struct dev_pgmap and a >>>> struct device (of the client doing the DMA transfer). Thus move the >>>> conversion to struct pci_devs for the provider and client into this >>>> function. >>> >>> Actually, this is the wrong direction to go! All of these pre-existing >>> pci_*() functions have a small problem already: they are dealing with >>> struct device, instead of struct pci_dev. And so refactoring should be >>> pushing the conversion to pci_dev *up* the calling stack, not lower as >>> the patch here proposes. >>> >>> Also, there is no improvement in clarity by passing in (pgmap, dev) >>> instead of the previous (provider, client). Now you have to do more type >>> checking in the leaf function, which is another indication of a problem. >>> >>> Let's go that direction, please? Just convert to pci_dev much higher in >>> the calling stack, and you'll find that everything fits together better. >>> And it's OK to pass in extra params if that turns out to be necessary, >>> after all. >> >> No, I disagree with this and it seems a bit confused. This change is > > I am not confused here, no. Other places, yes, but not at this moment. :) I only meant confused because you suggested that such a change would reduce checks in the leaf functions when in fact it's the opposite. >> allowing callers to call the function with what they have and doing more >> checks inside the called function. This allows for *less* checks in the >> leaf function, not more checks. (I mean, look at the patch itself, it >> puts a bunch of checks in both call sites into the callee and makes the >> code a lot cleaner -- it's removing more lines than it adds). >> >> Similar argument can be made with the pci_p2pdma_distance_many() (which >> I assume you are referring to). If the function took struct pci_dev >> instead of struct device, every caller would need to do all checks and >> conversions to struct pci_dev. That is not an improvement. >> > > > IMHO, it is better to have all of the pci_*() functions dealing with pci_dev > instead of dev, but it is also true that this is a larger change, so I > won't press the point too hard right now. As a general rule, I'd agree with you. However, it's not good to blindly follow the rule when there might be good reasons to do it differently. In this case, the caller doesn't have PCI devices. The nvme fabrics code has a number of block devices and an RDMA device. It doesn't even know if these devices are backed by PCI devices and it doesn't have a direct path to obtain the pci_dev. Each struct device, might be turned into a pci_dev using the static function find_parent_pci_dev(). If any device is not a PCI device then we reject the P2PDMA transaction as not supported. Pushing the find_parent_pci_dev() logic into the callers is, IMO, just asking the callers to replicate a bunch of logic it shouldn't even be aware of creating messier code as a result. Logan
On 5/3/21 11:56 AM, Logan Gunthorpe wrote: ... >> IMHO, it is better to have all of the pci_*() functions dealing with pci_dev >> instead of dev, but it is also true that this is a larger change, so I >> won't press the point too hard right now. > > As a general rule, I'd agree with you. However, it's not good to blindly > follow the rule when there might be good reasons to do it differently. > > In this case, the caller doesn't have PCI devices. The nvme fabrics code > has a number of block devices and an RDMA device. It doesn't even know > if these devices are backed by PCI devices and it doesn't have a direct > path to obtain the pci_dev. > > Each struct device, might be turned into a pci_dev using the static > function find_parent_pci_dev(). If any device is not a PCI device then > we reject the P2PDMA transaction as not supported. Pushing the > find_parent_pci_dev() logic into the callers is, IMO, just asking the > callers to replicate a bunch of logic it shouldn't even be aware of > creating messier code as a result. > I guess my main concern here is that there are these pci*() functions that somehow want to pass around struct device. If a layer is carefully named throughout with pci in the function names, then something is still misaligned. This can happen over time, of course. But the really best patchsets try to avoid or mitigate the effect, by keeping names and functionality carefully aligned. Anyway, I've bugged you enough, I should just wait and see what the next round looks like, at this point. :) thanks,
On Mon, May 03, 2021 at 02:54:26PM -0700, John Hubbard wrote: > I guess my main concern here is that there are these pci*() functions > that somehow want to pass around struct device. Well, this is the main issue - helpers being used inside IOMMU code should not be called pci* functions. This is some generic device p2p interface that happens to only support PCI to PCI transfers today. Jason
On 5/3/21 3:57 PM, Jason Gunthorpe wrote: > On Mon, May 03, 2021 at 02:54:26PM -0700, John Hubbard wrote: > >> I guess my main concern here is that there are these pci*() functions >> that somehow want to pass around struct device. > > Well, this is the main issue - helpers being used inside IOMMU code > should not be called pci* functions. This is some generic device p2p > interface that happens to only support PCI to PCI transfers today. > Yes, maybe renaming a few levels of functions would help at least clarify what the code can do. Once the code reaches layers that truly are PCI-specific, that's where it should transition to using pci_dev args, and that's also where it should return -ENOTSUPP back up the calling stack. thanks,
diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c index 2574a062a255..bcb1a6d6119d 100644 --- a/drivers/pci/p2pdma.c +++ b/drivers/pci/p2pdma.c @@ -822,14 +822,21 @@ void pci_p2pmem_publish(struct pci_dev *pdev, bool publish) } EXPORT_SYMBOL_GPL(pci_p2pmem_publish); -static enum pci_p2pdma_map_type pci_p2pdma_map_type(struct pci_dev *provider, - struct pci_dev *client) +static enum pci_p2pdma_map_type pci_p2pdma_map_type(struct dev_pagemap *pgmap, + struct device *dev) { + struct pci_dev *provider = to_p2p_pgmap(pgmap)->provider; enum pci_p2pdma_map_type ret; + struct pci_dev *client; if (!provider->p2pdma) return PCI_P2PDMA_MAP_NOT_SUPPORTED; + if (!dev_is_pci(dev)) + return PCI_P2PDMA_MAP_NOT_SUPPORTED; + + client = to_pci_dev(dev); + ret = xa_to_value(xa_load(&provider->p2pdma->map_types, map_types_idx(client))); if (ret != PCI_P2PDMA_MAP_UNKNOWN) @@ -871,14 +878,8 @@ int pci_p2pdma_map_sg_attrs(struct device *dev, struct scatterlist *sg, { struct pci_p2pdma_pagemap *p2p_pgmap = to_p2p_pgmap(sg_page(sg)->pgmap); - struct pci_dev *client; - - if (WARN_ON_ONCE(!dev_is_pci(dev))) - return 0; - client = to_pci_dev(dev); - - switch (pci_p2pdma_map_type(p2p_pgmap->provider, client)) { + switch (pci_p2pdma_map_type(sg_page(sg)->pgmap, dev)) { case PCI_P2PDMA_MAP_THRU_HOST_BRIDGE: return dma_map_sg_attrs(dev, sg, nents, dir, attrs); case PCI_P2PDMA_MAP_BUS_ADDR: @@ -901,17 +902,9 @@ EXPORT_SYMBOL_GPL(pci_p2pdma_map_sg_attrs); void pci_p2pdma_unmap_sg_attrs(struct device *dev, struct scatterlist *sg, int nents, enum dma_data_direction dir, unsigned long attrs) { - struct pci_p2pdma_pagemap *p2p_pgmap = - to_p2p_pgmap(sg_page(sg)->pgmap); enum pci_p2pdma_map_type map_type; - struct pci_dev *client; - - if (WARN_ON_ONCE(!dev_is_pci(dev))) - return; - - client = to_pci_dev(dev); - map_type = pci_p2pdma_map_type(p2p_pgmap->provider, client); + map_type = pci_p2pdma_map_type(sg_page(sg)->pgmap, dev); if (map_type == PCI_P2PDMA_MAP_THRU_HOST_BRIDGE) dma_unmap_sg_attrs(dev, sg, nents, dir, attrs);
All callers of pci_p2pdma_map_type() have a struct dev_pgmap and a struct device (of the client doing the DMA transfer). Thus move the conversion to struct pci_devs for the provider and client into this function. Signed-off-by: Logan Gunthorpe <logang@deltatee.com> --- drivers/pci/p2pdma.c | 29 +++++++++++------------------ 1 file changed, 11 insertions(+), 18 deletions(-)