linux-kernel - Re: [PATCH v2 2/2] Enhance read_block of page

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20130114023338.GB18097@blaptop>
Date:	Mon, 14 Jan 2013 11:33:38 +0900
From:	Minchan Kim <minchan@...nel.org>
To:	Michal Nazarewicz <mina86@...a86.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org, Mel Gorman <mgorman@...e.de>,
	Andy Whitcroft <apw@...dowen.org>,
	Alexander Nyberg <alexn@....su.se>,
	Randy Dunlap <rdunlap@...radead.org>
Subject: Re: [PATCH v2 2/2] Enhance read_block of page_owner.c

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 “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

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@...e.de>
> > Cc: Andy Whitcroft <apw@...dowen.org>
> > Cc: Alexander Nyberg <alexn@....su.se>
> > Cc: Randy Dunlap <rdunlap@...radead.org>
> > Signed-off-by: Michal Nazarewicz <mina86@...a86.com>
> > Signed-off-by: Minchan Kim <minchan@...nel.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@...gle.com>--------------ooO--(_)--Ooo--





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