Re: [PATCH 01/16] PCI/P2PDMA: Pass gfp_mask flags to upstream_bridge_distance_warn()

[John Hubbard]

On 4/8/21 10:01 AM, Logan Gunthorpe wrote:
> In order to call upstream_bridge_distance_warn() from a dma_map function,
> it must not sleep. The only reason it does sleep is to allocate the seqbuf
> to print which devices are within the ACS path.
>
> Switch the kmalloc call to use a passed in gfp_mask and don't print that
> message if the buffer fails to be allocated.
>
> Signed-off-by: Logan Gunthorpe <logang@xxxxxxxxxxxx>
> Acked-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> ---
>   drivers/pci/p2pdma.c | 21 +++++++++++----------
>   1 file changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
> index 196382630363..bd89437faf06 100644
> --- a/drivers/pci/p2pdma.c
> +++ b/drivers/pci/p2pdma.c
> @@ -267,7 +267,7 @@ static int pci_bridge_has_acs_redir(struct pci_dev *pdev)
>   
>   static void seq_buf_print_bus_devfn(struct seq_buf *buf, struct pci_dev *pdev)
>   {
> -	if (!buf)
> +	if (!buf || !buf->buffer)

This is not great, sort of from an overall design point of view, even though
it makes the rest of the patch work. See below for other ideas, that will
avoid the need for this sort of odd point fix.

>   		return;
>   
>   	seq_buf_printf(buf, "%s;", pci_name(pdev));
> @@ -495,25 +495,26 @@ upstream_bridge_distance(struct pci_dev *provider, struct pci_dev *client,
>   
>   static enum pci_p2pdma_map_type
>   upstream_bridge_distance_warn(struct pci_dev *provider, struct pci_dev *client,
> -			      int *dist)
> +			      int *dist, gfp_t gfp_mask)
>   {
>   	struct seq_buf acs_list;
>   	bool acs_redirects;
>   	int ret;
>   
> -	seq_buf_init(&acs_list, kmalloc(PAGE_SIZE, GFP_KERNEL), PAGE_SIZE);
> -	if (!acs_list.buffer)
> -		return -ENOMEM;

Another odd thing: this used to check for memory failure and just give
up, and now it doesn't. Yes, I realize that it all still works at the
moment, but this is quirky and we shouldn't stop here.

Instead, a cleaner approach would be to push the memory allocation
slightly higher up the call stack, out to the
pci_p2pdma_distance_many(). So pci_p2pdma_distance_many() should make
the kmalloc() call, and fail out if it can't get a page for the seq_buf
buffer. Then you don't have to do all this odd stuff.

Furthermore, the call sites can then decide for themselves which GFP
flags, GFP_ATOMIC or GFP_KERNEL or whatever they want for kmalloc().

A related thing: this whole exercise would go better if there were a
preparatory patch or two that changed the return codes in this file to
something less crazy. There are too many functions that can fail, but
are treated as if they sort-of-mostly-would-never-fail, in the hopes of
using the return value directly for counting and such. This is badly
mistaken, and it leads developers to try to avoid returning -ENOMEM
(which is what we need here).

Really, these functions should all be doing "0 for success, -ERRNO for
failure, and pass other values, including results, in the arg list".


> +	seq_buf_init(&acs_list, kmalloc(PAGE_SIZE, gfp_mask), PAGE_SIZE);
>   
>   	ret = upstream_bridge_distance(provider, client, dist, &acs_redirects,
>   				       &acs_list);
>   	if (acs_redirects) {
>   		pci_warn(client, "ACS redirect is set between the client and provider (%s)\n",
>   			 pci_name(provider));
> -		/* Drop final semicolon */
> -		acs_list.buffer[acs_list.len-1] = 0;
> -		pci_warn(client, "to disable ACS redirect for this path, add the kernel parameter: pci=disable_acs_redir=%s\n",
> -			 acs_list.buffer);
> +
> +		if (acs_list.buffer) {
> +			/* Drop final semicolon */
> +			acs_list.buffer[acs_list.len - 1] = 0;
> +			pci_warn(client, "to disable ACS redirect for this path, add the kernel parameter: pci=disable_acs_redir=%s\n",
> +				 acs_list.buffer);
> +		}
>   	}
>   
>   	if (ret == PCI_P2PDMA_MAP_NOT_SUPPORTED) {
> @@ -566,7 +567,7 @@ int pci_p2pdma_distance_many(struct pci_dev *provider, struct device **clients,
>   
>   		if (verbose)
>   			ret = upstream_bridge_distance_warn(provider,
> -					pci_client, &distance);
> +					pci_client, &distance, GFP_KERNEL);
>   		else
>   			ret = upstream_bridge_distance(provider, pci_client,
>   						       &distance, NULL, NULL);

thanks,
--
John Hubbard
NVIDIA