[PATCH v2 1/2] Fix wrong EOF compare

[PATCH v2 1/2] Fix wrong EOF compare

[Minchan Kim]

The C standards allows the character type char to be singed or unsinged,
depending on the platform and compiler. Most of systems uses signed char,
but those based on PowerPC and ARM processors typically use unsigned char.
This can lead to unexpected results when the variable is used to compare
with EOF(-1). It happens my ARM system and this patch fixes it.

Cc: Mel Gorman <mgorman@suse.de>
Cc: Andy Whitcroft <apw@shadowen.org>
Cc: Alexander Nyberg <alexn@dsv.su.se>
Cc: Michal Nazarewicz <mina86@mina86.com>
Cc: Randy Dunlap <rdunlap@infradead.org>
Signed-off-by: Minchan Kim <minchan@kernel.org>
---
 Documentation/page_owner.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/Documentation/page_owner.c b/Documentation/page_owner.c
index f0156e1..43dde96 100644
--- a/Documentation/page_owner.c
+++ b/Documentation/page_owner.c
@@ -32,12 +32,13 @@ int read_block(char *buf, FILE *fin)
 {
 	int ret = 0;
 	int hit = 0;
+	int val;
 	char *curr = buf;
 
 	for (;;) {
-		*curr = getc(fin);
-		if (*curr == EOF) return -1;
-
+		val = getc(fin);
+		if (val == EOF) return -1;
+		*curr = val;
 		ret++;
 		if (*curr == '\n' && hit == 1)
 			return ret - 1;
-- 
1.7.9.5

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH v2 2/2] Enhance read_block of page_owner.c

[Minchan Kim]

The read_block reads char one by one until meeting two newline.
It's not good for the performance and current code isn't good shape
for readability.

This patch enhances speed and clean up.

Cc: Mel Gorman <mgorman@suse.de>
Cc: Andy Whitcroft <apw@shadowen.org>
Cc: Alexander Nyberg <alexn@dsv.su.se>
Cc: Randy Dunlap <rdunlap@infradead.org>
Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
Signed-off-by: Minchan Kim <minchan@kernel.org>
---
 Documentation/page_owner.c |   34 +++++++++++++---------------------
 1 file changed, 13 insertions(+), 21 deletions(-)

diff --git a/Documentation/page_owner.c b/Documentation/page_owner.c
index 43dde96..96bf481 100644
--- a/Documentation/page_owner.c
+++ b/Documentation/page_owner.c
@@ -28,26 +28,17 @@ static int max_size;
 
 struct block_list *block_head;
 
-int read_block(char *buf, FILE *fin)
+int read_block(char *buf, int buf_size, FILE *fin)
 {
-	int ret = 0;
-	int hit = 0;
-	int val;
-	char *curr = buf;
-
-	for (;;) {
-		val = getc(fin);
-		if (val == EOF) return -1;
-		*curr = val;
-		ret++;
-		if (*curr == '\n' && hit == 1)
-			return ret - 1;
-		else if (*curr == '\n')
-			hit = 1;
-		else
-			hit = 0;
-		curr++;
+	char *curr = buf, *const buf_end = buf + buf_size;
+
+	while (buf_end - curr > 1 && fgets(curr, buf_end - curr, fin)) {
+		if (*curr == '\n') /* empty line */
+			return curr - buf;
+		curr += strlen(curr);
 	}
+
+	return -1; /* EOF or no space left in buf. */
 }
 
 static int compare_txt(struct block_list *l1, struct block_list *l2)
@@ -84,10 +75,12 @@ static void add_list(char *buf, int len)
 	}
 }
 
+#define BUF_SIZE	1024
+
 int main(int argc, char **argv)
 {
 	FILE *fin, *fout;
-	char buf[1024];
+	char buf[BUF_SIZE];
 	int ret, i, count;
 	struct block_list *list2;
 	struct stat st;
@@ -106,11 +99,10 @@ int main(int argc, char **argv)
 	list = malloc(max_size * sizeof(*list));
 
 	for(;;) {
-		ret = read_block(buf, fin);
+		ret = read_block(buf, BUF_SIZE, fin);
 		if (ret < 0)
 			break;
 
-		buf[ret] = '\0';
 		add_list(buf, ret);
 	}
 
-- 
1.7.9.5

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2 2/2] Enhance read_block of page_owner.c

[Michal Nazarewicz]

[-- Attachment #1: Type: text/plain, Size: 3413 bytes --]

It occurred to me -- and I know it will sound like a heresy -- that
maybe providing an overly long example in C is not the best option here.
Why not page_owner.py with the following content instead (not tested):


#!/usr/bin/python
import collections
import sys

counts = collections.defaultdict(int)

txt = ''
for line in sys.stdin:
    if line == '\n':
        counts[txt] += 1
        txt = ''
    else:
        txt += line
counts[txt] += 1

for txt, num in sorted(counts.items(), txt=lambda x: x[1]):
    if len(txt) > 1:
        print '%d times:\n%s' % num, txt


And it's so “long” only because I chose not to read the whole file at
once as in:

    
counts = collections.defaultdict(int)
for txt in sys.stdin.read().split('\n\n'):
    counts[txt] += 1


On Fri, Jan 11 2013, Minchan Kim wrote:
> The read_block reads char one by one until meeting two newline.
> It's not good for the performance and current code isn't good shape
> for readability.
>
> This patch enhances speed and clean up.
>
> Cc: Mel Gorman <mgorman@suse.de>
> Cc: Andy Whitcroft <apw@shadowen.org>
> Cc: Alexander Nyberg <alexn@dsv.su.se>
> Cc: Randy Dunlap <rdunlap@infradead.org>
> Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
> Signed-off-by: Minchan Kim <minchan@kernel.org>
> ---
>  Documentation/page_owner.c |   34 +++++++++++++---------------------
>  1 file changed, 13 insertions(+), 21 deletions(-)
>
> diff --git a/Documentation/page_owner.c b/Documentation/page_owner.c
> index 43dde96..96bf481 100644
> --- a/Documentation/page_owner.c
> +++ b/Documentation/page_owner.c
> @@ -28,26 +28,17 @@ static int max_size;
>  
>  struct block_list *block_head;
>  
> -int read_block(char *buf, FILE *fin)
> +int read_block(char *buf, int buf_size, FILE *fin)
>  {
> -	int ret = 0;
> -	int hit = 0;
> -	int val;
> -	char *curr = buf;
> -
> -	for (;;) {
> -		val = getc(fin);
> -		if (val == EOF) return -1;
> -		*curr = val;
> -		ret++;
> -		if (*curr == '\n' && hit == 1)
> -			return ret - 1;
> -		else if (*curr == '\n')
> -			hit = 1;
> -		else
> -			hit = 0;
> -		curr++;
> +	char *curr = buf, *const buf_end = buf + buf_size;
> +
> +	while (buf_end - curr > 1 && fgets(curr, buf_end - curr, fin)) {
> +		if (*curr == '\n') /* empty line */
> +			return curr - buf;
> +		curr += strlen(curr);
>  	}
> +
> +	return -1; /* EOF or no space left in buf. */
>  }
>  
>  static int compare_txt(struct block_list *l1, struct block_list *l2)
> @@ -84,10 +75,12 @@ static void add_list(char *buf, int len)
>  	}
>  }
>  
> +#define BUF_SIZE	1024
> +
>  int main(int argc, char **argv)
>  {
>  	FILE *fin, *fout;
> -	char buf[1024];
> +	char buf[BUF_SIZE];
>  	int ret, i, count;
>  	struct block_list *list2;
>  	struct stat st;
> @@ -106,11 +99,10 @@ int main(int argc, char **argv)
>  	list = malloc(max_size * sizeof(*list));
>  
>  	for(;;) {
> -		ret = read_block(buf, fin);
> +		ret = read_block(buf, BUF_SIZE, fin);
>  		if (ret < 0)
>  			break;
>  
> -		buf[ret] = '\0';
>  		add_list(buf, ret);
>  	}
>  
> -- 
> 1.7.9.5
>

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +----<email/xmpp: mpn@google.com>--------------ooO--(_)--Ooo--

[-- Attachment #2.1: Type: text/plain, Size: 0 bytes --]



[-- Attachment #2.2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH v2 2/2] Enhance read_block of page_owner.c

[Minchan Kim]

On Fri, Jan 11, 2013 at 05:01:29PM +0100, Michal Nazarewicz wrote:
> It occurred to me -- and I know it will sound like a heresy -- that
> maybe providing an overly long example in C is not the best option here.
> Why not page_owner.py with the following content instead (not tested):
> 
> 
> #!/usr/bin/python
> import collections
> import sys
> 
> counts = collections.defaultdict(int)
> 
> txt = ''
> for line in sys.stdin:
>     if line == '\n':
>         counts[txt] += 1
>         txt = ''
>     else:
>         txt += line
> counts[txt] += 1
> 
> for txt, num in sorted(counts.items(), txt=lambda x: x[1]):
>     if len(txt) > 1:
>         print '%d times:\n%s' % num, txt
> 
> 
> And it's so a??longa?? only because I chose not to read the whole file at
> once as in:
> 
>     
> counts = collections.defaultdict(int)
> for txt in sys.stdin.read().split('\n\n'):
>     counts[txt] += 1

I'm not familar with Python but I can see the point of the program.
It's very short and good for maintainace but I have a concern about the size.
For working it in embedded side, we have to port python in that machine. :(
You might argue we can parse it on host after downloading from target machine.
But the problem is somecase we have no facility to download it from target
machine because only connection to outside is LCD.
In case of that, just small C program when we release product would be
good choice.

But I'm not strong aginst on your simple python program. If it is merged,
we will just continue to use C program instead of python's one.
If you have a strong opinion, send it to akpm as separate patch.

Thanks.

> 
> 
> On Fri, Jan 11 2013, Minchan Kim wrote:
> > The read_block reads char one by one until meeting two newline.
> > It's not good for the performance and current code isn't good shape
> > for readability.
> >
> > This patch enhances speed and clean up.
> >
> > Cc: Mel Gorman <mgorman@suse.de>
> > Cc: Andy Whitcroft <apw@shadowen.org>
> > Cc: Alexander Nyberg <alexn@dsv.su.se>
> > Cc: Randy Dunlap <rdunlap@infradead.org>
> > Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
> > Signed-off-by: Minchan Kim <minchan@kernel.org>
> > ---
> >  Documentation/page_owner.c |   34 +++++++++++++---------------------
> >  1 file changed, 13 insertions(+), 21 deletions(-)
> >
> > diff --git a/Documentation/page_owner.c b/Documentation/page_owner.c
> > index 43dde96..96bf481 100644
> > --- a/Documentation/page_owner.c
> > +++ b/Documentation/page_owner.c
> > @@ -28,26 +28,17 @@ static int max_size;
> >  
> >  struct block_list *block_head;
> >  
> > -int read_block(char *buf, FILE *fin)
> > +int read_block(char *buf, int buf_size, FILE *fin)
> >  {
> > -	int ret = 0;
> > -	int hit = 0;
> > -	int val;
> > -	char *curr = buf;
> > -
> > -	for (;;) {
> > -		val = getc(fin);
> > -		if (val == EOF) return -1;
> > -		*curr = val;
> > -		ret++;
> > -		if (*curr == '\n' && hit == 1)
> > -			return ret - 1;
> > -		else if (*curr == '\n')
> > -			hit = 1;
> > -		else
> > -			hit = 0;
> > -		curr++;
> > +	char *curr = buf, *const buf_end = buf + buf_size;
> > +
> > +	while (buf_end - curr > 1 && fgets(curr, buf_end - curr, fin)) {
> > +		if (*curr == '\n') /* empty line */
> > +			return curr - buf;
> > +		curr += strlen(curr);
> >  	}
> > +
> > +	return -1; /* EOF or no space left in buf. */
> >  }
> >  
> >  static int compare_txt(struct block_list *l1, struct block_list *l2)
> > @@ -84,10 +75,12 @@ static void add_list(char *buf, int len)
> >  	}
> >  }
> >  
> > +#define BUF_SIZE	1024
> > +
> >  int main(int argc, char **argv)
> >  {
> >  	FILE *fin, *fout;
> > -	char buf[1024];
> > +	char buf[BUF_SIZE];
> >  	int ret, i, count;
> >  	struct block_list *list2;
> >  	struct stat st;
> > @@ -106,11 +99,10 @@ int main(int argc, char **argv)
> >  	list = malloc(max_size * sizeof(*list));
> >  
> >  	for(;;) {
> > -		ret = read_block(buf, fin);
> > +		ret = read_block(buf, BUF_SIZE, fin);
> >  		if (ret < 0)
> >  			break;
> >  
> > -		buf[ret] = '\0';
> >  		add_list(buf, ret);
> >  	}
> >  
> > -- 
> > 1.7.9.5
> >
> 
> -- 
> Best regards,                                         _     _
> .o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
> ..o | Computer Science,  MichaA? a??mina86a?? Nazarewicz    (o o)
> ooo +----<email/xmpp: mpn@google.com>--------------ooO--(_)--Ooo--





-- 
Kind regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2 2/2] Enhance read_block of page_owner.c

[Michal Nazarewicz]

[-- Attachment #1: Type: text/plain, Size: 1510 bytes --]

On Mon, Jan 14 2013, Minchan Kim <minchan@kernel.org> wrote:
> I'm not familar with Python but I can see the point of the program.
> It's very short and good for maintainace but I have a concern about the size.
> For working it in embedded side, we have to port python in that
> machine. :(  [...]
> In case of that, just small C program when we release product would be
> good choice.

But is this program intended to be used as is? Or rather to serve as an
example?  If the former, than I think it should be in tools/ rather than
in Documentation/.  If the latter, than I think it does not really
matter whether it's C or some scripting language, since the purpose is
to show how /proc/page_owner can be used, and in fact showing the
general idea may be simpler with a shorter program which does not have
to deal with memory management.

And if Python is not your fancy, you can always use some shell: ;)

	awk -vRS= '{ gsub("\n", "\\n"); print $0 }' |sort |uniq -c

> But I'm not strong aginst on your simple python program. If it is merged,
> we will just continue to use C program instead of python's one.
> If you have a strong opinion, send it to akpm as separate patch.

Not at all.  I'm just throwing ideas.

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +----<email/xmpp: mpn@google.com>--------------ooO--(_)--Ooo--

[-- Attachment #2.1: Type: text/plain, Size: 0 bytes --]



[-- Attachment #2.2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH v2 1/2] Fix wrong EOF compare

[Michal Nazarewicz]

[-- Attachment #1: Type: text/plain, Size: 1677 bytes --]

On Fri, Jan 11 2013, Minchan Kim <minchan@kernel.org> wrote:
> The C standards allows the character type char to be singed or unsinged,
> depending on the platform and compiler. Most of systems uses signed char,
> but those based on PowerPC and ARM processors typically use unsigned char.
> This can lead to unexpected results when the variable is used to compare
> with EOF(-1). It happens my ARM system and this patch fixes it.
>
> Cc: Mel Gorman <mgorman@suse.de>
> Cc: Andy Whitcroft <apw@shadowen.org>
> Cc: Alexander Nyberg <alexn@dsv.su.se>
> Cc: Michal Nazarewicz <mina86@mina86.com>

Acked-by: Michal Nazarewicz <mina86@mina86.com>

> Cc: Randy Dunlap <rdunlap@infradead.org>
> Signed-off-by: Minchan Kim <minchan@kernel.org>
> ---
>  Documentation/page_owner.c |    7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/page_owner.c b/Documentation/page_owner.c
> index f0156e1..43dde96 100644
> --- a/Documentation/page_owner.c
> +++ b/Documentation/page_owner.c
> @@ -32,12 +32,13 @@ int read_block(char *buf, FILE *fin)
>  {
>  	int ret = 0;
>  	int hit = 0;
> +	int val;
>  	char *curr = buf;
>  
>  	for (;;) {
> -		*curr = getc(fin);
> -		if (*curr == EOF) return -1;
> -
> +		val = getc(fin);
> +		if (val == EOF) return -1;
> +		*curr = val;
>  		ret++;
>  		if (*curr == '\n' && hit == 1)
>  			return ret - 1;

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +----<email/xmpp: mpn@google.com>--------------ooO--(_)--Ooo--

[-- Attachment #2.1: Type: text/plain, Size: 0 bytes --]



[-- Attachment #2.2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH v2 1/2] Fix wrong EOF compare

[Rob Landley]

On 01/11/2013 08:21:55 AM, Michal Nazarewicz wrote:
> On Fri, Jan 11 2013, Minchan Kim <minchan@kernel.org> wrote:
> > The C standards allows the character type char to be singed or  
> unsinged,
> > depending on the platform and compiler. Most of systems uses signed  
> char,
> > but those based on PowerPC and ARM processors typically use  
> unsigned char.
> > This can lead to unexpected results when the variable is used to  
> compare
> > with EOF(-1). It happens my ARM system and this patch fixes it.
> >
> > Cc: Mel Gorman <mgorman@suse.de>
> > Cc: Andy Whitcroft <apw@shadowen.org>
> > Cc: Alexander Nyberg <alexn@dsv.su.se>
> > Cc: Michal Nazarewicz <mina86@mina86.com>
> 
> Acked-by: Michal Nazarewicz <mina86@mina86.com>
> 
> > Cc: Randy Dunlap <rdunlap@infradead.org>
> > Signed-off-by: Minchan Kim <minchan@kernel.org>
> > ---
> >  Documentation/page_owner.c |    7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)

My kernel tree doesn't have Documentation/page_owner.c, where do I find  
this file?

Rob
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2 1/2] Fix wrong EOF compare

[Randy Dunlap]

On 01/13/13 03:44, Rob Landley wrote:
> On 01/11/2013 08:21:55 AM, Michal Nazarewicz wrote:
>> On Fri, Jan 11 2013, Minchan Kim <minchan@kernel.org> wrote:
>> > The C standards allows the character type char to be singed or unsinged,
>> > depending on the platform and compiler. Most of systems uses signed char,
>> > but those based on PowerPC and ARM processors typically use unsigned char.
>> > This can lead to unexpected results when the variable is used to compare
>> > with EOF(-1). It happens my ARM system and this patch fixes it.
>> >
>> > Cc: Mel Gorman <mgorman@suse.de>
>> > Cc: Andy Whitcroft <apw@shadowen.org>
>> > Cc: Alexander Nyberg <alexn@dsv.su.se>
>> > Cc: Michal Nazarewicz <mina86@mina86.com>
>>
>> Acked-by: Michal Nazarewicz <mina86@mina86.com>
>>
>> > Cc: Randy Dunlap <rdunlap@infradead.org>
>> > Signed-off-by: Minchan Kim <minchan@kernel.org>
>> > ---
>> >  Documentation/page_owner.c |    7 ++++---
>> >  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> My kernel tree doesn't have Documentation/page_owner.c, where do I find this file?

It's in -mm (mmotm), so Andrew can/should merge this ...


-- 
~Randy

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2 1/2] Fix wrong EOF compare

[Rob Landley]

On 01/13/2013 12:15:41 PM, Randy Dunlap wrote:
> On 01/13/13 03:44, Rob Landley wrote:
> > On 01/11/2013 08:21:55 AM, Michal Nazarewicz wrote:
> >> On Fri, Jan 11 2013, Minchan Kim <minchan@kernel.org> wrote:
> >> > The C standards allows the character type char to be singed or  
> unsinged,
> >> > depending on the platform and compiler. Most of systems uses  
> signed char,
> >> > but those based on PowerPC and ARM processors typically use  
> unsigned char.
> >> > This can lead to unexpected results when the variable is used to  
> compare
> >> > with EOF(-1). It happens my ARM system and this patch fixes it.
> >> >
> >> > Cc: Mel Gorman <mgorman@suse.de>
> >> > Cc: Andy Whitcroft <apw@shadowen.org>
> >> > Cc: Alexander Nyberg <alexn@dsv.su.se>
> >> > Cc: Michal Nazarewicz <mina86@mina86.com>
> >>
> >> Acked-by: Michal Nazarewicz <mina86@mina86.com>
> >>
> >> > Cc: Randy Dunlap <rdunlap@infradead.org>
> >> > Signed-off-by: Minchan Kim <minchan@kernel.org>
> >> > ---
> >> >  Documentation/page_owner.c |    7 ++++---
> >> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > My kernel tree doesn't have Documentation/page_owner.c, where do I  
> find this file?
> 
> It's in -mm (mmotm), so Andrew can/should merge this ...

Actually, why is a .c source file at the top level of Documentation?

Example code is nice and all, but this name doesn't say "test" or  
"example" or anything like that, and isn't collated into a subdirectory  
with any kind of explanatory files like all the others are.

Rob
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>