Re: [PATCH 3/4] lib/hexdump.c: Replace ascii bool in hex_dump_to_buffer with flags
On Sat 2019-04-13 09:31:27, Alastair D'Silva wrote: > > -Original Message- > > From: Petr Mladek > > Sent: Saturday, 13 April 2019 12:12 AM > > To: Alastair D'Silva > > Cc: alast...@d-silva.org; Jani Nikula ; > Joonas > > Lahtinen ; Rodrigo Vivi > > ; David Airlie ; Daniel Vetter > > ; Karsten Keil ; Jassi Brar > > ; Tom Lendacky ; > > David S. Miller ; Jose Abreu > > ; Kalle Valo ; > > Stanislaw Gruszka ; Benson Leung > > ; Enric Balletbo i Serra > > ; James E.J. Bottomley > > ; Martin K. Petersen ; > > Greg Kroah-Hartman ; Alexander Viro > > ; Sergey Senozhatsky > > ; Steven Rostedt ; > > Andrew Morton ; intel- > > g...@lists.freedesktop.org; dri-de...@lists.freedesktop.org; linux- > > ker...@vger.kernel.org; net...@vger.kernel.org; > > ath...@lists.infradead.org; linux-wirel...@vger.kernel.org; linux- > > s...@vger.kernel.org; linux-fb...@vger.kernel.org; > > de...@driverdev.osuosl.org; linux-fsde...@vger.kernel.org > > Subject: Re: [PATCH 3/4] lib/hexdump.c: Replace ascii bool in > > hex_dump_to_buffer with flags > > > > On Wed 2019-04-10 13:17:19, Alastair D'Silva wrote: > > > From: Alastair D'Silva > > > > > > In order to support additional features in hex_dump_to_buffer, replace > > > the ascii bool parameter with flags. > > > > > > Signed-off-by: Alastair D'Silva > > > --- > > > drivers/gpu/drm/i915/intel_engine_cs.c| 2 +- > > > drivers/isdn/hardware/mISDN/mISDNisar.c | 6 -- > > > drivers/mailbox/mailbox-test.c| 2 +- > > > drivers/net/ethernet/amd/xgbe/xgbe-drv.c | 2 +- > > > drivers/net/ethernet/synopsys/dwc-xlgmac-common.c | 2 +- > > > drivers/net/wireless/ath/ath10k/debug.c | 3 ++- > > > drivers/net/wireless/intel/iwlegacy/3945-mac.c| 2 +- > > > drivers/platform/chrome/wilco_ec/debugfs.c| 3 ++- > > > drivers/scsi/scsi_logging.c | 8 +++- > > > drivers/staging/fbtft/fbtft-core.c| 2 +- > > > fs/seq_file.c | 3 ++- > > > include/linux/printk.h| 2 +- > > > lib/hexdump.c | 15 --- > > > lib/test_hexdump.c| 5 +++-- > > > 14 files changed, 31 insertions(+), 26 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c > > > b/drivers/gpu/drm/i915/intel_engine_cs.c > > > index 49fa43ff02ba..fb133e729f9a 100644 > > > --- a/drivers/gpu/drm/i915/intel_engine_cs.c > > > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c > > > @@ -1318,7 +1318,7 @@ static void hexdump(struct drm_printer *m, const > > void *buf, size_t len) > > > WARN_ON_ONCE(hex_dump_to_buffer(buf + pos, len - > > pos, > > > rowsize, sizeof(u32), > > > line, sizeof(line), > > > - false) >= sizeof(line)); > > > + 0) >= sizeof(line)); > > > > It might be more clear when we define: > > > > #define HEXDUMP_BINARY 0 > > This feels unnecessary, and weird. Omitting the flag won't disable the hex > output (as expected), and if you don't want hex output why call hexdump in > the first place? Why do we have HEXDUMP_ASCII then? Why is the above funtion not using HEXDUMP_ASCII? Who would call it with (HEXDUMP_ASCII | HEXDUMP_BINARY)? Best Regards, Petr
Re: [PATCH 2/4] lib/hexdump.c: Optionally suppress lines of filler bytes
On Sat 2019-04-13 09:28:03, Alastair D'Silva wrote:
> > -Original Message-
> > From: Petr Mladek
> > Sent: Saturday, 13 April 2019 12:04 AM
> > To: Alastair D'Silva
> > Cc: alast...@d-silva.org; Jani Nikula ;
> Joonas
> > Lahtinen ; Rodrigo Vivi
> > ; David Airlie ; Daniel Vetter
> > ; Karsten Keil ; Jassi Brar
> > ; Tom Lendacky ;
> > David S. Miller ; Jose Abreu
> > ; Kalle Valo ;
> > Stanislaw Gruszka ; Benson Leung
> > ; Enric Balletbo i Serra
> > ; James E.J. Bottomley
> > ; Martin K. Petersen ;
> > Greg Kroah-Hartman ; Alexander Viro
> > ; Sergey Senozhatsky
> > ; Steven Rostedt ;
> > Andrew Morton ; intel-
> > g...@lists.freedesktop.org; dri-de...@lists.freedesktop.org; linux-
> > ker...@vger.kernel.org; net...@vger.kernel.org;
> > ath...@lists.infradead.org; linux-wirel...@vger.kernel.org; linux-
> > s...@vger.kernel.org; linux-fb...@vger.kernel.org;
> > de...@driverdev.osuosl.org; linux-fsde...@vger.kernel.org
> > Subject: Re: [PATCH 2/4] lib/hexdump.c: Optionally suppress lines of
> filler
> > bytes
> >
> > On Wed 2019-04-10 13:17:18, Alastair D'Silva wrote:
> > > From: Alastair D'Silva
> > >
> > > Some buffers may only be partially filled with useful data, while the
> > > rest is padded (typically with 0x00 or 0xff).
> > >
> > > This patch introduces flags which allow lines of padding bytes to be
> > > suppressed, making the output easier to interpret:
> > > HEXDUMP_SUPPRESS_0X00, HEXDUMP_SUPPRESS_0XFF
> > >
> > > The first and last lines are not suppressed by default, so the
> > > function always outputs something. This behaviour can be further
> > > controlled with the HEXDUMP_SUPPRESS_FIRST &
> > HEXDUMP_SUPPRESS_LAST flags.
> > >
> > > An inline wrapper function is provided for backwards compatibility
> > > with existing code, which maintains the original behaviour.
> > >
> >
> > > diff --git a/lib/hexdump.c b/lib/hexdump.c index
> > > b8a164814744..2f3bafb55a44 100644
> > > --- a/lib/hexdump.c
> > > +++ b/lib/hexdump.c
> > > +void print_hex_dump_ext(const char *level, const char *prefix_str,
> > > + int prefix_type, int rowsize, int groupsize,
> > > + const void *buf, size_t len, u64 flags)
> > > {
> > > const u8 *ptr = buf;
> > > - int i, linelen, remaining = len;
> > > + int i, remaining = len;
> > > unsigned char linebuf[64 * 3 + 2 + 64 + 1];
> > > + bool first_line = true;
> > >
> > > if (rowsize != 16 && rowsize != 32 && rowsize != 64)
> > > rowsize = 16;
> > >
> > > for (i = 0; i < len; i += rowsize) {
> > > - linelen = min(remaining, rowsize);
> > > + bool skip = false;
> > > + int linelen = min(remaining, rowsize);
> > > +
> > > remaining -= rowsize;
> > >
> > > + if (flags & HEXDUMP_SUPPRESS_0X00)
> > > + skip = buf_is_all(ptr + i, linelen, 0x00);
> > > +
> > > + if (!skip && (flags & HEXDUMP_SUPPRESS_0XFF))
> > > + skip = buf_is_all(ptr + i, linelen, 0xff);
> > > +
> > > + if (first_line && !(flags & HEXDUMP_SUPPRESS_FIRST))
> > > + skip = false;
> > > +
> > > + if (remaining <= 0 && !(flags & HEXDUMP_SUPPRESS_LAST))
> > > + skip = false;
> > > +
> > > + if (skip)
> > > + continue;
> >
> > IMHO, quietly skipping lines could cause a lot of confusion, espcially
> when the address is not printed.
> >
> It's up to the caller to decide how they want it displayed.
I wonder who would want to quietly skip some data values.
Are you using it yourself? Could you please provide an
example?
I do not see why we would need to complicate the API and code
by this.
The behavior proposed by Tvrtko Ursulin makes much more
sense. I mean
https://lkml.kernel.org/r/929244ed-cc7f-b0f3-b5ac-50e798e83...@linux.intel.com
> > I wonder how it would look like when we print something like:
> >
> > --- skipped XX lines full of 0x00 ---
>
> This could be added as a later enhancement, with a new flag (eg.
> HEXDUMP_SUPPRESS_VERBOSE).
Who will add this later? Frankly, this looks like a half baked
feature that it good enough for you. If you want it upstream,
it must behave reasonably and it must be worth it.
Best Regards,
Petr
Re: [PATCH 1/4] lib/hexdump.c: Allow 64 bytes per line
On Sat 2019-04-13 09:22:05, Alastair D'Silva wrote: > > -Original Message- > > From: Petr Mladek > > Sent: Friday, 12 April 2019 11:48 PM > > To: Alastair D'Silva > > Cc: alast...@d-silva.org; Jani Nikula ; > Joonas > > Lahtinen ; Rodrigo Vivi > > ; David Airlie ; Daniel Vetter > > ; Karsten Keil ; Jassi Brar > > ; Tom Lendacky ; > > David S. Miller ; Jose Abreu > > ; Kalle Valo ; > > Stanislaw Gruszka ; Benson Leung > > ; Enric Balletbo i Serra > > ; James E.J. Bottomley > > ; Martin K. Petersen ; > > Greg Kroah-Hartman ; Alexander Viro > > ; Sergey Senozhatsky > > ; Steven Rostedt ; > > Andrew Morton ; intel- > > g...@lists.freedesktop.org; dri-de...@lists.freedesktop.org; linux- > > ker...@vger.kernel.org; net...@vger.kernel.org; > > ath...@lists.infradead.org; linux-wirel...@vger.kernel.org; linux- > > s...@vger.kernel.org; linux-fb...@vger.kernel.org; > > de...@driverdev.osuosl.org; linux-fsde...@vger.kernel.org > > Subject: Re: [PATCH 1/4] lib/hexdump.c: Allow 64 bytes per line > > > > On Wed 2019-04-10 13:17:17, Alastair D'Silva wrote: > > > From: Alastair D'Silva > > > > > > With modern high resolution screens, we can display more data, which > > > makes life a bit easier when debugging. > > > > I have quite some doubts about this feature. > > > > We are talking about more than 256 characters per-line. I wonder if such a > > long line is really easier to read for a human. > > It's basically 2 separate panes of information side by side, the hexdump and > the ASCII version. > > I'm using this myself when dealing with the pmem labels, and it works quite > nicely. I am sure that it works for you. But I do not believe that it would be useful in general. > > I am not expert but there is a reason why the standard is 80 characters > per- > > line. I guess that anything above 100 characters is questionable. > > https://en.wikipedia.org/wiki/Line_length > > somehow confirms that. > > > > Second, if we take 8 pixels per-character. Then we need > > 2048 to show the 256 characters. It is more than HD. > > IMHO, there is still huge number of people that even do not have HD > display, > > especially on a notebook. > > The intent is to make debugging easier when dealing with large chunks of > binary data. I don't expect end users to see this output. How is it supposed to be used then? Only by your temporary patches? Best Regards, Petr
Re: [PATCH 3/4] lib/hexdump.c: Replace ascii bool in hex_dump_to_buffer with flags
On Wed 2019-04-10 13:17:19, Alastair D'Silva wrote:
> From: Alastair D'Silva
>
> In order to support additional features in hex_dump_to_buffer, replace
> the ascii bool parameter with flags.
>
> Signed-off-by: Alastair D'Silva
> ---
> drivers/gpu/drm/i915/intel_engine_cs.c| 2 +-
> drivers/isdn/hardware/mISDN/mISDNisar.c | 6 --
> drivers/mailbox/mailbox-test.c| 2 +-
> drivers/net/ethernet/amd/xgbe/xgbe-drv.c | 2 +-
> drivers/net/ethernet/synopsys/dwc-xlgmac-common.c | 2 +-
> drivers/net/wireless/ath/ath10k/debug.c | 3 ++-
> drivers/net/wireless/intel/iwlegacy/3945-mac.c| 2 +-
> drivers/platform/chrome/wilco_ec/debugfs.c| 3 ++-
> drivers/scsi/scsi_logging.c | 8 +++-
> drivers/staging/fbtft/fbtft-core.c| 2 +-
> fs/seq_file.c | 3 ++-
> include/linux/printk.h| 2 +-
> lib/hexdump.c | 15 ---
> lib/test_hexdump.c| 5 +++--
> 14 files changed, 31 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c
> b/drivers/gpu/drm/i915/intel_engine_cs.c
> index 49fa43ff02ba..fb133e729f9a 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -1318,7 +1318,7 @@ static void hexdump(struct drm_printer *m, const void
> *buf, size_t len)
> WARN_ON_ONCE(hex_dump_to_buffer(buf + pos, len - pos,
> rowsize, sizeof(u32),
> line, sizeof(line),
> - false) >= sizeof(line));
> + 0) >= sizeof(line));
It might be more clear when we define:
#define HEXDUMP_BINARY 0
> drm_printf(m, "[%04zx] %s\n", pos, line);
>
> prev = buf + pos;
> diff --git a/include/linux/printk.h b/include/linux/printk.h
> index c014e5573665..82975853c400 100644
> --- a/include/linux/printk.h
> +++ b/include/linux/printk.h
> @@ -493,7 +493,7 @@ enum {
>
> extern int hex_dump_to_buffer(const void *buf, size_t len, int rowsize,
> int groupsize, char *linebuf, size_t linebuflen,
> - bool ascii);
> + u64 flags);
I wonder how fancy hex_dump could be. IMHO, u32 should be enough.
The last famous words ;-)
Best Regards,
Petr
Re: [PATCH 2/4] lib/hexdump.c: Optionally suppress lines of filler bytes
On Wed 2019-04-10 13:17:18, Alastair D'Silva wrote:
> From: Alastair D'Silva
>
> Some buffers may only be partially filled with useful data, while the rest
> is padded (typically with 0x00 or 0xff).
>
> This patch introduces flags which allow lines of padding bytes to be
> suppressed, making the output easier to interpret: HEXDUMP_SUPPRESS_0X00,
> HEXDUMP_SUPPRESS_0XFF
>
> The first and last lines are not suppressed by default, so the function
> always outputs something. This behaviour can be further controlled with
> the HEXDUMP_SUPPRESS_FIRST & HEXDUMP_SUPPRESS_LAST flags.
>
> An inline wrapper function is provided for backwards compatibility with
> existing code, which maintains the original behaviour.
>
> diff --git a/lib/hexdump.c b/lib/hexdump.c
> index b8a164814744..2f3bafb55a44 100644
> --- a/lib/hexdump.c
> +++ b/lib/hexdump.c
> +void print_hex_dump_ext(const char *level, const char *prefix_str,
> + int prefix_type, int rowsize, int groupsize,
> + const void *buf, size_t len, u64 flags)
> {
> const u8 *ptr = buf;
> - int i, linelen, remaining = len;
> + int i, remaining = len;
> unsigned char linebuf[64 * 3 + 2 + 64 + 1];
> + bool first_line = true;
>
> if (rowsize != 16 && rowsize != 32 && rowsize != 64)
> rowsize = 16;
>
> for (i = 0; i < len; i += rowsize) {
> - linelen = min(remaining, rowsize);
> + bool skip = false;
> + int linelen = min(remaining, rowsize);
> +
> remaining -= rowsize;
>
> + if (flags & HEXDUMP_SUPPRESS_0X00)
> + skip = buf_is_all(ptr + i, linelen, 0x00);
> +
> + if (!skip && (flags & HEXDUMP_SUPPRESS_0XFF))
> + skip = buf_is_all(ptr + i, linelen, 0xff);
> +
> + if (first_line && !(flags & HEXDUMP_SUPPRESS_FIRST))
> + skip = false;
> +
> + if (remaining <= 0 && !(flags & HEXDUMP_SUPPRESS_LAST))
> + skip = false;
> +
> + if (skip)
> + continue;
IMHO, quietly skipping lines could cause a lot of confusion,
espcially when the address is not printed.
I wonder how it would look like when we print something like:
--- skipped XX lines full of 0x00 ---
Then we might even remove the SUPPRESS_FIRST, SUPPRESS_LAST
and the ambiguous QUIET flags.
> +
> + first_line = false;
This should be above the if (skip).
> +
> hex_dump_to_buffer(ptr + i, linelen, rowsize, groupsize,
> -linebuf, sizeof(linebuf), ascii);
> +linebuf, sizeof(linebuf),
> +flags & HEXDUMP_ASCII);
>
> switch (prefix_type) {
> case DUMP_PREFIX_ADDRESS:
> @@ -272,7 +316,7 @@ void print_hex_dump(const char *level, const char
> *prefix_str, int prefix_type,
> }
> }
> }
> -EXPORT_SYMBOL(print_hex_dump);
> +EXPORT_SYMBOL(print_hex_dump_ext);
We should still export even the original function that
is still used everywhere.
Best Regards,
Petr
Re: [PATCH 1/4] lib/hexdump.c: Allow 64 bytes per line
On Wed 2019-04-10 13:17:17, Alastair D'Silva wrote: > From: Alastair D'Silva > > With modern high resolution screens, we can display more data, which makes > life a bit easier when debugging. I have quite some doubts about this feature. We are talking about more than 256 characters per-line. I wonder if such a long line is really easier to read for a human. I am not expert but there is a reason why the standard is 80 characters per-line. I guess that anything above 100 characters is questionable. https://en.wikipedia.org/wiki/Line_length somehow confirms that. Second, if we take 8 pixels per-character. Then we need 2048 to show the 256 characters. It is more than HD. IMHO, there is still huge number of people that even do not have HD display, especially on a notebook. I am sorry but I am strongly against having this hardcoded anywhere. And I doubt that there will be enough users to complicate the code and make it configurable. Best Regards, Petr
Re: [PATCH v2 1/1] treewide: Switch printk users from %pf and %pF to %ps and %pS, respectively
On Wed 2019-04-03 14:28:14, Sakari Ailus wrote: > Ping. > > On Tue, Mar 26, 2019 at 02:35:10PM +0100, Petr Mladek wrote: > > Linus, > > > > On Mon 2019-03-25 21:32:28, Sakari Ailus wrote: > > > %pF and %pf are functionally equivalent to %pS and %ps conversion > > > specifiers. The former are deprecated, therefore switch the current users > > > to use the preferred variant. > > > > > > The changes have been produced by the following command: > > > > > > git grep -l '%p[fF]' | grep -v '^\(tools\|Documentation\)/' | \ > > > while read i; do perl -i -pe 's/%pf/%ps/g; s/%pF/%pS/g;' $i; done > > > > > > And verifying the result. > > > > I guess that the best timing for such tree-wide clean up is the end > > of the merge window. Should we wait for 5.2 or is it still acceptable > > to push this for 5.1-rc3? > > The patch still cleanly applies to linux-next as wells as Linus's tree. > Some %pf bits have appeared and fixed since (include/trace/events/timer.h); > the fix is in linux-next so once that and this patch are merged, there are > no remaining %pf (or %pF) users left. I have pushed the patch into printk.git, branch for-5.2-pf-removal. It is v2 without the include/trace/events/timer.h stuff. Best Regards, Petr
Re: 答复: Re: 答复: Re: [PATCH v2] scsi: Introduce sdev_printk_ratelimited tothrottlefrequent printk
On Mon 2018-04-09 10:13:43, wen.yan...@zte.com.cn wrote: > That's a good idea, but it only solves part of the problem. > loopping printks under spinlock, there's two path: > one path is: > scsi_request_fn --> loop -> blk_peek_request-> scsi_prep_fn -> > scsi_prep_state_check -> sdev_printk > another path is: > scsi_request_fn --> loop -> sdev_printk Is this message redundant? It seems to be printed in the same situations as the messages in scsi_prep_state_check(). In fact, there seems to be a mismatch. scsi_request_fn() prints about offline device also for sdev->sdev_state == SDEV_DEL. While scsi_prep_state_check() prints about a dead device in this case. Would it make sense to remove the redundant dev_printk() from scsi_request_fn()? Then we could add a flag into struct scsi_device that would remember if we already printed the error in scsi_prep_state_check() for the given device. It could be used to print the error only once. The flag might be scsi_device_state value for which we printed the error last time. We would need to reset it scsi_prep_state_check() see another state. It is a bit hairy but it really does not make much sense to print the same error message thousand times. Best Regards, Petr PS: Your mail was again strangely formatted. Please, send mails in plain text format (no html).
Re: 答复: Re: [PATCH v2] scsi: Introduce sdev_printk_ratelimited to throttlefrequent printk
On Fri 2018-04-06 10:30:16, Petr Mladek wrote:
> On Tue 2018-04-03 14:19:43, wen.yan...@zte.com.cn wrote:
> > On the other hand，queue_lock is big, looping doing something under spinlock
> >
> > may locked many things and taking a long time, may cause some problems.
> >
> > So This code needs to be optimized later:
> >
> > scsi_request_fn()
> > {
> > for (;;) {
> > int rtn;
> > /*
> > * get next queueable request. We do this early to make sure
> > * that the request is fully prepared even if we cannot
> > * accept it.
> > */
> >
> > req = blk_peek_request(q);
> >
> > if (!req)
> > break;
> >
> > if (unlikely(!scsi_device_online(sdev))) {
> > sdev_printk(KERN_ERR, sdev,
> > "rejected I/O to offline device\n");
> > scsi_kill_request(req, q);
> > continue;
> >
> > ^ still under spinlock
> > }
>
> I wonder if the following might be the best solution after all:
>
> if (unlikely(!scsi_device_online(sdev))) {
> scsi_kill_request(req, q);
>
> /*
>* printk() might take a while on slow consoles.
>* Prevent solftlockups by releasing the lock.
>*/
> spin_unlock_irq(q->queue_lock);
> sdev_printk(KERN_ERR, sdev,
> "rejecting I/O to offline device\n");
> spin_lock_irq(q->queue_lock);
> continue;
> }
>
> I see that the lock is released also in several other situations.
> Therefore it looks safe. Also handling too many requests without
> releasing the lock seems to be a bad idea in general. I think
> that this solution was already suggested earlier.
Just to be sure. Is it safe to kill first few requests and proceed
the others?
I wonder if the device could actually get online without releasing
the queue lock. If not, we normally killed all requests.
I wonder if a local flag might actually help to reduce the number
of messages but keep the existing behavior. I mean something like
static void scsi_request_fn(struct request_queue *q)
{
struct scsi_device *sdev = q->queuedata;
^
The device is the same for each request
in this queue.
struct request *req;
+ bool offline_reported = false;
/*
* To start with, we keep looping until the queue is empty, or until
* the host is no longer able to accept any more requests.
*/
shost = sdev->host;
for (;;) {
int rtn;
req = blk_peek_request(q);
if (!req)
break;
if (unlikely(!scsi_device_online(sdev))) {
+ if (!offline_reported) {
sdev_printk(KERN_ERR, sdev,
"rejecting I/O to offline device\n");
+ offline_reported = true;
+ }
scsi_kill_request(req, q);
continue;
}
Please, note that I am not familiar with the scsi code. I am involved
because this is printk related. Unfortunately, we could not make
printk() faster. The main principle is to get messages on the console
ASAP. Nobody knows when the system might die and any message might
be important.
Best Regards,
Petr
Re: 答复: Re: [PATCH v2] scsi: Introduce sdev_printk_ratelimited to throttlefrequent printk
On Tue 2018-04-03 14:19:43, wen.yan...@zte.com.cn wrote:
> On the other hand，queue_lock is big, looping doing something under spinlock
>
> may locked many things and taking a long time, may cause some problems.
>
> So This code needs to be optimized later:
>
> scsi_request_fn()
> {
> for (;;) {
> int rtn;
> /*
>* get next queueable request. We do this early to make sure
>* that the request is fully prepared even if we cannot
>* accept it.
>*/
>
> req = blk_peek_request(q);
>
> if (!req)
> break;
>
> if (unlikely(!scsi_device_online(sdev))) {
> sdev_printk(KERN_ERR, sdev,
> "rejected I/O to offline device\n");
> scsi_kill_request(req, q);
> continue;
>
> ^ still under spinlock
> }
I wonder if the following might be the best solution after all:
if (unlikely(!scsi_device_online(sdev))) {
scsi_kill_request(req, q);
/*
* printk() might take a while on slow consoles.
* Prevent solftlockups by releasing the lock.
*/
spin_unlock_irq(q->queue_lock);
sdev_printk(KERN_ERR, sdev,
"rejecting I/O to offline device\n");
spin_lock_irq(q->queue_lock);
continue;
}
I see that the lock is released also in several other situations.
Therefore it looks safe. Also handling too many requests without
releasing the lock seems to be a bad idea in general. I think
that this solution was already suggested earlier.
Please, note that I moved scsi_kill_request() up. It looks natural
to remove it from the queue before we release the queue lock.
Best Regards,
Petr
BTW: Your mail had strange formatting. Please, try to avoid using
html.
Re: [PATCH v3] scsi: Introduce sdev_printk_ratelimited to throttle frequent printk
On Tue 2018-04-03 14:04:40, Wen Yang wrote:
> There would be so many same lines printed by frequent printk if one
> disk went wrong, like,
> [ 546.185242] sd 0:1:0:0: rejecting I/O to offline device
> [ 546.185258] sd 0:1:0:0: rejecting I/O to offline device
> [ 546.185280] sd 0:1:0:0: rejecting I/O to offline device
> [ 546.185307] sd 0:1:0:0: rejecting I/O to offline device
> [ 546.185334] sd 0:1:0:0: rejecting I/O to offline device
> [ 546.185364] sd 0:1:0:0: rejecting I/O to offline device
> [ 546.185390] sd 0:1:0:0: rejecting I/O to offline device
> [ 546.185410] sd 0:1:0:0: rejecting I/O to offline device
> For slow serial console, the frequent printk may be blocked for a
> long time, and if any spin_lock has been acquired before the printk
> like in scsi_request_fn, watchdog could be triggered.
>
> Related disscussion can be found here,
> https://bugzilla.kernel.org/show_bug.cgi?id=199003
> And Petr brought the idea to throttle the frequent printk, it's
> useless to print the same lines frequently after all.
>
> v2: fix some typos
> v3: limit the print only for the same device
>
> Suggested-by: Petr Mladek
> Suggested-by: Sergey Senozhatsky
> Signed-off-by: Wen Yang
> Signed-off-by: Jiang Biao
> Signed-off-by: Tan Hu
> Reviewed-by: Bart Van Assche
> CC: BartVanAssche
> CC: Petr Mladek
> CC: Sergey Senozhatsky
> CC: Martin K. Petersen
> CC: "James E.J. Bottomley"
> CC: Tejun Heo
> CC: JasonYan
> ---
> drivers/scsi/scsi_lib.c| 6 +++---
> drivers/scsi/scsi_scan.c | 3 +++
> include/scsi/scsi_device.h | 8
> 3 files changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index c84f931..f77e801 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1301,7 +1301,7 @@ static int scsi_setup_cmnd(struct scsi_device *sdev,
> struct request *req)
>* commands. The device must be brought online
>* before trying any recovery commands.
>*/
> - sdev_printk(KERN_ERR, sdev,
> + sdev_printk_ratelimited(KERN_ERR, sdev,
> "rejecting I/O to offline device\n");
> ret = BLKPREP_KILL;
> break;
> @@ -1310,7 +1310,7 @@ static int scsi_setup_cmnd(struct scsi_device *sdev,
> struct request *req)
>* If the device is fully deleted, we refuse to
>* process any commands as well.
>*/
> - sdev_printk(KERN_ERR, sdev,
> + sdev_printk_ratelimited(KERN_ERR, sdev,
> "rejecting I/O to dead device\n");
> ret = BLKPREP_KILL;
> break;
> @@ -1802,7 +1802,7 @@ static void scsi_request_fn(struct request_queue *q)
> break;
>
> if (unlikely(!scsi_device_online(sdev))) {
> - sdev_printk(KERN_ERR, sdev,
> + sdev_printk_ratelimited(KERN_ERR, sdev,
> "rejecting I/O to offline device\n");
> scsi_kill_request(req, q);
> continue;
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index 0880d97..a6da935 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -288,6 +288,9 @@ static struct scsi_device *scsi_alloc_sdev(struct
> scsi_target *starget,
> scsi_change_queue_depth(sdev, sdev->host->cmd_per_lun ?
> sdev->host->cmd_per_lun : 1);
>
> + /* Enable message ratelimiting. Default is 10 messages per 5 secs. */
> + ratelimit_state_init(&sdev->sdev_ratelimit_state,
> + DEFAULT_RATELIMIT_INTERVAL, DEFAULT_RATELIMIT_BURST);
This makes the ratelimiting device independent but it adds another
problem. Several unrelated messages share the ratelimit data now.
It means that cycling on one message might cause that people will
not see the others.
One question is if we really need to ratelimit all three messages.
Another question if we are really printing all the messages in
a single cycle without releasing the spin lock. Then I wonder what
event will cause that the cycle finishes. If the event is independent
then ratelimiting the messages need not help to avoid the softlockup.
I mean that we might cycle faster without the printk but it does
not mean the event would unblock the cycle faster.
Best Regards,
Petr
> scsi_sysfs_device_initialize(sdev);
>
> if
Re: [PATCH 12/13] kthread: Convert callback to use from_timer()
On Wed 2017-10-04 16:27:06, Kees Cook wrote: > In preparation for unconditionally passing the struct timer_list pointer > to all timer callbacks, switch kthread to use from_timer() and pass the > timer pointer explicitly. > > Cc: Andrew Morton > Cc: Petr Mladek > Cc: Tejun Heo > Cc: Thomas Gleixner > Cc: Oleg Nesterov > Signed-off-by: Kees Cook Reviewed-by: Petr Mladek Best Regards, Petr
Re: [PATCH 11/13] timer: Remove expires argument from __TIMER_INITIALIZER()
On Wed 2017-10-04 16:27:05, Kees Cook wrote: > The expires field is normally initialized during the first mod_timer() > call. It was unused by all callers, so remove it from the macro. > > Signed-off-by: Kees Cook > --- > include/linux/kthread.h | 2 +- > include/linux/timer.h | 5 ++--- > include/linux/workqueue.h | 2 +- > 3 files changed, 4 insertions(+), 5 deletions(-) I was primary interested into the change in kthread.h. But the entire patch is simple and looks fine: Reviewed-by: Petr Mladek Best Regards, Petr
Re: [PATCH 27/38] Rearrange buffer formatting in printk()
On Tue 30-09-14 18:16:20, Petr Mladek wrote:
> On Mon 29-09-14 13:58:56, Hannes Reinecke wrote:
> > Move buffer formatting to the start of the function as it
> > doesn't require to be done under any locks.
> > No functional change, required by the next patch.
> >
> > Cc: Steven Rostedt
> > Cc: LKML
>
> printk stuff is maintained by Andrew in mm tree, so adding him into CC.
>
> > Signed-off-by: Hannes Reinecke
>
> Reviewed-by: Petr Mladek
I have just realized that textbuf[LOG_LINE_MAX] is a static variable.
It means that it must be used under the lock and this patch is wrong.
I want to take back the Reviewed-by and instead do
Nacked->by: Petr Mladek
also it means that it does not take the extra space on the stack
and you probably does not need the two patches at all.
Best Regards,
Petr
> I do not see any obvious problem. Just note that
> [sched_delayed] has already been removed in -mm tree. You might
> want to base this patch on top of the commit 460d73c35ffa17979422290
> ("printk: git rid of [sched_delayed] message for printk_deferred")
> from linux-next.
>
> Best Regards,
> Petr
>
> > ---
> > kernel/printk/printk.c | 21 ++---
> > 1 file changed, 10 insertions(+), 11 deletions(-)
> >
> > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> > index 1ce7706..d13675e 100644
> > --- a/kernel/printk/printk.c
> > +++ b/kernel/printk/printk.c
> > @@ -1633,8 +1633,18 @@ asmlinkage int vprintk_emit(int facility, int level,
> > if (level == SCHED_MESSAGE_LOGLEVEL) {
> > level = -1;
> > in_sched = true;
> > +
> > + /*
> > +* The printf needs to come first; we need the syslog
> > +* prefix which might be passed-in as a parameter.
> > +*/
> > + text_len = scnprintf(text, sizeof(textbuf),
> > +KERN_WARNING "[sched_delayed] ");
> > }
> >
> > + text_len += vscnprintf(text + text_len,
> > + sizeof(textbuf) - text_len, fmt, args);
> > +
> > boot_delay_msec(level);
> > printk_delay();
> >
> > @@ -1676,17 +1686,6 @@ asmlinkage int vprintk_emit(int facility, int level,
> > strlen(recursion_msg));
> > }
> >
> > - /*
> > -* The printf needs to come first; we need the syslog
> > -* prefix which might be passed-in as a parameter.
> > -*/
> > - if (in_sched)
> > - text_len = scnprintf(text, sizeof(textbuf),
> > -KERN_WARNING "[sched_delayed] ");
> > -
> > - text_len += vscnprintf(text + text_len,
> > - sizeof(textbuf) - text_len, fmt, args);
> > -
> > /* mark and strip a trailing newline */
> > if (text_len && text[text_len-1] == '\n') {
> > text_len--;
> > --
> > 1.8.5.2
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majord...@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at http://www.tux.org/lkml/
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 28/38] Externalize string buffer for printk
On Mon 29-09-14 13:58:57, Hannes Reinecke wrote:
> This patch splits off the actual logging from vprintk_emit()
> into printk_emit_string(), with vprintk_emit() just being a
> simple wrapper for formatting the message into a
> static buffer.
>
> With that the caller can pass in a local buffer for
> printk_emit_string() without increasing the overall stack size.
>
> Cc: Steven Rostedt
> Cc: LKML
Adding Andrew into CC here as well.
There is one potential problem, see below.
> Signed-off-by: Hannes Reinecke
> ---
> include/linux/printk.h | 5 +
> kernel/printk/printk.c | 36
> 2 files changed, 29 insertions(+), 12 deletions(-)
>
> diff --git a/include/linux/printk.h b/include/linux/printk.h
> index d78125f..9639900 100644
> --- a/include/linux/printk.h
> +++ b/include/linux/printk.h
> @@ -130,6 +130,11 @@ int vprintk_emit(int facility, int level,
>const char *dict, size_t dictlen,
>const char *fmt, va_list args);
>
> +asmlinkage
> +int printk_emit_string(int facility, int level,
> +const char *dict, size_t dictlen,
> +char *textbuf, size_t text_len);
> +
> asmlinkage __printf(1, 0)
> int vprintk(const char *fmt, va_list args);
>
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index d13675e..303a1fe 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -1618,22 +1618,11 @@ asmlinkage int vprintk_emit(int facility, int level,
> const char *dict, size_t dictlen,
> const char *fmt, va_list args)
> {
> - static int recursion_bug;
> static char textbuf[LOG_LINE_MAX];
>
> char *text = textbuf;
> size_t text_len = 0;
> - enum log_flags lflags = 0;
> - unsigned long flags;
> - int this_cpu;
> - int printed_len = 0;
> - bool in_sched = false;
> - /* cpu currently holding logbuf_lock in this function */
> - static volatile unsigned int logbuf_cpu = UINT_MAX;
>
> if (level == SCHED_MESSAGE_LOGLEVEL) {
> - level = -1;
> - in_sched = true;
> -
> /*
>* The printf needs to come first; we need the syslog
>* prefix which might be passed-in as a parameter.
> @@ -1644,6 +1633,24 @@ asmlinkage int vprintk_emit(int facility, int level,
>
> text_len += vscnprintf(text + text_len,
> sizeof(textbuf) - text_len, fmt, args);
> + return printk_emit_string(facility, level, dict, dictlen,
> + textbuf, text_len);
> +}
> +EXPORT_SYMBOL(vprintk_emit);
> +
> +asmlinkage int printk_emit_string(int facility, int level,
> + const char *dict, size_t dictlen,
> + char *textbuf, size_t text_len)
> +{
> + static int recursion_bug;
> + char *text = textbuf;
> + enum log_flags lflags = 0;
> + unsigned long flags;
> + int this_cpu;
> + int printed_len = 0;
> + bool in_sched = false;
> + /* cpu currently holding logbuf_lock in this function */
> + static volatile unsigned int logbuf_cpu = UINT_MAX;
We should make sure that text_len is lower or equal LOG_LINE_MAX.
I am afraid that printk() code is not able to process longer
lines. For example, syslog_print() does:
text = kmalloc(LOG_LINE_MAX + PREFIX_MAX, GFP_KERNEL);
Then it calls msg_print_text() that works with entire messages. So,
any longer message would freeze syslog_print() because it would newer
fit into the buffer.
I guess that there are more locations like this.
Maybe, we should make LOG_LINE_MAX public, so it could be used on the
other location either to allocate the buffer or to check the size.
Best Regards,
Petr
> boot_delay_msec(level);
> printk_delay();
> @@ -1652,6 +1659,11 @@ asmlinkage int vprintk_emit(int facility, int level,
> local_irq_save(flags);
> this_cpu = smp_processor_id();
>
> + if (level == SCHED_MESSAGE_LOGLEVEL) {
> + level = -1;
> + in_sched = true;
> + }
> +
> /*
>* Ouch, printk recursed into itself!
>*/
> @@ -1789,7 +1801,7 @@ asmlinkage int vprintk_emit(int facility, int level,
>
> return printed_len;
> }
> -EXPORT_SYMBOL(vprintk_emit);
> +EXPORT_SYMBOL(printk_emit_string);
>
> asmlinkage int vprintk(const char *fmt, va_list args)
> {
> --
> 1.8.5.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 27/38] Rearrange buffer formatting in printk()
On Mon 29-09-14 13:58:56, Hannes Reinecke wrote:
> Move buffer formatting to the start of the function as it
> doesn't require to be done under any locks.
> No functional change, required by the next patch.
>
> Cc: Steven Rostedt
> Cc: LKML
printk stuff is maintained by Andrew in mm tree, so adding him into CC.
> Signed-off-by: Hannes Reinecke
Reviewed-by: Petr Mladek
I do not see any obvious problem. Just note that
[sched_delayed] has already been removed in -mm tree. You might
want to base this patch on top of the commit 460d73c35ffa17979422290
("printk: git rid of [sched_delayed] message for printk_deferred")
from linux-next.
Best Regards,
Petr
> ---
> kernel/printk/printk.c | 21 ++---
> 1 file changed, 10 insertions(+), 11 deletions(-)
>
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 1ce7706..d13675e 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -1633,8 +1633,18 @@ asmlinkage int vprintk_emit(int facility, int level,
> if (level == SCHED_MESSAGE_LOGLEVEL) {
> level = -1;
> in_sched = true;
> +
> + /*
> + * The printf needs to come first; we need the syslog
> + * prefix which might be passed-in as a parameter.
> + */
> + text_len = scnprintf(text, sizeof(textbuf),
> + KERN_WARNING "[sched_delayed] ");
> }
>
> + text_len += vscnprintf(text + text_len,
> +sizeof(textbuf) - text_len, fmt, args);
> +
> boot_delay_msec(level);
> printk_delay();
>
> @@ -1676,17 +1686,6 @@ asmlinkage int vprintk_emit(int facility, int level,
>strlen(recursion_msg));
> }
>
> - /*
> - * The printf needs to come first; we need the syslog
> - * prefix which might be passed-in as a parameter.
> - */
> - if (in_sched)
> - text_len = scnprintf(text, sizeof(textbuf),
> - KERN_WARNING "[sched_delayed] ");
> -
> - text_len += vscnprintf(text + text_len,
> -sizeof(textbuf) - text_len, fmt, args);
> -
> /* mark and strip a trailing newline */
> if (text_len && text[text_len-1] == '\n') {
> text_len--;
> --
> 1.8.5.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
