| Message ID | 20180712185930.2492-3-jcrouse@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Jordan Crouse (2018-07-12 19:59:19) > Add a drm printer suitable for use with the read callback for > devcoredump or other suitable buffer based output format that > isn't otherwise covered by seq_file. > > Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org> > --- > drivers/gpu/drm/drm_print.c | 74 +++++++++++++++++++++++++++++++++++++ > include/drm/drm_print.h | 27 ++++++++++++++ > 2 files changed, 101 insertions(+) > > diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c > index b25f98f33f6c..03d1f98e5ac7 100644 > --- a/drivers/gpu/drm/drm_print.c > +++ b/drivers/gpu/drm/drm_print.c > @@ -30,6 +30,80 @@ > #include <drm/drmP.h> > #include <drm/drm_print.h> > > +void __drm_printfn_coredump(struct drm_printer *p, struct va_format *vaf) > +{ > + struct drm_print_iterator *iterator = p->arg; > + ssize_t len; > + > + if (!iterator->remain) > + return; > + > + /* Figure out how big the string will be */ > + len = snprintf(NULL, 0, "%pV", vaf); I was thinking there's some duplication here (kmalloc + snprintf) that could be reduced to kasprintf here. Is avoiding that allocation important or frequent enough to merit open coding? It's pity the kernel's printk doesn't support %n, so that leaves with buf = kasprintf(GFP_... , "%pV", vaf); if (!buf) return; len = strlen(buf); and even the copy + increment looks like it can then be factored to share more code. -Chris
On Thu, Jul 12, 2018 at 12:59:19PM -0600, Jordan Crouse wrote: > Add a drm printer suitable for use with the read callback for > devcoredump or other suitable buffer based output format that > isn't otherwise covered by seq_file. > > Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org> Hm, why not add seq_file support to dev_coredump? Neither git blame nor google sched any light on why seq_file wasn't picked over the custom read interface ... Adding Johannes and Greg about this. If we go with this, one comment below. > --- > drivers/gpu/drm/drm_print.c | 74 +++++++++++++++++++++++++++++++++++++ > include/drm/drm_print.h | 27 ++++++++++++++ > 2 files changed, 101 insertions(+) > > diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c > index b25f98f33f6c..03d1f98e5ac7 100644 > --- a/drivers/gpu/drm/drm_print.c > +++ b/drivers/gpu/drm/drm_print.c > @@ -30,6 +30,80 @@ > #include <drm/drmP.h> > #include <drm/drm_print.h> > > +void __drm_printfn_coredump(struct drm_printer *p, struct va_format *vaf) > +{ > + struct drm_print_iterator *iterator = p->arg; > + ssize_t len; > + > + if (!iterator->remain) > + return; > + > + /* Figure out how big the string will be */ > + len = snprintf(NULL, 0, "%pV", vaf); > + > + if (iterator->offset < iterator->start) { > + char *buf; > + ssize_t copy; > + > + if (iterator->offset + len <= iterator->start) { > + iterator->offset += len; > + return; > + } > + > + /* Print the string into a temporary buffer */ > + buf = kmalloc(len + 1, > + GFP_KERNEL | __GFP_NOWARN | __GFP_NORETRY); > + if (!buf) > + return; > + > + snprintf(buf, len + 1, "%pV", vaf); > + > + copy = len - (iterator->start - iterator->offset); > + > + if (copy > iterator->remain) > + copy = iterator->remain; > + > + /* Copy out the bit of the string that we need */ > + memcpy(iterator->data, > + buf + (iterator->start - iterator->offset), copy); > + > + iterator->offset = iterator->start + copy; > + iterator->remain -= copy; > + > + kfree(buf); > + } else { > + char *buf; > + ssize_t pos = iterator->offset - iterator->start; > + > + if (len < iterator->remain) { > + snprintf(((char *) iterator->data) + pos, > + iterator->remain, "%pV", vaf); > + > + iterator->offset += len; > + iterator->remain -= len; > + > + return; > + } > + > + /* Print the string into a temporary buffer */ > + buf = kmalloc(len + 1, > + GFP_KERNEL | __GFP_NOWARN | __GFP_NORETRY); > + if (!buf) > + return; > + > + snprintf(buf, len + 1, "%pV", vaf); > + > + /* Copy out the remaining bits */ > + memcpy(iterator->data + pos, buf, iterator->remain); > + > + iterator->offset += iterator->remain; > + iterator->remain = 0; > + > + kfree(buf); > + } > +} > +EXPORT_SYMBOL(__drm_printfn_coredump); > + > void __drm_printfn_seq_file(struct drm_printer *p, struct va_format *vaf) > { > seq_printf(p->arg, "%pV", vaf); > diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h > index e1a46e9991cc..0ea440fb5ec3 100644 > --- a/include/drm/drm_print.h > +++ b/include/drm/drm_print.h > @@ -73,6 +73,7 @@ struct drm_printer { > const char *prefix; > }; > > +void __drm_printfn_coredump(struct drm_printer *p, struct va_format *vaf); > void __drm_printfn_seq_file(struct drm_printer *p, struct va_format *vaf); > void __drm_printfn_info(struct drm_printer *p, struct va_format *vaf); > void __drm_printfn_debug(struct drm_printer *p, struct va_format *vaf); > @@ -104,6 +105,32 @@ drm_vprintf(struct drm_printer *p, const char *fmt, va_list *va) > #define drm_printf_indent(printer, indent, fmt, ...) \ > drm_printf((printer), "%.*s" fmt, (indent), "\t\t\t\t\tX", ##__VA_ARGS__) > > +struct drm_print_iterator { > + void *data; > + > + ssize_t start; > + ssize_t offset; > + ssize_t remain; > +}; > + > +/** > + * drm_coredump_printer - construct a &drm_printer that can output to a buffer > + * from the read function for devcoredump > + * @iter: A pointer to a struct drm_print_iterator for the read instance Bit more flesh for the kerneldoc would be good here, maybe even with a small in-line example. Definitely a link to dev_coredumpm() which I assume is the function you're going to use this with. Pls also make sure it all looks nice using make htmldocs. -Daniel > + * > + * RETURNS: > + * The &drm_printer object > + */ > +static inline struct drm_printer > +drm_coredump_printer(struct drm_print_iterator *iter) > +{ > + struct drm_printer p = { > + .printfn = __drm_printfn_coredump, > + .arg = iter, > + }; > + return p; > +} > + > /** > * drm_seq_file_printer - construct a &drm_printer that outputs to &seq_file > * @f: the &struct seq_file to output to > -- > 2.17.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, Jul 12, 2018 at 08:40:55PM +0100, Chris Wilson wrote: > Quoting Jordan Crouse (2018-07-12 19:59:19) > > Add a drm printer suitable for use with the read callback for > > devcoredump or other suitable buffer based output format that > > isn't otherwise covered by seq_file. > > > > Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org> > > --- > > drivers/gpu/drm/drm_print.c | 74 +++++++++++++++++++++++++++++++++++++ > > include/drm/drm_print.h | 27 ++++++++++++++ > > 2 files changed, 101 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c > > index b25f98f33f6c..03d1f98e5ac7 100644 > > --- a/drivers/gpu/drm/drm_print.c > > +++ b/drivers/gpu/drm/drm_print.c > > @@ -30,6 +30,80 @@ > > #include <drm/drmP.h> > > #include <drm/drm_print.h> > > > > +void __drm_printfn_coredump(struct drm_printer *p, struct va_format *vaf) > > +{ > > + struct drm_print_iterator *iterator = p->arg; > > + ssize_t len; > > + > > + if (!iterator->remain) > > + return; > > + > > + /* Figure out how big the string will be */ > > + len = snprintf(NULL, 0, "%pV", vaf); > > I was thinking there's some duplication here (kmalloc + snprintf) that > could be reduced to kasprintf here. Is avoiding that allocation > important or frequent enough to merit open coding? > > It's pity the kernel's printk doesn't support %n, so that leaves with > > buf = kasprintf(GFP_... , "%pV", vaf); > if (!buf) > return; > > len = strlen(buf); > and even the copy + increment looks like it can then be factored to share > more code. I could profile it to see if avoiding the allocation is worth while. I have a use case that prints approximately 1MB (stupid GPU, be less complex) so that is enough to be able to see noticeable deltas if they exist. Jordan
On Thu, Jul 12, 2018 at 09:46:58PM +0200, Daniel Vetter wrote: > On Thu, Jul 12, 2018 at 12:59:19PM -0600, Jordan Crouse wrote: > > Add a drm printer suitable for use with the read callback for > > devcoredump or other suitable buffer based output format that > > isn't otherwise covered by seq_file. > > > > Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org> > > Hm, why not add seq_file support to dev_coredump? Neither git blame nor > google sched any light on why seq_file wasn't picked over the custom read > interface ... > > Adding Johannes and Greg about this. Main reason was that this is used for devcoredump which has its own similar but not quite seq_file compatible callback. If there is synergy to be had there that would be great because reinventing the wheel isn't fun. > If we go with this, one comment below. > > > --- > > drivers/gpu/drm/drm_print.c | 74 +++++++++++++++++++++++++++++++++++++ > > include/drm/drm_print.h | 27 ++++++++++++++ > > 2 files changed, 101 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c > > index b25f98f33f6c..03d1f98e5ac7 100644 > > --- a/drivers/gpu/drm/drm_print.c > > +++ b/drivers/gpu/drm/drm_print.c > > @@ -30,6 +30,80 @@ > > #include <drm/drmP.h> > > #include <drm/drm_print.h> > > > > +void __drm_printfn_coredump(struct drm_printer *p, struct va_format *vaf) > > +{ > > + struct drm_print_iterator *iterator = p->arg; > > + ssize_t len; > > + > > + if (!iterator->remain) > > + return; > > + > > + /* Figure out how big the string will be */ > > + len = snprintf(NULL, 0, "%pV", vaf); > > + > > + if (iterator->offset < iterator->start) { > > + char *buf; > > + ssize_t copy; > > + > > + if (iterator->offset + len <= iterator->start) { > > + iterator->offset += len; > > + return; > > + } > > + > > + /* Print the string into a temporary buffer */ > > + buf = kmalloc(len + 1, > > + GFP_KERNEL | __GFP_NOWARN | __GFP_NORETRY); > > + if (!buf) > > + return; > > + > > + snprintf(buf, len + 1, "%pV", vaf); > > + > > + copy = len - (iterator->start - iterator->offset); > > + > > + if (copy > iterator->remain) > > + copy = iterator->remain; > > + > > + /* Copy out the bit of the string that we need */ > > + memcpy(iterator->data, > > + buf + (iterator->start - iterator->offset), copy); > > + > > + iterator->offset = iterator->start + copy; > > + iterator->remain -= copy; > > + > > + kfree(buf); > > + } else { > > + char *buf; > > + ssize_t pos = iterator->offset - iterator->start; > > + > > + if (len < iterator->remain) { > > + snprintf(((char *) iterator->data) + pos, > > + iterator->remain, "%pV", vaf); > > + > > + iterator->offset += len; > > + iterator->remain -= len; > > + > > + return; > > + } > > + > > + /* Print the string into a temporary buffer */ > > + buf = kmalloc(len + 1, > > + GFP_KERNEL | __GFP_NOWARN | __GFP_NORETRY); > > + if (!buf) > > + return; > > + > > + snprintf(buf, len + 1, "%pV", vaf); > > + > > + /* Copy out the remaining bits */ > > + memcpy(iterator->data + pos, buf, iterator->remain); > > + > > + iterator->offset += iterator->remain; > > + iterator->remain = 0; > > + > > + kfree(buf); > > + } > > +} > > +EXPORT_SYMBOL(__drm_printfn_coredump); > > + > > void __drm_printfn_seq_file(struct drm_printer *p, struct va_format *vaf) > > { > > seq_printf(p->arg, "%pV", vaf); > > diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h > > index e1a46e9991cc..0ea440fb5ec3 100644 > > --- a/include/drm/drm_print.h > > +++ b/include/drm/drm_print.h > > @@ -73,6 +73,7 @@ struct drm_printer { > > const char *prefix; > > }; > > > > +void __drm_printfn_coredump(struct drm_printer *p, struct va_format *vaf); > > void __drm_printfn_seq_file(struct drm_printer *p, struct va_format *vaf); > > void __drm_printfn_info(struct drm_printer *p, struct va_format *vaf); > > void __drm_printfn_debug(struct drm_printer *p, struct va_format *vaf); > > @@ -104,6 +105,32 @@ drm_vprintf(struct drm_printer *p, const char *fmt, va_list *va) > > #define drm_printf_indent(printer, indent, fmt, ...) \ > > drm_printf((printer), "%.*s" fmt, (indent), "\t\t\t\t\tX", ##__VA_ARGS__) > > > > +struct drm_print_iterator { > > + void *data; > > + > > + ssize_t start; > > + ssize_t offset; > > + ssize_t remain; > > +}; > > + > > +/** > > + * drm_coredump_printer - construct a &drm_printer that can output to a buffer > > + * from the read function for devcoredump > > + * @iter: A pointer to a struct drm_print_iterator for the read instance > > Bit more flesh for the kerneldoc would be good here, maybe even with a > small in-line example. Definitely a link to dev_coredumpm() which I assume > is the function you're going to use this with. > Pls also make sure it all looks nice using make htmldocs. Can do. Thanks. > -Daniel > > > > + * > > + * RETURNS: > > + * The &drm_printer object > > + */ > > +static inline struct drm_printer > > +drm_coredump_printer(struct drm_print_iterator *iter) > > +{ > > + struct drm_printer p = { > > + .printfn = __drm_printfn_coredump, > > + .arg = iter, > > + }; > > + return p; > > +} > > + > > /** > > * drm_seq_file_printer - construct a &drm_printer that outputs to &seq_file > > * @f: the &struct seq_file to output to > > -- > > 2.17.1 > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
> > Hm, why not add seq_file support to dev_coredump? Neither git blame > > nor google sched any light on why seq_file wasn't picked over the > > custom read interface ... > > > > Adding Johannes and Greg about this. > > Main reason was that this is used for devcoredump which has its own similar > but not quite seq_file compatible callback. If there is synergy to be had there > that would be great because reinventing the wheel isn't fun. Adding or changing it to seq_file is fine with me, I don't think we really need the devm_coredump() much these days since we have the vmalloc one. (apologies for the footer and all - I'm on vacation and in a hurry) johannes
On Thu, Jul 12, 2018 at 08:40:55PM +0100, Chris Wilson wrote: > Quoting Jordan Crouse (2018-07-12 19:59:19) > > Add a drm printer suitable for use with the read callback for > > devcoredump or other suitable buffer based output format that > > isn't otherwise covered by seq_file. > > > > Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org> > > --- > > drivers/gpu/drm/drm_print.c | 74 +++++++++++++++++++++++++++++++++++++ > > include/drm/drm_print.h | 27 ++++++++++++++ > > 2 files changed, 101 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c > > index b25f98f33f6c..03d1f98e5ac7 100644 > > --- a/drivers/gpu/drm/drm_print.c > > +++ b/drivers/gpu/drm/drm_print.c > > @@ -30,6 +30,80 @@ > > #include <drm/drmP.h> > > #include <drm/drm_print.h> > > > > +void __drm_printfn_coredump(struct drm_printer *p, struct va_format *vaf) > > +{ > > + struct drm_print_iterator *iterator = p->arg; > > + ssize_t len; > > + > > + if (!iterator->remain) > > + return; > > + > > + /* Figure out how big the string will be */ > > + len = snprintf(NULL, 0, "%pV", vaf); > > I was thinking there's some duplication here (kmalloc + snprintf) that > could be reduced to kasprintf here. Is avoiding that allocation > important or frequent enough to merit open coding? > It's pity the kernel's printk doesn't support %n, so that leaves with > > buf = kasprintf(GFP_... , "%pV", vaf); > if (!buf) > return; > > len = strlen(buf); > > and even the copy + increment looks like it can then be factored to share > more code. I did a quick test - using kasprintf() unconditionally increased the total time in my use case by about 4x. I think this was mainly due to this case: if (iterator->offset + len <= iterator->start) { .... return; } That said, I was able to organize the code so that we can reuse much of the same code for both the printf and puts funcs which saves us a bit of churn later. Jordan
diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c index b25f98f33f6c..03d1f98e5ac7 100644 --- a/drivers/gpu/drm/drm_print.c +++ b/drivers/gpu/drm/drm_print.c @@ -30,6 +30,80 @@ #include <drm/drmP.h> #include <drm/drm_print.h> +void __drm_printfn_coredump(struct drm_printer *p, struct va_format *vaf) +{ + struct drm_print_iterator *iterator = p->arg; + ssize_t len; + + if (!iterator->remain) + return; + + /* Figure out how big the string will be */ + len = snprintf(NULL, 0, "%pV", vaf); + + if (iterator->offset < iterator->start) { + char *buf; + ssize_t copy; + + if (iterator->offset + len <= iterator->start) { + iterator->offset += len; + return; + } + + /* Print the string into a temporary buffer */ + buf = kmalloc(len + 1, + GFP_KERNEL | __GFP_NOWARN | __GFP_NORETRY); + if (!buf) + return; + + snprintf(buf, len + 1, "%pV", vaf); + + copy = len - (iterator->start - iterator->offset); + + if (copy > iterator->remain) + copy = iterator->remain; + + /* Copy out the bit of the string that we need */ + memcpy(iterator->data, + buf + (iterator->start - iterator->offset), copy); + + iterator->offset = iterator->start + copy; + iterator->remain -= copy; + + kfree(buf); + } else { + char *buf; + ssize_t pos = iterator->offset - iterator->start; + + if (len < iterator->remain) { + snprintf(((char *) iterator->data) + pos, + iterator->remain, "%pV", vaf); + + iterator->offset += len; + iterator->remain -= len; + + return; + } + + /* Print the string into a temporary buffer */ + buf = kmalloc(len + 1, + GFP_KERNEL | __GFP_NOWARN | __GFP_NORETRY); + if (!buf) + return; + + snprintf(buf, len + 1, "%pV", vaf); + + /* Copy out the remaining bits */ + memcpy(iterator->data + pos, buf, iterator->remain); + + iterator->offset += iterator->remain; + iterator->remain = 0; + + kfree(buf); + } +} +EXPORT_SYMBOL(__drm_printfn_coredump); + void __drm_printfn_seq_file(struct drm_printer *p, struct va_format *vaf) { seq_printf(p->arg, "%pV", vaf); diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h index e1a46e9991cc..0ea440fb5ec3 100644 --- a/include/drm/drm_print.h +++ b/include/drm/drm_print.h @@ -73,6 +73,7 @@ struct drm_printer { const char *prefix; }; +void __drm_printfn_coredump(struct drm_printer *p, struct va_format *vaf); void __drm_printfn_seq_file(struct drm_printer *p, struct va_format *vaf); void __drm_printfn_info(struct drm_printer *p, struct va_format *vaf); void __drm_printfn_debug(struct drm_printer *p, struct va_format *vaf); @@ -104,6 +105,32 @@ drm_vprintf(struct drm_printer *p, const char *fmt, va_list *va) #define drm_printf_indent(printer, indent, fmt, ...) \ drm_printf((printer), "%.*s" fmt, (indent), "\t\t\t\t\tX", ##__VA_ARGS__) +struct drm_print_iterator { + void *data; + + ssize_t start; + ssize_t offset; + ssize_t remain; +}; + +/** + * drm_coredump_printer - construct a &drm_printer that can output to a buffer + * from the read function for devcoredump + * @iter: A pointer to a struct drm_print_iterator for the read instance + * + * RETURNS: + * The &drm_printer object + */ +static inline struct drm_printer +drm_coredump_printer(struct drm_print_iterator *iter) +{ + struct drm_printer p = { + .printfn = __drm_printfn_coredump, + .arg = iter, + }; + return p; +} + /** * drm_seq_file_printer - construct a &drm_printer that outputs to &seq_file * @f: the &struct seq_file to output to
Add a drm printer suitable for use with the read callback for devcoredump or other suitable buffer based output format that isn't otherwise covered by seq_file. Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org> --- drivers/gpu/drm/drm_print.c | 74 +++++++++++++++++++++++++++++++++++++ include/drm/drm_print.h | 27 ++++++++++++++ 2 files changed, 101 insertions(+)