diff mbox series

[03/16] PCI/P2PDMA: Attempt to set map_type if it has not been set

Message ID 20210408170123.8788-4-logang@deltatee.com (mailing list archive)
State New
Headers show
Series Add new DMA mapping operation for P2PDMA | expand

Commit Message

Logan Gunthorpe April 8, 2021, 5:01 p.m. UTC
Attempt to find the mapping type for P2PDMA pages on the first
DMA map attempt if it has not been done ahead of time.

Previously, the mapping type was expected to be calculated ahead of
time, but if pages are to come from userspace then there's no
way to ensure the path was checked ahead of time.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/pci/p2pdma.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

Comments

John Hubbard May 2, 2021, 7:58 p.m. UTC | #1
On 4/8/21 10:01 AM, Logan Gunthorpe wrote:
> Attempt to find the mapping type for P2PDMA pages on the first
> DMA map attempt if it has not been done ahead of time.
> 
> Previously, the mapping type was expected to be calculated ahead of
> time, but if pages are to come from userspace then there's no
> way to ensure the path was checked ahead of time.
> 
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
> ---
>   drivers/pci/p2pdma.c | 12 +++++++++---
>   1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
> index 473a08940fbc..2574a062a255 100644
> --- a/drivers/pci/p2pdma.c
> +++ b/drivers/pci/p2pdma.c
> @@ -825,11 +825,18 @@ EXPORT_SYMBOL_GPL(pci_p2pmem_publish);
>   static enum pci_p2pdma_map_type pci_p2pdma_map_type(struct pci_dev *provider,
>   						    struct pci_dev *client)
>   {
> +	enum pci_p2pdma_map_type ret;
> +
>   	if (!provider->p2pdma)
>   		return PCI_P2PDMA_MAP_NOT_SUPPORTED;
>   
> -	return xa_to_value(xa_load(&provider->p2pdma->map_types,
> -				   map_types_idx(client)));
> +	ret = xa_to_value(xa_load(&provider->p2pdma->map_types,
> +				  map_types_idx(client)));
> +	if (ret != PCI_P2PDMA_MAP_UNKNOWN)
> +		return ret;
> +
> +	return upstream_bridge_distance_warn(provider, client, NULL,
> +					     GFP_ATOMIC);

Returning a "bridge distance" from a "get map type" routine is jarring,
and I think it is because of a pre-existing problem: the above function
is severely misnamed. Let's try renaming it (and the other one) to
approximately:

     upstream_bridge_map_type_warn()
     upstream_bridge_map_type()

...and that should fix that. Well, that, plus tweaking the kernel doc
comments, which are also confused. I think someone started off thinking
about distances through PCIe, but in the end, the routine boils down to
just a few situations that are not distances at all.

Also, the above will read a little better if it is written like this:

	ret = xa_to_value(xa_load(&provider->p2pdma->map_types,
				  map_types_idx(client)));

	if (ret == PCI_P2PDMA_MAP_UNKNOWN)
		ret = upstream_bridge_map_type_warn(provider, client, NULL,
						    GFP_ATOMIC);
	
	return ret;


>   }
>   
>   static int __pci_p2pdma_map_sg(struct pci_p2pdma_pagemap *p2p_pgmap,
> @@ -877,7 +884,6 @@ int pci_p2pdma_map_sg_attrs(struct device *dev, struct scatterlist *sg,
>   	case PCI_P2PDMA_MAP_BUS_ADDR:
>   		return __pci_p2pdma_map_sg(p2p_pgmap, dev, sg, nents);
>   	default:
> -		WARN_ON_ONCE(1);

Why? Or at least, why, in this patch? It looks like an accidental
leftover from something, seeing as how it is not directly related to the
patch, and is not mentioned at all.


thanks,
Logan Gunthorpe May 3, 2021, 4:17 p.m. UTC | #2
On 2021-05-02 1:58 p.m., John Hubbard wrote:
> On 4/8/21 10:01 AM, Logan Gunthorpe wrote:
>> Attempt to find the mapping type for P2PDMA pages on the first
>> DMA map attempt if it has not been done ahead of time.
>>
>> Previously, the mapping type was expected to be calculated ahead of
>> time, but if pages are to come from userspace then there's no
>> way to ensure the path was checked ahead of time.
>>
>> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
>> ---
>>   drivers/pci/p2pdma.c | 12 +++++++++---
>>   1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
>> index 473a08940fbc..2574a062a255 100644
>> --- a/drivers/pci/p2pdma.c
>> +++ b/drivers/pci/p2pdma.c
>> @@ -825,11 +825,18 @@ EXPORT_SYMBOL_GPL(pci_p2pmem_publish);
>>   static enum pci_p2pdma_map_type pci_p2pdma_map_type(struct pci_dev *provider,
>>   						    struct pci_dev *client)
>>   {
>> +	enum pci_p2pdma_map_type ret;
>> +
>>   	if (!provider->p2pdma)
>>   		return PCI_P2PDMA_MAP_NOT_SUPPORTED;
>>   
>> -	return xa_to_value(xa_load(&provider->p2pdma->map_types,
>> -				   map_types_idx(client)));
>> +	ret = xa_to_value(xa_load(&provider->p2pdma->map_types,
>> +				  map_types_idx(client)));
>> +	if (ret != PCI_P2PDMA_MAP_UNKNOWN)
>> +		return ret;
>> +
>> +	return upstream_bridge_distance_warn(provider, client, NULL,
>> +					     GFP_ATOMIC);
> 
> Returning a "bridge distance" from a "get map type" routine is jarring,
> and I think it is because of a pre-existing problem: the above function
> is severely misnamed. Let's try renaming it (and the other one) to
> approximately:
> 
>      upstream_bridge_map_type_warn()
>      upstream_bridge_map_type()
> 
> ...and that should fix that. Well, that, plus tweaking the kernel doc
> comments, which are also confused. I think someone started off thinking
> about distances through PCIe, but in the end, the routine boils down to
> just a few situations that are not distances at all.
> 
> Also, the above will read a little better if it is written like this:
> 
> 	ret = xa_to_value(xa_load(&provider->p2pdma->map_types,
> 				  map_types_idx(client)));
> 
> 	if (ret == PCI_P2PDMA_MAP_UNKNOWN)
> 		ret = upstream_bridge_map_type_warn(provider, client, NULL,
> 						    GFP_ATOMIC);
> 	
> 	return ret;
> 
> 
>>   }

I agree that some of this has evolved in a way that some of the names
are a bit odd now. Could definitely use a cleanup, but that's not really
part of this series. When I have some time I can look at doing a cleanup
series to help with some of this.

>>   static int __pci_p2pdma_map_sg(struct pci_p2pdma_pagemap *p2p_pgmap,
>> @@ -877,7 +884,6 @@ int pci_p2pdma_map_sg_attrs(struct device *dev, struct scatterlist *sg,
>>   	case PCI_P2PDMA_MAP_BUS_ADDR:
>>   		return __pci_p2pdma_map_sg(p2p_pgmap, dev, sg, nents);
>>   	default:
>> -		WARN_ON_ONCE(1);
> 
> Why? Or at least, why, in this patch? It looks like an accidental
> leftover from something, seeing as how it is not directly related to the
> patch, and is not mentioned at all.

Before this patch, it was required that users of P2PDMA call
pci_p2pdma_distance_many() in some form before calling
pci_p2pdma_map_sg(). So, by convention, a usable map type had to already
be in the cache. The warning was there to yell at anyone who wrote code
that violated that convention.

This patch removes that convention and allows users to map P2PDMA pages
sight unseen and if the mapping type isn't in the cache, then it will
determine the mapping type at dma mapping time. Thus, the warning can be
removed and the function can fail normally if the mapping is unsupported.

Logan
John Hubbard May 3, 2021, 6:22 p.m. UTC | #3
On 5/3/21 9:17 AM, Logan Gunthorpe wrote:
>> Returning a "bridge distance" from a "get map type" routine is jarring,
>> and I think it is because of a pre-existing problem: the above function
>> is severely misnamed. Let's try renaming it (and the other one) to
>> approximately:
>>
>>       upstream_bridge_map_type_warn()
>>       upstream_bridge_map_type()
>>
>> ...and that should fix that. Well, that, plus tweaking the kernel doc
>> comments, which are also confused. I think someone started off thinking
>> about distances through PCIe, but in the end, the routine boils down to
>> just a few situations that are not distances at all.
>>
>> Also, the above will read a little better if it is written like this:
>>
>> 	ret = xa_to_value(xa_load(&provider->p2pdma->map_types,
>> 				  map_types_idx(client)));
>>
>> 	if (ret == PCI_P2PDMA_MAP_UNKNOWN)
>> 		ret = upstream_bridge_map_type_warn(provider, client, NULL,
>> 						    GFP_ATOMIC);
>> 	
>> 	return ret;
>>
>>
>>>    }
> 
> I agree that some of this has evolved in a way that some of the names
> are a bit odd now. Could definitely use a cleanup, but that's not really
> part of this series. When I have some time I can look at doing a cleanup
> series to help with some of this.

I'm OK with doing cleanup later. I just tend to call it out when I see it.

> 
>>>    static int __pci_p2pdma_map_sg(struct pci_p2pdma_pagemap *p2p_pgmap,
>>> @@ -877,7 +884,6 @@ int pci_p2pdma_map_sg_attrs(struct device *dev, struct scatterlist *sg,
>>>    	case PCI_P2PDMA_MAP_BUS_ADDR:
>>>    		return __pci_p2pdma_map_sg(p2p_pgmap, dev, sg, nents);
>>>    	default:
>>> -		WARN_ON_ONCE(1);
>>
>> Why? Or at least, why, in this patch? It looks like an accidental
>> leftover from something, seeing as how it is not directly related to the
>> patch, and is not mentioned at all.
> 
> Before this patch, it was required that users of P2PDMA call
> pci_p2pdma_distance_many() in some form before calling
> pci_p2pdma_map_sg(). So, by convention, a usable map type had to already
> be in the cache. The warning was there to yell at anyone who wrote code
> that violated that convention.
> 
> This patch removes that convention and allows users to map P2PDMA pages
> sight unseen and if the mapping type isn't in the cache, then it will
> determine the mapping type at dma mapping time. Thus, the warning can be
> removed and the function can fail normally if the mapping is unsupported.
> 

Let's add some of those words to the commit description, perhaps, it's nice
to have. Obviously a minor point though.

thanks,
Christoph Hellwig May 3, 2021, 6:35 p.m. UTC | #4
On Mon, May 03, 2021 at 10:17:59AM -0600, Logan Gunthorpe wrote:
> I agree that some of this has evolved in a way that some of the names
> are a bit odd now. Could definitely use a cleanup, but that's not really
> part of this series. When I have some time I can look at doing a cleanup
> series to help with some of this.

I think adding it to the series would be very helpful.
Logan Gunthorpe May 3, 2021, 6:46 p.m. UTC | #5
On 2021-05-03 12:35 p.m., Christoph Hellwig wrote:
> On Mon, May 03, 2021 at 10:17:59AM -0600, Logan Gunthorpe wrote:
>> I agree that some of this has evolved in a way that some of the names
>> are a bit odd now. Could definitely use a cleanup, but that's not really
>> part of this series. When I have some time I can look at doing a cleanup
>> series to help with some of this.
> 
> I think adding it to the series would be very helpful.

Ok, I'll prepend a handful of cleanup patches.

Logan
diff mbox series

Patch

diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
index 473a08940fbc..2574a062a255 100644
--- a/drivers/pci/p2pdma.c
+++ b/drivers/pci/p2pdma.c
@@ -825,11 +825,18 @@  EXPORT_SYMBOL_GPL(pci_p2pmem_publish);
 static enum pci_p2pdma_map_type pci_p2pdma_map_type(struct pci_dev *provider,
 						    struct pci_dev *client)
 {
+	enum pci_p2pdma_map_type ret;
+
 	if (!provider->p2pdma)
 		return PCI_P2PDMA_MAP_NOT_SUPPORTED;
 
-	return xa_to_value(xa_load(&provider->p2pdma->map_types,
-				   map_types_idx(client)));
+	ret = xa_to_value(xa_load(&provider->p2pdma->map_types,
+				  map_types_idx(client)));
+	if (ret != PCI_P2PDMA_MAP_UNKNOWN)
+		return ret;
+
+	return upstream_bridge_distance_warn(provider, client, NULL,
+					     GFP_ATOMIC);
 }
 
 static int __pci_p2pdma_map_sg(struct pci_p2pdma_pagemap *p2p_pgmap,
@@ -877,7 +884,6 @@  int pci_p2pdma_map_sg_attrs(struct device *dev, struct scatterlist *sg,
 	case PCI_P2PDMA_MAP_BUS_ADDR:
 		return __pci_p2pdma_map_sg(p2p_pgmap, dev, sg, nents);
 	default:
-		WARN_ON_ONCE(1);
 		return 0;
 	}
 }