| Message ID | 20210408170123.8788-2-logang@deltatee.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add new DMA mapping operation for P2PDMA | expand |
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@deltatee.com> > Acked-by: Bjorn Helgaas <bhelgaas@google.com> > --- > 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,
On 2021-05-01 9:58 p.m., John Hubbard wrote: > 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. I don't really agree with this assessment. If kmalloc fails to initialize the seq_buf() (which should be very rare), the only thing that is lost is the one warning print that tells the user the command line parameter needed disable the ACS. Everything else works fine, nothing else can fail. I don't see the need to add extra complexity just so the code errors out in no-mem instead of just skipping the one, slightly more informative, warning line. Also, keep in mind the result of all these functions are cached so it only ever happens once. So for this to matter, the user would have to do their first transaction between two devices exactly at the time memory allocations would fail. > 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). Hmm? Which functions can fail? and how? Logan
On 5/3/21 8:57 AM, Logan Gunthorpe wrote: > > > On 2021-05-01 9:58 p.m., John Hubbard wrote: >> 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. > > I don't really agree with this assessment. If kmalloc fails to > initialize the seq_buf() (which should be very rare), the only thing > that is lost is the one warning print that tells the user the command > line parameter needed disable the ACS. Everything else works fine, > nothing else can fail. I don't see the need to add extra complexity just > so the code errors out in no-mem instead of just skipping the one, > slightly more informative, warning line. That's the thing: memory failure should be exceedingly rare for this. Therefore, just fail out entirely (which I don't expect we'll likely ever see), instead of doing all this weird stuff to try to continue on if you cannot allocate a single page. If you are in that case, the system is not in a state that is going to run your dma p2p setup well anyway. I think it's *less* complexity to allocate up front, fail early if allocation fails, and then not have to deal with these really odd quirks at the lower levels. > > Also, keep in mind the result of all these functions are cached so it > only ever happens once. So for this to matter, the user would have to do > their first transaction between two devices exactly at the time memory > allocations would fail. > > >> 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). > > Hmm? Which functions can fail? and how? > Let's defer that to the other patches, I was sort of looking ahead to those, sorry. thanks,
On 2021-05-03 12:17 p.m., John Hubbard wrote: > On 5/3/21 8:57 AM, Logan Gunthorpe wrote: >> >> >> On 2021-05-01 9:58 p.m., John Hubbard wrote: >>> 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. >> >> I don't really agree with this assessment. If kmalloc fails to >> initialize the seq_buf() (which should be very rare), the only thing >> that is lost is the one warning print that tells the user the command >> line parameter needed disable the ACS. Everything else works fine, >> nothing else can fail. I don't see the need to add extra complexity just >> so the code errors out in no-mem instead of just skipping the one, >> slightly more informative, warning line. > > That's the thing: memory failure should be exceedingly rare for this. > Therefore, just fail out entirely (which I don't expect we'll likely > ever see), instead of doing all this weird stuff to try to continue > on if you cannot allocate a single page. If you are in that case, the > system is not in a state that is going to run your dma p2p setup well > anyway. > > I think it's *less* complexity to allocate up front, fail early if > allocation fails, and then not have to deal with these really odd > quirks at the lower levels. > I don't see how it's all that weird. We're skipping a warning if we can't allocate memory to calculate part of the message. It's really not necessary. If the memory really can't be allocated then something else will fail, but we really don't need to fail here because we couldn't print a verbose warning message. Logan
On 5/3/21 11:20 AM, Logan Gunthorpe wrote: ... >> That's the thing: memory failure should be exceedingly rare for this. >> Therefore, just fail out entirely (which I don't expect we'll likely >> ever see), instead of doing all this weird stuff to try to continue >> on if you cannot allocate a single page. If you are in that case, the >> system is not in a state that is going to run your dma p2p setup well >> anyway. >> >> I think it's *less* complexity to allocate up front, fail early if >> allocation fails, and then not have to deal with these really odd >> quirks at the lower levels. >> > > I don't see how it's all that weird. We're skipping a warning if we > can't allocate memory to calculate part of the message. It's really not > necessary. If the memory really can't be allocated then something else > will fail, but we really don't need to fail here because we couldn't > print a verbose warning message. > Well, I really dislike the result we have in this particular patch, but I won't stand in the way of progress if that's how you really are going to do it. thanks,
On Mon, May 03, 2021 at 11:17:31AM -0700, John Hubbard wrote: > That's the thing: memory failure should be exceedingly rare for this. > Therefore, just fail out entirely (which I don't expect we'll likely > ever see), instead of doing all this weird stuff to try to continue > on if you cannot allocate a single page. If you are in that case, the > system is not in a state that is going to run your dma p2p setup well > anyway. > > I think it's *less* complexity to allocate up front, fail early if > allocation fails, and then not have to deal with these really odd > quirks at the lower levels. Agreed.
On 5/1/21 11:58 PM, John Hubbard wrote: > 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@deltatee.com> >> Acked-by: Bjorn Helgaas <bhelgaas@google.com> >> --- >> 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. > +1. In fact, I didn't see how the kmalloc was changed... you refactored the code to pass-in the GFP_KERNEL that was originally hard-coded into upstream_bridge_distance_warn(); I don't see how that avoided the kmalloc() call. in fact, I also see you lost a failed kmalloc() check, so it seems to have taken a step back. >> 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(). > agree, good proposal to avoid a sleep due to 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". > WFM! > >> + 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,
On 2021-05-11 10:05 a.m., Don Dutile wrote: > On 5/1/21 11:58 PM, John Hubbard wrote: >> 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@deltatee.com> >>> Acked-by: Bjorn Helgaas <bhelgaas@google.com> >>> --- >>> 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. >> > +1. > In fact, I didn't see how the kmalloc was changed... you refactored the code to pass-in the > GFP_KERNEL that was originally hard-coded into upstream_bridge_distance_warn(); > I don't see how that avoided the kmalloc() call. > in fact, I also see you lost a failed kmalloc() check, so it seems to have taken a step back. I've changed this in v2 to just use some memory allocated on the stack. Avoids this argument all together. Logan
On 5/11/21 12:12 PM, Logan Gunthorpe wrote: > > On 2021-05-11 10:05 a.m., Don Dutile wrote: >> On 5/1/21 11:58 PM, John Hubbard wrote: >>> 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@deltatee.com> >>>> Acked-by: Bjorn Helgaas <bhelgaas@google.com> >>>> --- >>>> 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. >>> >> +1. >> In fact, I didn't see how the kmalloc was changed... you refactored the code to pass-in the >> GFP_KERNEL that was originally hard-coded into upstream_bridge_distance_warn(); >> I don't see how that avoided the kmalloc() call. >> in fact, I also see you lost a failed kmalloc() check, so it seems to have taken a step back. > I've changed this in v2 to just use some memory allocated on the stack. > Avoids this argument all together. > > Logan > Looking fwd to the v2; again, my apologies for the delay, and the redundancy it's adding to your feedback review & changes. -Don
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) 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; + 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);