public inbox for ltp@lists.linux.it
 help / color / mirror / Atom feed
From: Cyril Hrubis <chrubis@suse.cz>
To: ltp@lists.linux.it
Subject: [LTP] [PATCH v2] kernel/irq: Add irqbalance01
Date: Thu, 9 Sep 2021 14:41:38 +0200	[thread overview]
Message-ID: <YToBAshr0Y94qcVU@yuki> (raw)
In-Reply-To: <20210831114406.8527-1-rpalethorpe@suse.com>

Hi!
> +	/* Read the CPU affinity masks for each IRQ. See bitmap_string() in the kernel (%*pb) */
> +	for (row = 0; row < nr_irqs; row++) {
> +		sprintf(path, "/proc/irq/%u/smp_affinity", irq_ids[row]);
> +		ret = SAFE_FILE_READAT(0, path, buf, size);
> +		if (ret < 1)
> +			tst_brk(TBROK, "Empty /proc/irq/%u/smp_affinity?", irq_ids[row]);
> +		c = buf;
> +		col = 0;
> +
> +		while (*c != '\0') {
> +			switch (*c) {
> +			case '\n':
> +			case ' ':
> +			case ',':
> +				c++;
> +				continue;
> +			case '0' ... '9':
> +				acc = *c - '0';
> +				break;
> +			case 'a' ... 'f':
> +				acc = 10 + *c - 'a';
> +				break;
> +			default:
> +				tst_res(TINFO, "%u/smp_affnity: %s", irq_ids[row], buf);
> +				tst_brk(TBROK, "Wasn't expecting 0x%02x", *c);
> +			}
> +
> +			for (mask_pos = 0; mask_pos < 4; mask_pos++) {
> +				if (mask_pos + col >= nr_cpus)
> +					break;
> +
> +				irq_affinity[row * nr_cpus + (nr_cpus - 1 - col - mask_pos)] =
> +					(acc & (8 >> mask_pos)) ? ALLOW : DENY;
> +			}

The mask here is actually wrong, since we go backward in the
irq_affinity array the bitmask has to grow not shrink so this should be
(acc & (1<<mask_pos))

> +			col += mask_pos;
> +			c++;
> +		}
> +	}
> +
> +	free(buf);
> +}
> +
> +static void print_irq_info(void)
> +{
> +	size_t row, col;
> +	unsigned int count;
> +	enum affinity aff;

It would be nice if we printed the CPUX header here as well so that it's
clear which column describes which CPU.

> +	for (row = 0; row < nr_irqs; row++) {
> +		tst_printf("%5u:", irq_ids[row]);
> +
> +		for (col = 0; col < nr_cpus; col++) {
> +			count = irq_stats[row * nr_cpus + col];
> +			aff = irq_affinity[row * nr_cpus + col] == ALLOW ? '+' : '-';
> +
> +			tst_printf("%10u%c", count, aff);
> +		}
> +
> +		tst_printf("\n");
> +	}
> +
> +	tst_printf("Total:");
> +
> +	for (col = 0; col < nr_cpus; col++)
> +		tst_printf("%11u", irq_stats[row * nr_cpus + col]);
                               ^
			       And I would change this to "%10u " so
			       that the value aligns with the rest of
			       the numbers in the columns.
> +	tst_printf("\n");
> +}

Otherwise the rest looks good.

-- 
Cyril Hrubis
chrubis@suse.cz

WARNING: multiple messages have this Message-ID (diff)
From: Cyril Hrubis <chrubis@suse.cz>
To: Richard Palethorpe <rpalethorpe@suse.com>
Cc: nhorman@gmail.com, Thomas Gleixner <tglx@linutronix.de>,
	anton@deadbeef.mx, ltp@lists.linux.it
Subject: Re: [LTP] [PATCH v2] kernel/irq: Add irqbalance01
Date: Thu, 9 Sep 2021 14:41:38 +0200	[thread overview]
Message-ID: <YToBAshr0Y94qcVU@yuki> (raw)
Message-ID: <20210909124138.XKDbGIN1Xrju8e_jPUtYgRyD_ZGTTmMdAr1oup-8CUI@z> (raw)
In-Reply-To: <20210831114406.8527-1-rpalethorpe@suse.com>

Hi!
> +	/* Read the CPU affinity masks for each IRQ. See bitmap_string() in the kernel (%*pb) */
> +	for (row = 0; row < nr_irqs; row++) {
> +		sprintf(path, "/proc/irq/%u/smp_affinity", irq_ids[row]);
> +		ret = SAFE_FILE_READAT(0, path, buf, size);
> +		if (ret < 1)
> +			tst_brk(TBROK, "Empty /proc/irq/%u/smp_affinity?", irq_ids[row]);
> +		c = buf;
> +		col = 0;
> +
> +		while (*c != '\0') {
> +			switch (*c) {
> +			case '\n':
> +			case ' ':
> +			case ',':
> +				c++;
> +				continue;
> +			case '0' ... '9':
> +				acc = *c - '0';
> +				break;
> +			case 'a' ... 'f':
> +				acc = 10 + *c - 'a';
> +				break;
> +			default:
> +				tst_res(TINFO, "%u/smp_affnity: %s", irq_ids[row], buf);
> +				tst_brk(TBROK, "Wasn't expecting 0x%02x", *c);
> +			}
> +
> +			for (mask_pos = 0; mask_pos < 4; mask_pos++) {
> +				if (mask_pos + col >= nr_cpus)
> +					break;
> +
> +				irq_affinity[row * nr_cpus + (nr_cpus - 1 - col - mask_pos)] =
> +					(acc & (8 >> mask_pos)) ? ALLOW : DENY;
> +			}

The mask here is actually wrong, since we go backward in the
irq_affinity array the bitmask has to grow not shrink so this should be
(acc & (1<<mask_pos))

> +			col += mask_pos;
> +			c++;
> +		}
> +	}
> +
> +	free(buf);
> +}
> +
> +static void print_irq_info(void)
> +{
> +	size_t row, col;
> +	unsigned int count;
> +	enum affinity aff;

It would be nice if we printed the CPUX header here as well so that it's
clear which column describes which CPU.

> +	for (row = 0; row < nr_irqs; row++) {
> +		tst_printf("%5u:", irq_ids[row]);
> +
> +		for (col = 0; col < nr_cpus; col++) {
> +			count = irq_stats[row * nr_cpus + col];
> +			aff = irq_affinity[row * nr_cpus + col] == ALLOW ? '+' : '-';
> +
> +			tst_printf("%10u%c", count, aff);
> +		}
> +
> +		tst_printf("\n");
> +	}
> +
> +	tst_printf("Total:");
> +
> +	for (col = 0; col < nr_cpus; col++)
> +		tst_printf("%11u", irq_stats[row * nr_cpus + col]);
                               ^
			       And I would change this to "%10u " so
			       that the value aligns with the rest of
			       the numbers in the columns.
> +	tst_printf("\n");
> +}

Otherwise the rest looks good.

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

  parent reply	other threads:[~2021-09-09 12:41 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-24 10:10 [LTP] [PATCH] kernel/irq: Add irqbalance01 Richard Palethorpe
2021-08-30 19:26 ` Petr Vorel
2021-08-31  9:39   ` Richard Palethorpe
2021-08-31 11:44     ` [LTP] [PATCH v2] " Richard Palethorpe
2021-08-31 12:01       ` Petr Vorel
2021-09-09 12:41       ` Cyril Hrubis [this message]
2021-09-09 12:41         ` Cyril Hrubis
2021-09-09 14:09       ` Cyril Hrubis
2021-09-09 14:09         ` Cyril Hrubis
     [not found] <20210909140911.44EC9A4308@relay2.suse.de>
2021-09-16 12:32 ` Richard Palethorpe
2021-09-16 12:32   ` Richard Palethorpe via ltp
2021-09-16 20:55   ` Petr Vorel
2021-09-16 20:55     ` Petr Vorel
2021-10-08  8:23   ` Cyril Hrubis
2021-10-11  9:22     ` Richard Palethorpe

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YToBAshr0Y94qcVU@yuki \
    --to=chrubis@suse.cz \
    --cc=ltp@lists.linux.it \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox