| Message ID | a9f7ca9a-38d5-12e2-7d15-ab026425e85a@cybernetics.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mpt3sas and dmapool scalability | expand |
On Thu, Aug 2, 2018 at 10:57 PM, Tony Battersby <tonyb@cybernetics.com> wrote: > Remove code duplication in error messages. It is now safe to pas a NULL > dev to dev_err(), so the checks to avoid doing so are no longer > necessary. > > Example: > > Error message with dev != NULL: > mpt3sas 0000:02:00.0: dma_pool_destroy chain pool, (____ptrval____) busy > > Same error message with dev == NULL before patch: > dma_pool_destroy chain pool, (____ptrval____) busy > > Same error message with dev == NULL after patch: > (NULL device *): dma_pool_destroy chain pool, (____ptrval____) busy Have you checked a history of this? I'm pretty sure this was created in an order to avoid bad looking (and in some cases frightening) "NULL device *" part. If it it's the case, I would rather leave it as is, and even not the case, I'm slightly more bent to the current state.
On 08/03/2018 04:56 AM, Andy Shevchenko wrote: > On Thu, Aug 2, 2018 at 10:57 PM, Tony Battersby <tonyb@cybernetics.com> wrote: >> Remove code duplication in error messages. It is now safe to pas a NULL >> dev to dev_err(), so the checks to avoid doing so are no longer >> necessary. >> >> Example: >> >> Error message with dev != NULL: >> mpt3sas 0000:02:00.0: dma_pool_destroy chain pool, (____ptrval____) busy >> >> Same error message with dev == NULL before patch: >> dma_pool_destroy chain pool, (____ptrval____) busy >> >> Same error message with dev == NULL after patch: >> (NULL device *): dma_pool_destroy chain pool, (____ptrval____) busy > Have you checked a history of this? > > I'm pretty sure this was created in an order to avoid bad looking (and > in some cases frightening) "NULL device *" part. > > If it it's the case, I would rather leave it as is, and even not the > case, I'm slightly more bent to the current state. > I did. "drivers/base/dmapool.c", later moved to "mm/dmapool.c", was added in linux-2.6.3, for which dev_err() did not work will a NULL dev, so the check was necessary back then. I agree that the (NULL device *): bit is ugly, but these messages should be printed only after a kernel bug, so it is not like they will be making a regular appearance in dmesg. Considering that, I think that it is better to keep it simple.
On 08/03/2018 09:41 AM, Tony Battersby wrote: > On 08/03/2018 04:56 AM, Andy Shevchenko wrote: >> On Thu, Aug 2, 2018 at 10:57 PM, Tony Battersby <tonyb@cybernetics.com> wrote: >>> Remove code duplication in error messages. It is now safe to pas a NULL >>> dev to dev_err(), so the checks to avoid doing so are no longer >>> necessary. >>> >>> Example: >>> >>> Error message with dev != NULL: >>> mpt3sas 0000:02:00.0: dma_pool_destroy chain pool, (____ptrval____) busy >>> >>> Same error message with dev == NULL before patch: >>> dma_pool_destroy chain pool, (____ptrval____) busy >>> >>> Same error message with dev == NULL after patch: >>> (NULL device *): dma_pool_destroy chain pool, (____ptrval____) busy >> Have you checked a history of this? >> >> I'm pretty sure this was created in an order to avoid bad looking (and >> in some cases frightening) "NULL device *" part. >> >> If it it's the case, I would rather leave it as is, and even not the >> case, I'm slightly more bent to the current state. >> > I did. "drivers/base/dmapool.c", later moved to "mm/dmapool.c", was > added in linux-2.6.3, for which dev_err() did not work will a NULL dev, > so the check was necessary back then. I agree that the (NULL device *): > bit is ugly, but these messages should be printed only after a kernel > bug, so it is not like they will be making a regular appearance in > dmesg. Considering that, I think that it is better to keep it simple. > My original unsubmitted patch used the following: +#define pool_err(pool, fmt, args...) \ + do { \ + if ((pool)->dev) \ + dev_err((pool)->dev, fmt, args); \ + else \ + pr_err(fmt, args); \ + } while (0) But then I decided to simplify it to just use dev_err(). I still have the old version. When I submit v3 of the patchset, which would you prefer?
On Fri, Aug 3, 2018 at 6:17 PM, Tony Battersby <tonyb@cybernetics.com> wrote: > On 08/03/2018 09:41 AM, Tony Battersby wrote: >> On 08/03/2018 04:56 AM, Andy Shevchenko wrote: >>> On Thu, Aug 2, 2018 at 10:57 PM, Tony Battersby <tonyb@cybernetics.com> wrote: >>>> Remove code duplication in error messages. It is now safe to pas a NULL >>>> dev to dev_err(), so the checks to avoid doing so are no longer >>>> necessary. >>>> >>>> Example: >>>> >>>> Error message with dev != NULL: >>>> mpt3sas 0000:02:00.0: dma_pool_destroy chain pool, (____ptrval____) busy >>>> >>>> Same error message with dev == NULL before patch: >>>> dma_pool_destroy chain pool, (____ptrval____) busy >>>> >>>> Same error message with dev == NULL after patch: >>>> (NULL device *): dma_pool_destroy chain pool, (____ptrval____) busy >>> Have you checked a history of this? >>> >>> I'm pretty sure this was created in an order to avoid bad looking (and >>> in some cases frightening) "NULL device *" part. >>> >>> If it it's the case, I would rather leave it as is, and even not the >>> case, I'm slightly more bent to the current state. >>> >> I did. "drivers/base/dmapool.c", later moved to "mm/dmapool.c", was >> added in linux-2.6.3, for which dev_err() did not work will a NULL dev, >> so the check was necessary back then. I agree that the (NULL device *): >> bit is ugly, but these messages should be printed only after a kernel >> bug, so it is not like they will be making a regular appearance in >> dmesg. Considering that, I think that it is better to keep it simple. >> > > My original unsubmitted patch used the following: > > +#define pool_err(pool, fmt, args...) \ > + do { \ > + if ((pool)->dev) \ > + dev_err((pool)->dev, fmt, args); \ > + else \ > + pr_err(fmt, args); \ > + } while (0) > > But then I decided to simplify it to just use dev_err(). I still have > the old version. When I submit v3 of the patchset, which would you prefer? JFYI: git log --no-merges --grep 'NULL device \*' P.S. I already shared my opinion on this anyway.
On Fri, Aug 3, 2018 at 6:59 PM, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Fri, Aug 3, 2018 at 6:17 PM, Tony Battersby <tonyb@cybernetics.com> wrote: >> But then I decided to simplify it to just use dev_err(). I still have >> the old version. When I submit v3 of the patchset, which would you prefer? > > JFYI: git log --no-merges --grep 'NULL device \*' Example: commit b4ba97e76763c4e582e3af1079e220e93b1b0d76 Author: Chris Wilson <chris@chris-wilson.co.uk> Date: Fri Aug 19 08:37:50 2016 +0100 drm: Avoid calling dev_printk(.dev = NULL) > P.S. I already shared my opinion on this anyway.
On 08/03/2018 12:01 PM, Andy Shevchenko wrote: > On Fri, Aug 3, 2018 at 6:59 PM, Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: >> On Fri, Aug 3, 2018 at 6:17 PM, Tony Battersby <tonyb@cybernetics.com> wrote: >>> But then I decided to simplify it to just use dev_err(). I still have >>> the old version. When I submit v3 of the patchset, which would you prefer? >> JFYI: git log --no-merges --grep 'NULL device \*' > Example: > > commit b4ba97e76763c4e582e3af1079e220e93b1b0d76 > Author: Chris Wilson <chris@chris-wilson.co.uk> > Date: Fri Aug 19 08:37:50 2016 +0100 > > drm: Avoid calling dev_printk(.dev = NULL) > Point taken. I'll go with pool_err() on the next round.
On Fri, Aug 03, 2018 at 06:59:20PM +0300, Andy Shevchenko wrote: > >>> I'm pretty sure this was created in an order to avoid bad looking (and > >>> in some cases frightening) "NULL device *" part. > > JFYI: git log --no-merges --grep 'NULL device \*' I think those commits actually argue in favour of Tony's patch to remove the special casing. Is it really useful to create dma pools with a NULL device?
On 08/03/2018 12:22 PM, Matthew Wilcox wrote: > On Fri, Aug 03, 2018 at 06:59:20PM +0300, Andy Shevchenko wrote: >>>>> I'm pretty sure this was created in an order to avoid bad looking (and >>>>> in some cases frightening) "NULL device *" part. >> JFYI: git log --no-merges --grep 'NULL device \*' > I think those commits actually argue in favour of Tony's patch to remove > the special casing. Is it really useful to create dma pools with a NULL > device? > > dma_alloc_coherent() does appear to support a NULL dev, so it might make sense in theory. But I can't find any in-tree callers that actually pass a NULL dev to dma_pool_create(). So for one of the dreaded (NULL device *) messages to show up, it would take both a new caller that passes a NULL dev to dma_pool_create() and a bug to cause the message to be printed. Is that worth the special casing?
On Fri, Aug 3, 2018 at 8:03 PM, Tony Battersby <tonyb@cybernetics.com> wrote: > On 08/03/2018 12:22 PM, Matthew Wilcox wrote: >> On Fri, Aug 03, 2018 at 06:59:20PM +0300, Andy Shevchenko wrote: >>>>>> I'm pretty sure this was created in an order to avoid bad looking (and >>>>>> in some cases frightening) "NULL device *" part. >>> JFYI: git log --no-merges --grep 'NULL device \*' >> I think those commits actually argue in favour of Tony's patch to remove >> the special casing. Is it really useful to create dma pools with a NULL >> device? > dma_alloc_coherent() does appear to support a NULL dev, so it might make > sense in theory. But I can't find any in-tree callers that actually > pass a NULL dev to dma_pool_create(). So for one of the dreaded (NULL > device *) messages to show up, it would take both a new caller that > passes a NULL dev to dma_pool_create() and a bug to cause the message to > be printed. Is that worth the special casing? So, then you need to rephrase the commit message explaining this ("NULL device is wrong to pass in the first place... bla bla bla").
On 08/03/2018 01:03 PM, Tony Battersby wrote: > On 08/03/2018 12:22 PM, Matthew Wilcox wrote: >> On Fri, Aug 03, 2018 at 06:59:20PM +0300, Andy Shevchenko wrote: >>>>>> I'm pretty sure this was created in an order to avoid bad looking (and >>>>>> in some cases frightening) "NULL device *" part. >>> JFYI: git log --no-merges --grep 'NULL device \*' >> I think those commits actually argue in favour of Tony's patch to remove >> the special casing. Is it really useful to create dma pools with a NULL >> device? >> >> > dma_alloc_coherent() does appear to support a NULL dev, so it might make > sense in theory. But I can't find any in-tree callers that actually > pass a NULL dev to dma_pool_create(). So for one of the dreaded (NULL > device *) messages to show up, it would take both a new caller that > passes a NULL dev to dma_pool_create() and a bug to cause the message to > be printed. Is that worth the special casing? > Out of curiosity, I just tried to create a dmapool with a NULL dev and it crashed on this: static inline int dev_to_node(struct device *dev) { return dev->numa_node; } struct dma_pool *dma_pool_create(const char *name, struct device *dev, size_t size, size_t align, size_t boundary) { ... retval = kmalloc_node(sizeof(*retval), GFP_KERNEL, dev_to_node(dev)); ... } So either it needs more special cases for supporting a NULL dev, or the special cases can be removed since no one does that anyway.
On 08/03/2018 02:38 PM, Andy Shevchenko wrote: > >> dma_alloc_coherent() does appear to support a NULL dev, so it might make >> sense in theory. But I can't find any in-tree callers that actually >> pass a NULL dev to dma_pool_create(). So for one of the dreaded (NULL >> device *) messages to show up, it would take both a new caller that >> passes a NULL dev to dma_pool_create() and a bug to cause the message to >> be printed. Is that worth the special casing? > So, then you need to rephrase the commit message explaining this > ("NULL device is wrong to pass in the first place... bla bla bla"). > Will do.
On 2018-08-03 02:38 PM, Andy Shevchenko wrote: > On Fri, Aug 3, 2018 at 8:03 PM, Tony Battersby <tonyb@cybernetics.com> wrote: >> On 08/03/2018 12:22 PM, Matthew Wilcox wrote: >>> On Fri, Aug 03, 2018 at 06:59:20PM +0300, Andy Shevchenko wrote: >>>>>>> I'm pretty sure this was created in an order to avoid bad looking (and >>>>>>> in some cases frightening) "NULL device *" part. >>>> JFYI: git log --no-merges --grep 'NULL device \*' >>> I think those commits actually argue in favour of Tony's patch to remove >>> the special casing. Is it really useful to create dma pools with a NULL >>> device? > >> dma_alloc_coherent() does appear to support a NULL dev, so it might make >> sense in theory. But I can't find any in-tree callers that actually >> pass a NULL dev to dma_pool_create(). So for one of the dreaded (NULL >> device *) messages to show up, it would take both a new caller that >> passes a NULL dev to dma_pool_create() and a bug to cause the message to >> be printed. Is that worth the special casing? > > So, then you need to rephrase the commit message explaining this > ("NULL device is wrong to pass in the first place... bla bla bla"). > "Pre-condition(s)", you might use that term for non-obvious requirements for a function. The assumption then is if it/they are violated that your function won't work. It also implies your function does not check them. One implicit pre-condition on almost all C functions that take a pointer: that the pointer points to accessible memory. Doug Gilbert
On Fri, Aug 03, 2018 at 02:43:07PM -0400, Tony Battersby wrote: > Out of curiosity, I just tried to create a dmapool with a NULL dev and > it crashed on this: > > static inline int dev_to_node(struct device *dev) > { > return dev->numa_node; > } > > struct dma_pool *dma_pool_create(const char *name, struct device *dev, > size_t size, size_t align, size_t boundary) > { > ... > retval = kmalloc_node(sizeof(*retval), GFP_KERNEL, dev_to_node(dev)); > ... > } > > So either it needs more special cases for supporting a NULL dev, or the > special cases can be removed since no one does that anyway. Actually, it's worse. dev_to_node() works with a NULL dev ... unless CONFIG_NUMA is set. So we're leaving a timebomb by pretending to allow it. Let's just 'if (!dev) return NULL;' early in create.
On 08/03/2018 05:07 PM, Matthew Wilcox wrote: > On Fri, Aug 03, 2018 at 02:43:07PM -0400, Tony Battersby wrote: >> Out of curiosity, I just tried to create a dmapool with a NULL dev and >> it crashed on this: >> >> static inline int dev_to_node(struct device *dev) >> { >> return dev->numa_node; >> } >> >> struct dma_pool *dma_pool_create(const char *name, struct device *dev, >> size_t size, size_t align, size_t boundary) >> { >> ... >> retval = kmalloc_node(sizeof(*retval), GFP_KERNEL, dev_to_node(dev)); >> ... >> } >> >> So either it needs more special cases for supporting a NULL dev, or the >> special cases can be removed since no one does that anyway. > Actually, it's worse. dev_to_node() works with a NULL dev ... unless > CONFIG_NUMA is set. So we're leaving a timebomb by pretending to > allow it. Let's just 'if (!dev) return NULL;' early in create. > > Looking further down it does stuff with dev->dma_pools unconditionally that doesn't depend on the config. So it would blow up on non-NUMA also. So no timebomb, just an immediate kaboom.
--- linux/mm/dmapool.c.orig 2018-08-02 09:54:25.000000000 -0400 +++ linux/mm/dmapool.c 2018-08-02 09:57:58.000000000 -0400 @@ -289,13 +289,9 @@ void dma_pool_destroy(struct dma_pool *p page = list_entry(pool->page_list.next, struct dma_page, page_list); if (is_page_busy(page)) { - if (pool->dev) - dev_err(pool->dev, - "dma_pool_destroy %s, %p busy\n", - pool->name, page->vaddr); - else - pr_err("dma_pool_destroy %s, %p busy\n", - pool->name, page->vaddr); + dev_err(pool->dev, + "dma_pool_destroy %s, %p busy\n", + pool->name, page->vaddr); /* leak the still-in-use consistent memory */ list_del(&page->page_list); kfree(page); @@ -357,13 +353,9 @@ void *dma_pool_alloc(struct dma_pool *po for (i = sizeof(page->offset); i < pool->size; i++) { if (data[i] == POOL_POISON_FREED) continue; - if (pool->dev) - dev_err(pool->dev, - "dma_pool_alloc %s, %p (corrupted)\n", - pool->name, retval); - else - pr_err("dma_pool_alloc %s, %p (corrupted)\n", - pool->name, retval); + dev_err(pool->dev, + "dma_pool_alloc %s, %p (corrupted)\n", + pool->name, retval); /* * Dump the first 4 bytes even if they are not @@ -418,13 +410,9 @@ void dma_pool_free(struct dma_pool *pool page = pool_find_page(pool, dma); if (!page) { spin_unlock_irqrestore(&pool->lock, flags); - if (pool->dev) - dev_err(pool->dev, - "dma_pool_free %s, %p/%lx (bad dma)\n", - pool->name, vaddr, (unsigned long)dma); - else - pr_err("dma_pool_free %s, %p/%lx (bad dma)\n", - pool->name, vaddr, (unsigned long)dma); + dev_err(pool->dev, + "dma_pool_free %s, %p/%lx (bad dma)\n", + pool->name, vaddr, (unsigned long)dma); return; } @@ -432,13 +420,9 @@ void dma_pool_free(struct dma_pool *pool #ifdef DMAPOOL_DEBUG if ((dma - page->dma) != offset) { spin_unlock_irqrestore(&pool->lock, flags); - if (pool->dev) - dev_err(pool->dev, - "dma_pool_free %s, %p (bad vaddr)/%pad\n", - pool->name, vaddr, &dma); - else - pr_err("dma_pool_free %s, %p (bad vaddr)/%pad\n", - pool->name, vaddr, &dma); + dev_err(pool->dev, + "dma_pool_free %s, %p (bad vaddr)/%pad\n", + pool->name, vaddr, &dma); return; } { @@ -449,12 +433,9 @@ void dma_pool_free(struct dma_pool *pool continue; } spin_unlock_irqrestore(&pool->lock, flags); - if (pool->dev) - dev_err(pool->dev, "dma_pool_free %s, dma %pad already free\n", - pool->name, &dma); - else - pr_err("dma_pool_free %s, dma %pad already free\n", - pool->name, &dma); + dev_err(pool->dev, + "dma_pool_free %s, dma %pad already free\n", + pool->name, &dma); return; } }
Remove code duplication in error messages. It is now safe to pas a NULL dev to dev_err(), so the checks to avoid doing so are no longer necessary. Example: Error message with dev != NULL: mpt3sas 0000:02:00.0: dma_pool_destroy chain pool, (____ptrval____) busy Same error message with dev == NULL before patch: dma_pool_destroy chain pool, (____ptrval____) busy Same error message with dev == NULL after patch: (NULL device *): dma_pool_destroy chain pool, (____ptrval____) busy Signed-off-by: Tony Battersby <tonyb@cybernetics.com> ---