[PATCHv2,2/3] x86, ptdump: Simplify page flag evaluation code
diff mbox series

Message ID 1411313216-2641-3-git-send-email-minipli@googlemail.com
State New, archived
Headers show
Series
  • x86, ptdump: a EFI related fix + enhancements
Related show

Commit Message

Mathias Krause Sept. 21, 2014, 3:26 p.m. UTC
The code evaluating the page flags is rather scattered. Simplify it by
folding the 'if .. else ..' part into the actual print call. Make use of
appropriate format strings to get the desired string width.

Also change the pt_dump_seq_printf() and pt_dump_cont_printf() macros to
use the common 'do ... while(0)' pattern instead of a compound statement
expression. We don't need no expression, just the statement.

Last, but not least, fix a few checkpatch warnings for the lines
touched.

Signed-off-by: Mathias Krause <minipli@googlemail.com>
Cc: Arjan van de Ven <arjan@linux.intel.com>
Cc: H. Peter Anvin <hpa@zytor.com>
---
v2: - re-add pt_dump prefix to macros (and ignore checkpatch warnings) as
      suggested by Ingo

 arch/x86/mm/dump_pagetables.c |  107 ++++++++++++---------------------
 1 file changed, 40 insertions(+), 67 deletions(-)

Comments

Arjan van de Ven Sept. 21, 2014, 7:49 p.m. UTC | #1
On 9/21/2014 8:26 AM, Mathias Krause wrote:
> -		if (pr & _PAGE_PCD)
> -			pt_dump_cont_printf(m, dmsg, "PCD ");
> -		else
> -			pt_dump_cont_printf(m, dmsg, "    ");
> +		pt_dump_cont(m, dmsg, "%-4s", pr & _PAGE_USER ? "USR" : "");

while you have some nice cleanups in your patch, I can't say I consider this an improvement.
Yes the C standard allows ? to be used like this
but no, I don't think it improves readability in general.
(I think for me the main exception is NULL pointer cases, but this is not one of these)


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Mathias Krause Sept. 21, 2014, 8:33 p.m. UTC | #2
On 21 September 2014 21:49, Arjan van de Ven <arjan@linux.intel.com> wrote:
> On 9/21/2014 8:26 AM, Mathias Krause wrote:
>>
>> -               if (pr & _PAGE_PCD)
>> -                       pt_dump_cont_printf(m, dmsg, "PCD ");
>> -               else
>> -                       pt_dump_cont_printf(m, dmsg, "    ");
>> +               pt_dump_cont(m, dmsg, "%-4s", pr & _PAGE_USER ? "USR" :
>> "");
>
>
> while you have some nice cleanups in your patch, I can't say I consider this
> an improvement.
> Yes the C standard allows ? to be used like this
> but no, I don't think it improves readability in general.

Not in general, but in this case, it does, IMHO.

> (I think for me the main exception is NULL pointer cases, but this is not
> one of these)

Apparently such a pattern (using the question mark operator combined
with a bit test to choose string constants) is used quite often in the
linux kernel as a simple grep tells me (probably catches a few false
positives but still a representative number):

$ git grep '[^&]&[^&].*? *"' | wc -l
2668

And, honestly, the bit test combined with the question mark operator
makes the code way more readable for me. It not only makes the code
more compact (1 instead of 4 lines). It also allows to have the common
parts written only once and thereby removing the possibility of having
them conflict with each other, e.g. make them generate strings of
different lengths.

Would you prefer the bit test to be surrounded by braces to make it
more easy to understand? Even though, the operator precedence is
clearly defined by the C standard for this case, so no braces are
needed.


Thanks,
Mathias
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Ingo Molnar Sept. 24, 2014, 7:45 a.m. UTC | #3
* Mathias Krause <minipli@googlemail.com> wrote:

> On 21 September 2014 21:49, Arjan van de Ven <arjan@linux.intel.com> wrote:
> > On 9/21/2014 8:26 AM, Mathias Krause wrote:
> >>
> >> -               if (pr & _PAGE_PCD)
> >> -                       pt_dump_cont_printf(m, dmsg, "PCD ");
> >> -               else
> >> -                       pt_dump_cont_printf(m, dmsg, "    ");
> >> +               pt_dump_cont(m, dmsg, "%-4s", pr & _PAGE_USER ? "USR" :
> >> "");
> >
> >
> > while you have some nice cleanups in your patch, I can't say I consider this
> > an improvement.
> > Yes the C standard allows ? to be used like this
> > but no, I don't think it improves readability in general.
> 
> Not in general, but in this case, it does, IMHO.
> 
> > (I think for me the main exception is NULL pointer cases, but this is not
> > one of these)
> 
> Apparently such a pattern (using the question mark operator combined
> with a bit test to choose string constants) is used quite often in the
> linux kernel as a simple grep tells me (probably catches a few false
> positives but still a representative number):

Both can be used (although I too find the original version easier 
to read), and it's usually the taste/opinion of the original 
author whose choice we prefer.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Mathias Krause Sept. 25, 2014, 7:27 p.m. UTC | #4
On 24 September 2014 09:45, Ingo Molnar <mingo@kernel.org> wrote:
> * Mathias Krause <minipli@googlemail.com> wrote:
>> On 21 September 2014 21:49, Arjan van de Ven <arjan@linux.intel.com> wrote:
>> > On 9/21/2014 8:26 AM, Mathias Krause wrote:
>> >>
>> >> -               if (pr & _PAGE_PCD)
>> >> -                       pt_dump_cont_printf(m, dmsg, "PCD ");
>> >> -               else
>> >> -                       pt_dump_cont_printf(m, dmsg, "    ");
>> >> +               pt_dump_cont(m, dmsg, "%-4s", pr & _PAGE_USER ? "USR" :
>> >> "");
>> >
>> >
>> > while you have some nice cleanups in your patch, I can't say I consider this
>> > an improvement.
>> > Yes the C standard allows ? to be used like this
>> > but no, I don't think it improves readability in general.
>>
>> Not in general, but in this case, it does, IMHO.
>>
>> > (I think for me the main exception is NULL pointer cases, but this is not
>> > one of these)
>>
>> Apparently such a pattern (using the question mark operator combined
>> with a bit test to choose string constants) is used quite often in the
>> linux kernel as a simple grep tells me (probably catches a few false
>> positives but still a representative number):
>
> Both can be used (although I too find the original version easier
> to read), and it's usually the taste/opinion of the original
> author whose choice we prefer.

So I should start writing more code from the scratch than changing
others... ;)

But concerning this patch, are you interested in the following other
pieces:
- changing the macros from being a compound statement expression to
  the common 'do .. while(0)' pattern
- use of pr_info()/pr_cont() instead of printk(KERN_INFO/KERN_CONT)
- use of format strings in pt_dump_cont_printf() calls
- removing the trailing blank before '\n' in the "... # entries
  skipped ..." message
...or should I just drop patch 2 altogether?


Thanks,
Mathias
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Ingo Molnar Sept. 26, 2014, 9:25 a.m. UTC | #5
* Mathias Krause <minipli@googlemail.com> wrote:

> On 24 September 2014 09:45, Ingo Molnar <mingo@kernel.org> wrote:
> > * Mathias Krause <minipli@googlemail.com> wrote:
> >> On 21 September 2014 21:49, Arjan van de Ven <arjan@linux.intel.com> wrote:
> >> > On 9/21/2014 8:26 AM, Mathias Krause wrote:
> >> >>
> >> >> -               if (pr & _PAGE_PCD)
> >> >> -                       pt_dump_cont_printf(m, dmsg, "PCD ");
> >> >> -               else
> >> >> -                       pt_dump_cont_printf(m, dmsg, "    ");
> >> >> +               pt_dump_cont(m, dmsg, "%-4s", pr & _PAGE_USER ? "USR" :
> >> >> "");
> >> >
> >> >
> >> > while you have some nice cleanups in your patch, I can't say I consider this
> >> > an improvement.
> >> > Yes the C standard allows ? to be used like this
> >> > but no, I don't think it improves readability in general.
> >>
> >> Not in general, but in this case, it does, IMHO.
> >>
> >> > (I think for me the main exception is NULL pointer cases, but this is not
> >> > one of these)
> >>
> >> Apparently such a pattern (using the question mark operator combined
> >> with a bit test to choose string constants) is used quite often in the
> >> linux kernel as a simple grep tells me (probably catches a few false
> >> positives but still a representative number):
> >
> > Both can be used (although I too find the original version easier
> > to read), and it's usually the taste/opinion of the original
> > author whose choice we prefer.
> 
> So I should start writing more code from the scratch than changing
> others... ;)
> 
> But concerning this patch, are you interested in the following other
> pieces:
> - changing the macros from being a compound statement expression to
>   the common 'do .. while(0)' pattern
> - use of pr_info()/pr_cont() instead of printk(KERN_INFO/KERN_CONT)
> - use of format strings in pt_dump_cont_printf() calls
> - removing the trailing blank before '\n' in the "... # entries
>   skipped ..." message
> ...or should I just drop patch 2 altogether?

Honestly? I'm not interested at all in self-centered make-work 
cleanups that don't really improve the code materially - I'm more 
interested in cleanups that are a side effect of real work and 
real interest.

There's literally 1 million checkpatch 'failures' in the kernel 
source code, do we really want to clean them all up, waste 
reviewer and maintainer bandwidth and pretend that those 1 
million extra cleanup commits are just as valuable as the 'other' 
work that goes on in the kernel?

Why don't you do something different: for example something 
functionally useful (some real improvements to the code!), and 
make sure it's all clean while you are doing it? That gives an 
opportunity to clean up anything that your changes touch as well. 

We always welcome cleanups that are a side effect of feature 
work. Cleanups for code that is perfectly readable, just for 
cleanup's sake, is counterproductive ...

Doing printk() formatting fixes is fine for a newbie, but as I 
told Perches a couple of times already, and as I told Bunk before 
him, most kernel developers grow beyond printk() fixes after 
their first few commits.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Borislav Petkov Sept. 26, 2014, 10:11 a.m. UTC | #6
On Fri, Sep 26, 2014 at 11:25:39AM +0200, Ingo Molnar wrote:
> We always welcome cleanups that are a side effect of feature 
> work. Cleanups for code that is perfectly readable, just for 
> cleanup's sake, is counterproductive ...

Greg says there's enough work to be done in the staging drivers if
people wanna. :-)
Ingo Molnar Sept. 26, 2014, 11:15 a.m. UTC | #7
* Borislav Petkov <bp@alien8.de> wrote:

> On Fri, Sep 26, 2014 at 11:25:39AM +0200, Ingo Molnar wrote:
>
> > We always welcome cleanups that are a side effect of feature 
> > work. Cleanups for code that is perfectly readable, just for 
> > cleanup's sake, is counterproductive ...
> 
> Greg says there's enough work to be done in the staging drivers 
> if people wanna. :-)

Yeah, and drivers/staging/ is often unreadable code, so doing 
cleanups there is inherently useful.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Mathias Krause Sept. 26, 2014, 12:35 p.m. UTC | #8
On 26 September 2014 11:25, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Mathias Krause <minipli@googlemail.com> wrote:
>
>> On 24 September 2014 09:45, Ingo Molnar <mingo@kernel.org> wrote:
>> > * Mathias Krause <minipli@googlemail.com> wrote:
>> >> On 21 September 2014 21:49, Arjan van de Ven <arjan@linux.intel.com> wrote:
>> >> > On 9/21/2014 8:26 AM, Mathias Krause wrote:
>> >> >>
>> >> >> -               if (pr & _PAGE_PCD)
>> >> >> -                       pt_dump_cont_printf(m, dmsg, "PCD ");
>> >> >> -               else
>> >> >> -                       pt_dump_cont_printf(m, dmsg, "    ");
>> >> >> +               pt_dump_cont(m, dmsg, "%-4s", pr & _PAGE_USER ? "USR" :
>> >> >> "");
>> >> >
>> >> >
>> >> > while you have some nice cleanups in your patch, I can't say I consider this
>> >> > an improvement.
>> >> > Yes the C standard allows ? to be used like this
>> >> > but no, I don't think it improves readability in general.
>> >>
>> >> Not in general, but in this case, it does, IMHO.
>> >>
>> >> > (I think for me the main exception is NULL pointer cases, but this is not
>> >> > one of these)
>> >>
>> >> Apparently such a pattern (using the question mark operator combined
>> >> with a bit test to choose string constants) is used quite often in the
>> >> linux kernel as a simple grep tells me (probably catches a few false
>> >> positives but still a representative number):
>> >
>> > Both can be used (although I too find the original version easier
>> > to read), and it's usually the taste/opinion of the original
>> > author whose choice we prefer.
>>
>> So I should start writing more code from the scratch than changing
>> others... ;)
>>
>> But concerning this patch, are you interested in the following other
>> pieces:
>> - changing the macros from being a compound statement expression to
>>   the common 'do .. while(0)' pattern
>> - use of pr_info()/pr_cont() instead of printk(KERN_INFO/KERN_CONT)
>> - use of format strings in pt_dump_cont_printf() calls
>> - removing the trailing blank before '\n' in the "... # entries
>>   skipped ..." message
>> ...or should I just drop patch 2 altogether?
>
> Honestly? I'm not interested at all in self-centered make-work
> cleanups that don't really improve the code materially - I'm more
> interested in cleanups that are a side effect of real work and
> real interest.

That's exactly what this patch was all about. A cleanup patch in the
middle of a series fixing one issue (patch 1) and extending the
functionality (patch 3). The latter was touching the code this patch
tries to clean-up. At least for *me* it makes the code more readable
as it's now more compact -- one print call per flag. Others may have
different opinions which is perfectly fine. But still, why do think
this is "self-centered make-work"?

To recap, patch 1 makes the EFI runtime mappings visible again, commit
3891a04aafd6 broke. I really *do* want to see, that those are RWX
mapped into the kernel address space, so this nastiness is visible --
not wiped under the carpet. It's a nice little attack surface for all
those SMEP enabled systems, attackers are probably aware of but, after
commit 3891a04aafd6, we're not so much any more. While trying to do
something about it (playing with protection flags on the PGD level) I
noticed the page table dump code did not honor those. So I fixed that
too in patch 3. An improvement to the code I immediately could make
use of.

>
> There's literally 1 million checkpatch 'failures' in the kernel
> source code, do we really want to clean them all up, waste
> reviewer and maintainer bandwidth and pretend that those 1
> million extra cleanup commits are just as valuable as the 'other'
> work that goes on in the kernel?

I don't think the time is wasted if the end result is cleaner and more
readable code. If those commits are as valuable as "real" commits? I
don't think so either. But they still have value -- making code more
readable and fixing small bugs.

>
> Why don't you do something different: for example something
> functionally useful (some real improvements to the code!), and
> make sure it's all clean while you are doing it? That gives an
> opportunity to clean up anything that your changes touch as well.

I though that's what I have done in this series, as explained above.
Why do you think it does not?

Fiddling with EFI is a PITA, I can tell you. So having tools
supporting me while debugging issues I have is valuable. I thought, it
might be valueable for others, too. Why do you think this is not the
case?

>
> We always welcome cleanups that are a side effect of feature
> work. Cleanups for code that is perfectly readable, just for
> cleanup's sake, is counterproductive ...
>
> Doing printk() formatting fixes is fine for a newbie, but as I
> told Perches a couple of times already, and as I told Bunk before
> him, most kernel developers grow beyond printk() fixes after
> their first few commits.

Thanks, I'm fine. I've contributed other, imho valuable things in the
past already. But I'm no "professional" kernel developer whatsoever.
I'm doing most of this in my spare time. It's a hobby of mine. Nothing
more.


Thanks,
Mathias

>
> Thanks,
>
>         Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Patch
diff mbox series

diff --git a/arch/x86/mm/dump_pagetables.c b/arch/x86/mm/dump_pagetables.c
index 1a8053d1012e..0c3680332fcc 100644
--- a/arch/x86/mm/dump_pagetables.c
+++ b/arch/x86/mm/dump_pagetables.c
@@ -100,23 +100,23 @@  static struct addr_marker address_markers[] = {
 #define PUD_LEVEL_MULT (PTRS_PER_PMD * PMD_LEVEL_MULT)
 #define PGD_LEVEL_MULT (PTRS_PER_PUD * PUD_LEVEL_MULT)
 
-#define pt_dump_seq_printf(m, to_dmesg, fmt, args...)		\
-({								\
-	if (to_dmesg)					\
-		printk(KERN_INFO fmt, ##args);			\
-	else							\
-		if (m)						\
-			seq_printf(m, fmt, ##args);		\
-})
-
-#define pt_dump_cont_printf(m, to_dmesg, fmt, args...)		\
-({								\
-	if (to_dmesg)					\
-		printk(KERN_CONT fmt, ##args);			\
-	else							\
-		if (m)						\
-			seq_printf(m, fmt, ##args);		\
-})
+#define pt_dump_print(m, to_dmesg, fmt, args...)		\
+	do {							\
+		if (to_dmesg)					\
+			pr_info(fmt, ##args);			\
+		else						\
+			if (m)					\
+				seq_printf(m, fmt, ##args);	\
+	} while (0)
+
+#define pt_dump_cont(m, to_dmesg, fmt, args...)			\
+	do {							\
+		if (to_dmesg)					\
+			pr_cont(fmt, ##args);			\
+		else						\
+			if (m)					\
+				seq_printf(m, fmt, ##args);	\
+	} while (0)
 
 /*
  * Print a readable form of a pgprot_t to the seq_file
@@ -129,47 +129,23 @@  static void printk_prot(struct seq_file *m, pgprot_t prot, int level, bool dmsg)
 
 	if (!pgprot_val(prot)) {
 		/* Not present */
-		pt_dump_cont_printf(m, dmsg, "                          ");
+		pt_dump_cont(m, dmsg, "%-26s", "");
 	} else {
-		if (pr & _PAGE_USER)
-			pt_dump_cont_printf(m, dmsg, "USR ");
-		else
-			pt_dump_cont_printf(m, dmsg, "    ");
-		if (pr & _PAGE_RW)
-			pt_dump_cont_printf(m, dmsg, "RW ");
-		else
-			pt_dump_cont_printf(m, dmsg, "ro ");
-		if (pr & _PAGE_PWT)
-			pt_dump_cont_printf(m, dmsg, "PWT ");
-		else
-			pt_dump_cont_printf(m, dmsg, "    ");
-		if (pr & _PAGE_PCD)
-			pt_dump_cont_printf(m, dmsg, "PCD ");
-		else
-			pt_dump_cont_printf(m, dmsg, "    ");
+		pt_dump_cont(m, dmsg, "%-4s", pr & _PAGE_USER ? "USR" : "");
+		pt_dump_cont(m, dmsg, "%-3s", pr & _PAGE_RW ? "RW" : "ro");
+		pt_dump_cont(m, dmsg, "%-4s", pr & _PAGE_PWT ? "PWT" : "");
+		pt_dump_cont(m, dmsg, "%-4s", pr & _PAGE_PCD ? "PCD" : "");
 
 		/* Bit 9 has a different meaning on level 3 vs 4 */
-		if (level <= 3) {
-			if (pr & _PAGE_PSE)
-				pt_dump_cont_printf(m, dmsg, "PSE ");
-			else
-				pt_dump_cont_printf(m, dmsg, "    ");
-		} else {
-			if (pr & _PAGE_PAT)
-				pt_dump_cont_printf(m, dmsg, "pat ");
-			else
-				pt_dump_cont_printf(m, dmsg, "    ");
-		}
-		if (pr & _PAGE_GLOBAL)
-			pt_dump_cont_printf(m, dmsg, "GLB ");
+		if (level <= 3)
+			pt_dump_cont(m, dmsg, "%-4s", pr & _PAGE_PSE ? "PSE" : "");
 		else
-			pt_dump_cont_printf(m, dmsg, "    ");
-		if (pr & _PAGE_NX)
-			pt_dump_cont_printf(m, dmsg, "NX ");
-		else
-			pt_dump_cont_printf(m, dmsg, "x  ");
+			pt_dump_cont(m, dmsg, "%-4s", pr & _PAGE_PAT ? "pat" : "");
+
+		pt_dump_cont(m, dmsg, "%-4s", pr & _PAGE_GLOBAL ? "GLB" : "");
+		pt_dump_cont(m, dmsg, "%-3s", pr & _PAGE_NX ? "NX" : "x");
 	}
-	pt_dump_cont_printf(m, dmsg, "%s\n", level_name[level]);
+	pt_dump_cont(m, dmsg, "%s\n", level_name[level]);
 }
 
 /*
@@ -209,8 +185,8 @@  static void note_page(struct seq_file *m, struct pg_state *st,
 		st->level = level;
 		st->marker = address_markers;
 		st->lines = 0;
-		pt_dump_seq_printf(m, st->to_dmesg, "---[ %s ]---\n",
-				   st->marker->name);
+		pt_dump_print(m, st->to_dmesg, "---[ %s ]---\n",
+			      st->marker->name);
 	} else if (prot != cur || level != st->level ||
 		   st->current_address >= st->marker[1].start_address) {
 		const char *unit = units;
@@ -222,18 +198,16 @@  static void note_page(struct seq_file *m, struct pg_state *st,
 		 */
 		if (!st->marker->max_lines ||
 		    st->lines < st->marker->max_lines) {
-			pt_dump_seq_printf(m, st->to_dmesg,
-					   "0x%0*lx-0x%0*lx   ",
-					   width, st->start_address,
-					   width, st->current_address);
+			pt_dump_print(m, st->to_dmesg, "0x%0*lx-0x%0*lx   ",
+				      width, st->start_address,
+				      width, st->current_address);
 
 			delta = st->current_address - st->start_address;
 			while (!(delta & 1023) && unit[1]) {
 				delta >>= 10;
 				unit++;
 			}
-			pt_dump_cont_printf(m, st->to_dmesg, "%9lu%c ",
-					    delta, *unit);
+			pt_dump_cont(m, st->to_dmesg, "%9lu%c ", delta, *unit);
 			printk_prot(m, st->current_prot, st->level,
 				    st->to_dmesg);
 		}
@@ -249,15 +223,14 @@  static void note_page(struct seq_file *m, struct pg_state *st,
 			    st->lines > st->marker->max_lines) {
 				unsigned long nskip =
 					st->lines - st->marker->max_lines;
-				pt_dump_seq_printf(m, st->to_dmesg,
-						   "... %lu entr%s skipped ... \n",
-						   nskip,
-						   nskip == 1 ? "y" : "ies");
+				pt_dump_print(m, st->to_dmesg,
+					      "... %lu entr%s skipped ...\n",
+					      nskip, nskip == 1 ? "y" : "ies");
 			}
 			st->marker++;
 			st->lines = 0;
-			pt_dump_seq_printf(m, st->to_dmesg, "---[ %s ]---\n",
-					   st->marker->name);
+			pt_dump_print(m, st->to_dmesg, "---[ %s ]---\n",
+				      st->marker->name);
 		}
 
 		st->start_address = st->current_address;