* [PATCH 0/4] Replace lseek..write/read to pwrite/pread
@ 2014-04-26 4:07 Wang Nan
2014-04-26 4:07 ` [PATCH 1/4] makedumpfile: redefine numerical limitaction macros Wang Nan
` (4 more replies)
0 siblings, 5 replies; 13+ messages in thread
From: Wang Nan @ 2014-04-26 4:07 UTC (permalink / raw)
To: kumagai-atsushi; +Cc: Wang Nan, Liu Hua, Petr Tesarik, kexec, Geng Hui
In original code there are many operations read from /write to specific
positions of a file. This series of patches replace such patterns to
pread/pwrite calls, reduces more than 100 lines of code.
Wang Nan (4):
makedumpfile: redefine numerical limitaction macros.
makedumpfile: cleanup non-standard ULONGLONG_MAX macros
makedumpfile: add -D_GNU_SOURCE to CFLAGS
makedumpfile: use pread/pwrite to eliminate lseek
Makefile | 4 +-
common.h | 19 ++++--
makedumpfile.c | 198 ++++++++++++---------------------------------------------
3 files changed, 58 insertions(+), 163 deletions(-)
Signed-off-by: Wang Nan <wangnan0@huawei.com>
Cc: Atsushi Kumagai <kumagai-atsushi@mxc.nes.nec.co.jp>
Cc: Petr Tesarik <ptesarik@suse.cz>
Cc: kexec@lists.infradead.org
Cc: Geng Hui <hui.geng@huawei.com>
Cc: Liu Hua <sdu.liu@huawei.com>
--
1.8.4
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH 1/4] makedumpfile: redefine numerical limitaction macros. 2014-04-26 4:07 [PATCH 0/4] Replace lseek..write/read to pwrite/pread Wang Nan @ 2014-04-26 4:07 ` Wang Nan 2014-04-28 14:23 ` Petr Tesarik 2014-04-26 4:07 ` [PATCH 2/4] makedumpfile: cleanup non-standard ULONGLONG_MAX macros Wang Nan ` (3 subsequent siblings) 4 siblings, 1 reply; 13+ messages in thread From: Wang Nan @ 2014-04-26 4:07 UTC (permalink / raw) To: kumagai-atsushi Cc: Wang Nan, Liu Hua, kexec, Wang Nan, Petr Tesarik, Geng Hui From: Wang Nan <pi3orama@gmail.com> According to C standard, numerical limitations macros such as ULONG_MAX should be defined in <limits.h>, and must be defined as "constant expressions suitable for use in #if preprocessing directives." (see "Numerical limits" section in the standard). Original definition in common.h breaks this rule: #define LONG_MAX ((long)(~0UL>>1)) which causes macros like following failure: #if LONG_MAX == 2147483647 # define LONG_BIT 32 #else # define LONG_BIT 64 #endif Unfortunately, the above code piece is taken from real glibc header (/usr/include/bits/xopen_lim.h), which is happen to be included by <limits.h> if _GNU_SOURCE is defined. This patch include <limits.h> in common.h to use C standard numerical macros. For system without such macros defined by C, this patch also defines L(L)ONG_MAX in a standard compatible way. By checking wich gcc -dM -E - <<<'' we know that __LONG_MAX__ and __LLONG_MAX__ macros should be defined by gcc by default. Definition of ULONG_MAX and ULLONG_MAX are taken from gcc standard include file (include-fixed/limits.h). In addition, macro ULONGLONG_MAX is nonstandard, the standard way for defining max ulonglong is ULLONG_MAX. Signed-off-by: Wang Nan <wangnan0@huawei.com> Cc: Atsushi Kumagai <kumagai-atsushi@mxc.nes.nec.co.jp> Cc: Petr Tesarik <ptesarik@suse.cz> Cc: kexec@lists.infradead.org Cc: Geng Hui <hui.geng@huawei.com> Cc: Liu Hua <sdu.liu@huawei.com> --- common.h | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/common.h b/common.h index 6ad3ca7..124f107 100644 --- a/common.h +++ b/common.h @@ -16,17 +16,29 @@ #ifndef _COMMON_H #define _COMMON_H +#include <limits.h> + #define TRUE (1) #define FALSE (0) #define ERROR (-1) #ifndef LONG_MAX -#define LONG_MAX ((long)(~0UL>>1)) +# warning LONG_MAX should have been defined in <limits.h> +# define LONG_MAX __LONG_MAX__ #endif #ifndef ULONG_MAX -#define ULONG_MAX (~0UL) +# warning ULONG_MAX should have been defined in <limits.h> +# define ULONG_MAX (LONG_MAX * 2UL + 1UL) +#endif +#ifndef LLONG_MAX +# warning LLONG_MAX should have been defined in <limits.h> +# define LLONG_MAX __LONG_LONG_MAX__ +#endif +#ifndef ULLONG_MAX +# warning ULLONG_MAX should have been defined in <limits.h> +# define ULLONG_MAX (LLONG_MAX * 2ULL + 1ULL) #endif -#define ULONGLONG_MAX (~0ULL) +#define ULONGLONG_MAX ULLONG_MAX #define MAX(a,b) ((a) > (b) ? (a) : (b)) #define MIN(a,b) ((a) < (b) ? (a) : (b)) -- 1.8.4 _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/4] makedumpfile: redefine numerical limitaction macros. 2014-04-26 4:07 ` [PATCH 1/4] makedumpfile: redefine numerical limitaction macros Wang Nan @ 2014-04-28 14:23 ` Petr Tesarik 2014-04-28 22:21 ` Wang Nan 0 siblings, 1 reply; 13+ messages in thread From: Petr Tesarik @ 2014-04-28 14:23 UTC (permalink / raw) To: Wang Nan; +Cc: kexec, Wang Nan, kumagai-atsushi, Liu Hua, Geng Hui On Sat, 26 Apr 2014 12:07:06 +0800 Wang Nan <wangnan0@huawei.com> wrote: > From: Wang Nan <pi3orama@gmail.com> > > According to C standard, numerical limitations macros such as ULONG_MAX > should be defined in <limits.h>, and must be defined as "constant > expressions suitable for use in #if preprocessing directives." (see > "Numerical limits" section in the standard). > > Original definition in common.h breaks this rule: > > #define LONG_MAX ((long)(~0UL>>1)) > > which causes macros like following failure: > > #if LONG_MAX == 2147483647 > # define LONG_BIT 32 > #else > # define LONG_BIT 64 > #endif > > Unfortunately, the above code piece is taken from real glibc header > (/usr/include/bits/xopen_lim.h), which is happen to be included by > <limits.h> if _GNU_SOURCE is defined. > > This patch include <limits.h> in common.h to use C standard numerical > macros. For system without such macros defined by C, this patch also > defines L(L)ONG_MAX in a standard compatible way. By checking wich > > gcc -dM -E - <<<'' > > we know that __LONG_MAX__ and __LLONG_MAX__ macros should be defined by > gcc by default. Definition of ULONG_MAX and ULLONG_MAX are taken from > gcc standard include file (include-fixed/limits.h). > > In addition, macro ULONGLONG_MAX is nonstandard, the standard way for > defining max ulonglong is ULLONG_MAX. > > Signed-off-by: Wang Nan <wangnan0@huawei.com> > Cc: Atsushi Kumagai <kumagai-atsushi@mxc.nes.nec.co.jp> > Cc: Petr Tesarik <ptesarik@suse.cz> > Cc: kexec@lists.infradead.org > Cc: Geng Hui <hui.geng@huawei.com> > Cc: Liu Hua <sdu.liu@huawei.com> > > --- > common.h | 18 +++++++++++++++--- > 1 file changed, 15 insertions(+), 3 deletions(-) > > diff --git a/common.h b/common.h > index 6ad3ca7..124f107 100644 > --- a/common.h > +++ b/common.h > @@ -16,17 +16,29 @@ > #ifndef _COMMON_H > #define _COMMON_H > > +#include <limits.h> > + > #define TRUE (1) > #define FALSE (0) > #define ERROR (-1) > > #ifndef LONG_MAX > -#define LONG_MAX ((long)(~0UL>>1)) > +# warning LONG_MAX should have been defined in <limits.h> > +# define LONG_MAX __LONG_MAX__ > #endif > #ifndef ULONG_MAX > -#define ULONG_MAX (~0UL) > +# warning ULONG_MAX should have been defined in <limits.h> > +# define ULONG_MAX (LONG_MAX * 2UL + 1UL) > +#endif > +#ifndef LLONG_MAX > +# warning LLONG_MAX should have been defined in <limits.h> > +# define LLONG_MAX __LONG_LONG_MAX__ > +#endif > +#ifndef ULLONG_MAX > +# warning ULLONG_MAX should have been defined in <limits.h> > +# define ULLONG_MAX (LLONG_MAX * 2ULL + 1ULL) > #endif > -#define ULONGLONG_MAX (~0ULL) > +#define ULONGLONG_MAX ULLONG_MAX Hi Wang Nan, is this actually needed on some known platform? If not, then I'd rather remove all these #ifndef stanzas and rely on <limits.h>. I mean, if you can't rely on standard C constants, then why should be the gcc-specific pre-defined macros (__LONG_MAX__ et al.) available? It's probably better to put the burden on the person doing the port, because they should know what is appropriate for their compiler and/or libc. Just my opinion, Petr T _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/4] makedumpfile: redefine numerical limitaction macros. 2014-04-28 14:23 ` Petr Tesarik @ 2014-04-28 22:21 ` Wang Nan 0 siblings, 0 replies; 13+ messages in thread From: Wang Nan @ 2014-04-28 22:21 UTC (permalink / raw) To: Petr Tesarik; +Cc: kexec, Wang Nan, kumagai-atsushi, Liu Hua, Geng Hui On 2014/4/28 22:23, Petr Tesarik wrote: > On Sat, 26 Apr 2014 12:07:06 +0800 > Wang Nan <wangnan0@huawei.com> wrote: > >> From: Wang Nan <pi3orama@gmail.com> >> >> According to C standard, numerical limitations macros such as ULONG_MAX >> should be defined in <limits.h>, and must be defined as "constant >> expressions suitable for use in #if preprocessing directives." (see >> "Numerical limits" section in the standard). >> >> Original definition in common.h breaks this rule: >> >> #define LONG_MAX ((long)(~0UL>>1)) >> >> which causes macros like following failure: >> >> #if LONG_MAX == 2147483647 >> # define LONG_BIT 32 >> #else >> # define LONG_BIT 64 >> #endif >> >> Unfortunately, the above code piece is taken from real glibc header >> (/usr/include/bits/xopen_lim.h), which is happen to be included by >> <limits.h> if _GNU_SOURCE is defined. >> >> This patch include <limits.h> in common.h to use C standard numerical >> macros. For system without such macros defined by C, this patch also >> defines L(L)ONG_MAX in a standard compatible way. By checking wich >> >> gcc -dM -E - <<<'' >> >> we know that __LONG_MAX__ and __LLONG_MAX__ macros should be defined by >> gcc by default. Definition of ULONG_MAX and ULLONG_MAX are taken from >> gcc standard include file (include-fixed/limits.h). >> >> In addition, macro ULONGLONG_MAX is nonstandard, the standard way for >> defining max ulonglong is ULLONG_MAX. >> >> Signed-off-by: Wang Nan <wangnan0@huawei.com> >> Cc: Atsushi Kumagai <kumagai-atsushi@mxc.nes.nec.co.jp> >> Cc: Petr Tesarik <ptesarik@suse.cz> >> Cc: kexec@lists.infradead.org >> Cc: Geng Hui <hui.geng@huawei.com> >> Cc: Liu Hua <sdu.liu@huawei.com> >> >> --- >> common.h | 18 +++++++++++++++--- >> 1 file changed, 15 insertions(+), 3 deletions(-) >> >> diff --git a/common.h b/common.h >> index 6ad3ca7..124f107 100644 >> --- a/common.h >> +++ b/common.h >> @@ -16,17 +16,29 @@ >> #ifndef _COMMON_H >> #define _COMMON_H >> >> +#include <limits.h> >> + >> #define TRUE (1) >> #define FALSE (0) >> #define ERROR (-1) >> >> #ifndef LONG_MAX >> -#define LONG_MAX ((long)(~0UL>>1)) >> +# warning LONG_MAX should have been defined in <limits.h> >> +# define LONG_MAX __LONG_MAX__ >> #endif >> #ifndef ULONG_MAX >> -#define ULONG_MAX (~0UL) >> +# warning ULONG_MAX should have been defined in <limits.h> >> +# define ULONG_MAX (LONG_MAX * 2UL + 1UL) >> +#endif >> +#ifndef LLONG_MAX >> +# warning LLONG_MAX should have been defined in <limits.h> >> +# define LLONG_MAX __LONG_LONG_MAX__ >> +#endif >> +#ifndef ULLONG_MAX >> +# warning ULLONG_MAX should have been defined in <limits.h> >> +# define ULLONG_MAX (LLONG_MAX * 2ULL + 1ULL) >> #endif >> -#define ULONGLONG_MAX (~0ULL) >> +#define ULONGLONG_MAX ULLONG_MAX > > Hi Wang Nan, > > is this actually needed on some known platform? If not, then I'd rather > remove all these #ifndef stanzas and rely on <limits.h>. I mean, if you > can't rely on standard C constants, then why should be the gcc-specific > pre-defined macros (__LONG_MAX__ et al.) available? > These macros exist at the first version (at makedumpfile.h), an enforced by commit ab9c60bf (just because they conflict with limits.h ...). But I don't think there exists a real platform without <limits.h>. I agree with you that totally removing these macros should be better. > It's probably better to put the burden on the person doing the > port, because they should know what is appropriate for their compiler > and/or libc. > > Just my opinion, > Petr T > _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/4] makedumpfile: cleanup non-standard ULONGLONG_MAX macros 2014-04-26 4:07 [PATCH 0/4] Replace lseek..write/read to pwrite/pread Wang Nan 2014-04-26 4:07 ` [PATCH 1/4] makedumpfile: redefine numerical limitaction macros Wang Nan @ 2014-04-26 4:07 ` Wang Nan 2014-04-26 4:07 ` [PATCH 3/4] makedumpfile: add -D_GNU_SOURCE to CFLAGS Wang Nan ` (2 subsequent siblings) 4 siblings, 0 replies; 13+ messages in thread From: Wang Nan @ 2014-04-26 4:07 UTC (permalink / raw) To: kumagai-atsushi; +Cc: Wang Nan, Liu Hua, Petr Tesarik, kexec, Geng Hui Macro ULONGLONG_MAX is non-standard, the standard way to defining maximum limitation of unsigned long long is ULLONG_MAX, which is defined in <limits.h> by C standard. This patch replace all ULONGLONG_MAX to ULLONG_MAX. Signed-off-by: Wang Nan <wangnan0@huawei.com> Cc: Atsushi Kumagai <kumagai-atsushi@mxc.nes.nec.co.jp> Cc: Petr Tesarik <ptesarik@suse.cz> Cc: kexec@lists.infradead.org Cc: Geng Hui <hui.geng@huawei.com> Cc: Liu Hua <sdu.liu@huawei.com> --- common.h | 3 +-- makedumpfile.c | 10 +++++----- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/common.h b/common.h index 124f107..b86c57a 100644 --- a/common.h +++ b/common.h @@ -38,7 +38,6 @@ # warning ULLONG_MAX should have been defined in <limits.h> # define ULLONG_MAX (LLONG_MAX * 2ULL + 1ULL) #endif -#define ULONGLONG_MAX ULLONG_MAX #define MAX(a,b) ((a) > (b) ? (a) : (b)) #define MIN(a,b) ((a) < (b) ? (a) : (b)) @@ -52,7 +51,7 @@ */ #define NOT_MEMMAP_ADDR (0x0) #define NOT_KV_ADDR (0x0) -#define NOT_PADDR (ULONGLONG_MAX) +#define NOT_PADDR (ULLONG_MAX) #endif /* COMMON_H */ diff --git a/makedumpfile.c b/makedumpfile.c index ce4a866..33c378d 100644 --- a/makedumpfile.c +++ b/makedumpfile.c @@ -3797,7 +3797,7 @@ unsigned long long page_to_pfn(unsigned long page) { unsigned int num; - unsigned long long pfn = ULONGLONG_MAX; + unsigned long long pfn = ULLONG_MAX; unsigned long long index = 0; struct mem_map_data *mmd; @@ -3813,9 +3813,9 @@ page_to_pfn(unsigned long page) pfn = mmd->pfn_start + index; break; } - if (pfn == ULONGLONG_MAX) { + if (pfn == ULLONG_MAX) { ERRMSG("Can't convert the address of page descriptor (%lx) to pfn.\n", page); - return ULONGLONG_MAX; + return ULLONG_MAX; } return pfn; } @@ -3852,7 +3852,7 @@ reset_bitmap_of_free_pages(unsigned long node_zones, struct cycle *cycle) for (;curr != head;) { curr_page = curr - OFFSET(page.lru); start_pfn = page_to_pfn(curr_page); - if (start_pfn == ULONGLONG_MAX) + if (start_pfn == ULLONG_MAX) return FALSE; if (!readmem(VADDR, curr+OFFSET(list_head.prev), @@ -4678,7 +4678,7 @@ __exclude_unnecessary_pages(unsigned long mem_map, /* * Refresh the buffer of struct page, when changing mem_map. */ - pfn_read_start = ULONGLONG_MAX; + pfn_read_start = ULLONG_MAX; pfn_read_end = 0; for (pfn = pfn_start; pfn < pfn_end; pfn++, mem_map += SIZE(page)) { -- 1.8.4 _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 3/4] makedumpfile: add -D_GNU_SOURCE to CFLAGS 2014-04-26 4:07 [PATCH 0/4] Replace lseek..write/read to pwrite/pread Wang Nan 2014-04-26 4:07 ` [PATCH 1/4] makedumpfile: redefine numerical limitaction macros Wang Nan 2014-04-26 4:07 ` [PATCH 2/4] makedumpfile: cleanup non-standard ULONGLONG_MAX macros Wang Nan @ 2014-04-26 4:07 ` Wang Nan 2014-04-30 11:55 ` HATAYAMA Daisuke 2014-04-26 4:07 ` [PATCH 4/4] makedumpfile: use pread/pwrite to eliminate lseek Wang Nan 2014-04-30 11:41 ` [PATCH 0/4] Replace lseek..write/read to pwrite/pread HATAYAMA Daisuke 4 siblings, 1 reply; 13+ messages in thread From: Wang Nan @ 2014-04-26 4:07 UTC (permalink / raw) To: kumagai-atsushi; +Cc: Wang Nan, Liu Hua, Petr Tesarik, kexec, Geng Hui This patch is preparation for introduce pread/pwrite. Signed-off-by: Wang Nan <wangnan0@huawei.com> Cc: Atsushi Kumagai <kumagai-atsushi@mxc.nes.nec.co.jp> Cc: Petr Tesarik <ptesarik@suse.cz> Cc: kexec@lists.infradead.org Cc: Geng Hui <hui.geng@huawei.com> Cc: Liu Hua <sdu.liu@huawei.com> --- Makefile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index 68db947..1c65e61 100644 --- a/Makefile +++ b/Makefile @@ -9,11 +9,11 @@ CC = gcc endif CFLAGS = -g -O2 -Wall -D_FILE_OFFSET_BITS=64 \ - -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE \ + -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D_GNU_SOURCE \ -DVERSION='"$(VERSION)"' -DRELEASE_DATE='"$(DATE)"' \ -I/home/w00229757/makedumpfile/elfutils-root/include CFLAGS_ARCH = -g -O2 -Wall -D_FILE_OFFSET_BITS=64 \ - -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE + -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D_GNU_SOURCE # LDFLAGS = -L/usr/local/lib -I/usr/local/include LDFLAGS = -L/home/w00229757/makedumpfile/elfutils-root/lib -- 1.8.4 _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 3/4] makedumpfile: add -D_GNU_SOURCE to CFLAGS 2014-04-26 4:07 ` [PATCH 3/4] makedumpfile: add -D_GNU_SOURCE to CFLAGS Wang Nan @ 2014-04-30 11:55 ` HATAYAMA Daisuke 2014-05-04 1:28 ` Wang Nan 0 siblings, 1 reply; 13+ messages in thread From: HATAYAMA Daisuke @ 2014-04-30 11:55 UTC (permalink / raw) To: wangnan0; +Cc: ptesarik, kumagai-atsushi, kexec, sdu.liu, hui.geng From: Wang Nan <wangnan0@huawei.com> Subject: [PATCH 3/4] makedumpfile: add -D_GNU_SOURCE to CFLAGS Date: Sat, 26 Apr 2014 12:07:08 +0800 > This patch is preparation for introduce pread/pwrite. > Do you explain more about _GNU_SOURCE? Did you need to define this on your environment to build makedumpfile with pread/pwrite? I tried to build a very simple test program using pread like int main(void) { printf("%p\n", pread); } on RHEL5.4, RHEL6.5 and fc20, and all were done successfully without _GNU_SOURCE. They are all on x86_64. I checked man pread and man pwrite on each environments for _GNU_SOURCE but I didn't find it. What I found was _XOPEN_SOURCE description only. For example this is man pread on RHEL6.5. $ LANG=C man pread PREAD(2) Linux Programmer's Manual PREAD(2) NAME pread, pwrite - read from or write to a file descriptor at a given offset SYNOPSIS #define _XOPEN_SOURCE 500 #include <unistd.h> ssize_t pread(int fd, void *buf, size_t count, off_t offset); ssize_t pwrite(int fd, const void *buf, size_t count, off_t offset); Note that just as I said the above, building was successfully done on this environment. -- Thanks. HATAYAMA, Daisuke _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/4] makedumpfile: add -D_GNU_SOURCE to CFLAGS 2014-04-30 11:55 ` HATAYAMA Daisuke @ 2014-05-04 1:28 ` Wang Nan 0 siblings, 0 replies; 13+ messages in thread From: Wang Nan @ 2014-05-04 1:28 UTC (permalink / raw) To: HATAYAMA Daisuke; +Cc: ptesarik, kumagai-atsushi, kexec, sdu.liu, hui.geng On 2014/4/30 19:55, HATAYAMA Daisuke wrote: > From: Wang Nan <wangnan0@huawei.com> > Subject: [PATCH 3/4] makedumpfile: add -D_GNU_SOURCE to CFLAGS > Date: Sat, 26 Apr 2014 12:07:08 +0800 > >> This patch is preparation for introduce pread/pwrite. >> > > Do you explain more about _GNU_SOURCE? Did you need to define this on > your environment to build makedumpfile with pread/pwrite? > > I tried to build a very simple test program using pread like > > int main(void) > { > printf("%p\n", pread); > } > > on RHEL5.4, RHEL6.5 and fc20, and all were done successfully without > _GNU_SOURCE. They are all on x86_64. > > I checked man pread and man pwrite on each environments for > _GNU_SOURCE but I didn't find it. What I found was _XOPEN_SOURCE > description only. For example this is man pread on RHEL6.5. > > $ LANG=C man pread > PREAD(2) Linux Programmer's Manual PREAD(2) > > NAME > pread, pwrite - read from or write to a file descriptor at a given offset > > SYNOPSIS > #define _XOPEN_SOURCE 500 I define _GNU_SOURCE because it introduces _XOPEN_SOURCE. It also introduces others which I thought may help further improvements. Do you mean use only _XOPEN_SOURCE for pread/pwrite? > > #include <unistd.h> > > ssize_t pread(int fd, void *buf, size_t count, off_t offset); > > ssize_t pwrite(int fd, const void *buf, size_t count, off_t offset); > > Note that just as I said the above, building was successfully done on > this environment. > > -- > Thanks. > HATAYAMA, Daisuke > _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 4/4] makedumpfile: use pread/pwrite to eliminate lseek 2014-04-26 4:07 [PATCH 0/4] Replace lseek..write/read to pwrite/pread Wang Nan ` (2 preceding siblings ...) 2014-04-26 4:07 ` [PATCH 3/4] makedumpfile: add -D_GNU_SOURCE to CFLAGS Wang Nan @ 2014-04-26 4:07 ` Wang Nan 2014-04-30 11:41 ` [PATCH 0/4] Replace lseek..write/read to pwrite/pread HATAYAMA Daisuke 4 siblings, 0 replies; 13+ messages in thread From: Wang Nan @ 2014-04-26 4:07 UTC (permalink / raw) To: kumagai-atsushi; +Cc: Wang Nan, Liu Hua, Petr Tesarik, kexec, Geng Hui Original code use lseek..read/write pattern for read from/write to specific position of a file. This patch replaces them by pread/pwrite, reduces more than 100 LOC. Signed-off-by: Wang Nan <wangnan0@huawei.com> Cc: Atsushi Kumagai <kumagai-atsushi@mxc.nes.nec.co.jp> Cc: Petr Tesarik <ptesarik@suse.cz> Cc: kexec@lists.infradead.org Cc: Geng Hui <hui.geng@huawei.com> Cc: Liu Hua <sdu.liu@huawei.com> --- makedumpfile.c | 188 +++++++++++---------------------------------------------- 1 file changed, 36 insertions(+), 152 deletions(-) diff --git a/makedumpfile.c b/makedumpfile.c index 33c378d..934db88 100644 --- a/makedumpfile.c +++ b/makedumpfile.c @@ -25,6 +25,8 @@ #include <sys/time.h> #include <limits.h> +#include <unistd.h> + struct symbol_table symbol_table; struct size_table size_table; struct offset_table offset_table; @@ -259,16 +261,11 @@ read_page_desc(unsigned long long paddr, page_desc_t *pd) pfn = paddr_to_pfn(paddr); desc_pos = pfn_to_pos(pfn); offset += (off_t)desc_pos * sizeof(page_desc_t); - if (lseek(info->fd_memory, offset, SEEK_SET) < 0) { - ERRMSG("Can't seek %s. %s\n", - info->name_memory, strerror(errno)); - return FALSE; - } /* * Read page descriptor */ - if (read(info->fd_memory, pd, sizeof(*pd)) != sizeof(*pd)) { + if (pread(info->fd_memory, pd, sizeof(*pd), offset) != sizeof(*pd)) { ERRMSG("Can't read %s. %s\n", info->name_memory, strerror(errno)); return FALSE; @@ -374,8 +371,6 @@ next_region: static int read_from_vmcore(off_t offset, void *bufptr, unsigned long size) { - const off_t failed = (off_t)-1; - if (info->flag_usemmap == MMAP_ENABLE && page_is_fractional(offset) == FALSE) { if (!read_with_mmap(offset, bufptr, size)) { @@ -392,13 +387,7 @@ read_from_vmcore(off_t offset, void *bufptr, unsigned long size) read_from_vmcore(offset, bufptr, size); } } else { - if (lseek(info->fd_memory, offset, SEEK_SET) == failed) { - ERRMSG("Can't seek the dump memory(%s). (offset: %llx) %s\n", - info->name_memory, (unsigned long long)offset, strerror(errno)); - return FALSE; - } - - if (read(info->fd_memory, bufptr, size) != size) { + if (pread(info->fd_memory, bufptr, size, offset) != size) { ERRMSG("Can't read the dump memory(%s). %s\n", info->name_memory, strerror(errno)); return FALSE; @@ -520,18 +509,12 @@ readpage_kdump_compressed(unsigned long long paddr, void *bufptr) return FALSE; } - if (lseek(info->fd_memory, pd.offset, SEEK_SET) < 0) { - ERRMSG("Can't seek %s. %s\n", - info->name_memory, strerror(errno)); - return FALSE; - } - /* * Read page data */ rdbuf = pd.flags & (DUMP_DH_COMPRESSED_ZLIB | DUMP_DH_COMPRESSED_LZO | DUMP_DH_COMPRESSED_SNAPPY) ? buf : bufptr; - if (read(info->fd_memory, rdbuf, pd.size) != pd.size) { + if (pread(info->fd_memory, rdbuf, pd.size, pd.offset) != pd.size) { ERRMSG("Can't read %s. %s\n", info->name_memory, strerror(errno)); return FALSE; @@ -1555,7 +1538,6 @@ get_str_osrelease_from_vmlinux(void) struct utsname system_utsname; unsigned long long utsname; off_t offset; - const off_t failed = (off_t)-1; /* * Get the kernel version. @@ -1576,11 +1558,7 @@ get_str_osrelease_from_vmlinux(void) utsname); return FALSE; } - if (lseek(fd, offset, SEEK_SET) == failed) { - ERRMSG("Can't seek %s. %s\n", name, strerror(errno)); - return FALSE; - } - if (read(fd, &system_utsname, sizeof system_utsname) + if (pread(fd, &system_utsname, sizeof(system_utsname), offset) != sizeof system_utsname) { ERRMSG("Can't read %s. %s\n", name, strerror(errno)); return FALSE; @@ -2124,7 +2102,6 @@ copy_vmcoreinfo(off_t offset, unsigned long size) { int fd; char buf[VMCOREINFO_BYTES]; - const off_t failed = (off_t)-1; if (!offset || !size) return FALSE; @@ -2134,12 +2111,7 @@ copy_vmcoreinfo(off_t offset, unsigned long size) info->name_vmcoreinfo, strerror(errno)); return FALSE; } - if (lseek(info->fd_memory, offset, SEEK_SET) == failed) { - ERRMSG("Can't seek the dump memory(%s). %s\n", - info->name_memory, strerror(errno)); - return FALSE; - } - if (read(info->fd_memory, &buf, size) != size) { + if (pread(info->fd_memory, &buf, size, offset) != size) { ERRMSG("Can't read the dump memory(%s). %s\n", info->name_memory, strerror(errno)); return FALSE; @@ -3318,12 +3290,7 @@ set_bitmap(struct dump_bitmap *bitmap, unsigned long long pfn, new_offset = bitmap->offset + BUFSIZE_BITMAP * (pfn / PFN_BUFBITMAP); if (0 <= bitmap->no_block && old_offset != new_offset) { - if (lseek(bitmap->fd, old_offset, SEEK_SET) < 0 ) { - ERRMSG("Can't seek the bitmap(%s). %s\n", - bitmap->file_name, strerror(errno)); - return FALSE; - } - if (write(bitmap->fd, bitmap->buf, BUFSIZE_BITMAP) + if (pwrite(bitmap->fd, bitmap->buf, BUFSIZE_BITMAP, old_offset) != BUFSIZE_BITMAP) { ERRMSG("Can't write the bitmap(%s). %s\n", bitmap->file_name, strerror(errno)); @@ -3331,12 +3298,7 @@ set_bitmap(struct dump_bitmap *bitmap, unsigned long long pfn, } } if (old_offset != new_offset) { - if (lseek(bitmap->fd, new_offset, SEEK_SET) < 0 ) { - ERRMSG("Can't seek the bitmap(%s). %s\n", - bitmap->file_name, strerror(errno)); - return FALSE; - } - if (read(bitmap->fd, bitmap->buf, BUFSIZE_BITMAP) + if (pread(bitmap->fd, bitmap->buf, BUFSIZE_BITMAP, new_offset) != BUFSIZE_BITMAP) { ERRMSG("Can't read the bitmap(%s). %s\n", bitmap->file_name, strerror(errno)); @@ -3398,12 +3360,7 @@ sync_bitmap(struct dump_bitmap *bitmap) if (bitmap->no_block < 0) return TRUE; - if (lseek(bitmap->fd, offset, SEEK_SET) < 0 ) { - ERRMSG("Can't seek the bitmap(%s). %s\n", - bitmap->file_name, strerror(errno)); - return FALSE; - } - if (write(bitmap->fd, bitmap->buf, BUFSIZE_BITMAP) + if (pwrite(bitmap->fd, bitmap->buf, BUFSIZE_BITMAP, offset) != BUFSIZE_BITMAP) { ERRMSG("Can't write the bitmap(%s). %s\n", bitmap->file_name, strerror(errno)); @@ -3519,14 +3476,7 @@ is_in_segs(unsigned long long paddr) int read_cache(struct cache_data *cd) { - const off_t failed = (off_t)-1; - - if (lseek(cd->fd, cd->offset, SEEK_SET) == failed) { - ERRMSG("Can't seek the dump file(%s). %s\n", - cd->file_name, strerror(errno)); - return FALSE; - } - if (read(cd->fd, cd->buf, cd->cache_size) != cd->cache_size) { + if (pread(cd->fd, cd->buf, cd->cache_size, cd->offset) != cd->cache_size) { ERRMSG("Can't read the dump file(%s). %s\n", cd->file_name, strerror(errno)); return FALSE; @@ -4892,24 +4842,16 @@ copy_bitmap(void) offset = 0; while (offset < (info->len_bitmap / 2)) { - if (lseek(info->bitmap1->fd, info->bitmap1->offset + offset, - SEEK_SET) == failed) { - ERRMSG("Can't seek the bitmap(%s). %s\n", - info->name_bitmap, strerror(errno)); - return FALSE; - } - if (read(info->bitmap1->fd, buf, sizeof(buf)) != sizeof(buf)) { + if (pread(info->bitmap1->fd, buf, sizeof(buf), + info->bitmap1->offset + offset) + != sizeof(buf)) { ERRMSG("Can't read the dump memory(%s). %s\n", info->name_memory, strerror(errno)); return FALSE; } - if (lseek(info->bitmap2->fd, info->bitmap2->offset + offset, - SEEK_SET) == failed) { - ERRMSG("Can't seek the bitmap(%s). %s\n", - info->name_bitmap, strerror(errno)); - return FALSE; - } - if (write(info->bitmap2->fd, buf, sizeof(buf)) != sizeof(buf)) { + if (pwrite(info->bitmap2->fd, buf, sizeof(buf), + info->bitmap2->offset + offset) + != sizeof(buf)) { ERRMSG("Can't write the bitmap(%s). %s\n", info->name_bitmap, strerror(errno)); return FALSE; @@ -5456,12 +5398,8 @@ write_elf_header(struct cache_data *cd_header) strerror(errno)); goto out; } - if (lseek(info->fd_memory, offset_note_memory, SEEK_SET) == failed) { - ERRMSG("Can't seek the dump memory(%s). %s\n", - info->name_memory, strerror(errno)); - goto out; - } - if (read(info->fd_memory, buf, size_note) != size_note) { + if (pread(info->fd_memory, buf, size_note, offset_note_memory) + != size_note) { ERRMSG("Can't read the dump memory(%s). %s\n", info->name_memory, strerror(errno)); goto out; @@ -5570,12 +5508,8 @@ write_kdump_header(void) } if (!info->flag_sadump) { - if (lseek(info->fd_memory, offset_note, SEEK_SET) < 0) { - ERRMSG("Can't seek the dump memory(%s). %s\n", - info->name_memory, strerror(errno)); - goto out; - } - if (read(info->fd_memory, buf, size_note) != size_note) { + if (pread(info->fd_memory, buf, size_note, offset_note) + != size_note) { ERRMSG("Can't read the dump memory(%s). %s\n", info->name_memory, strerror(errno)); goto out; @@ -6586,12 +6520,7 @@ copy_eraseinfo(struct cache_data *cd_eraseinfo) strerror(errno)); return FALSE; } - if (lseek(info->fd_memory, offset, SEEK_SET) < 0) { - ERRMSG("Can't seek the dump memory(%s). %s\n", - info->name_memory, strerror(errno)); - goto out; - } - if (read(info->fd_memory, buf, size) != size) { + if (pread(info->fd_memory, buf, size, offset) != size) { ERRMSG("Can't read the dump memory(%s). %s\n", info->name_memory, strerror(errno)); goto out; @@ -7169,12 +7098,8 @@ init_xen_crash_info(void) return FALSE; } - if (lseek(info->fd_memory, offset_xen_crash_info, SEEK_SET) < 0) { - ERRMSG("Can't seek the dump memory(%s). %s\n", - info->name_memory, strerror(errno)); - return FALSE; - } - if (read(info->fd_memory, buf, size_xen_crash_info) + if (pread(info->fd_memory, buf, size_xen_crash_info, + offset_xen_crash_info) != size_xen_crash_info) { ERRMSG("Can't read the dump memory(%s). %s\n", info->name_memory, strerror(errno)); @@ -8155,12 +8080,7 @@ __read_disk_dump_header(struct disk_dump_header *dh, char *filename) filename, strerror(errno)); return FALSE; } - if (lseek(fd, 0x0, SEEK_SET) < 0) { - ERRMSG("Can't seek a file(%s). %s\n", - filename, strerror(errno)); - goto out; - } - if (read(fd, dh, sizeof(struct disk_dump_header)) + if (pread(fd, dh, sizeof(struct disk_dump_header), 0x0) != sizeof(struct disk_dump_header)) { ERRMSG("Can't read a file(%s). %s\n", filename, strerror(errno)); @@ -8204,12 +8124,7 @@ read_kdump_sub_header(struct kdump_sub_header *kh, char *filename) filename, strerror(errno)); return FALSE; } - if (lseek(fd, offset, SEEK_SET) < 0) { - ERRMSG("Can't seek a file(%s). %s\n", - filename, strerror(errno)); - goto out; - } - if (read(fd, kh, sizeof(struct kdump_sub_header)) + if (pread(fd, kh, sizeof(struct kdump_sub_header), offset) != sizeof(struct kdump_sub_header)) { ERRMSG("Can't read a file(%s). %s\n", filename, strerror(errno)); @@ -8373,19 +8288,11 @@ copy_same_data(int src_fd, int dst_fd, off_t offset, unsigned long size) ERRMSG("Can't allocate memory.\n"); return FALSE; } - if (lseek(src_fd, offset, SEEK_SET) < 0) { - ERRMSG("Can't seek a source file. %s\n", strerror(errno)); - goto out; - } - if (read(src_fd, buf, size) != size) { + if (pread(src_fd, buf, size, offset) != size) { ERRMSG("Can't read a source file. %s\n", strerror(errno)); goto out; } - if (lseek(dst_fd, offset, SEEK_SET) < 0) { - ERRMSG("Can't seek a destination file. %s\n", strerror(errno)); - goto out; - } - if (write(dst_fd, buf, size) != size) { + if (pwrite(pdst_fd, buf, size, offset) != size) { ERRMSG("Can't write a destination file. %s\n", strerror(errno)); goto out; } @@ -8412,12 +8319,7 @@ reassemble_kdump_header(void) if (!read_disk_dump_header(&dh, SPLITTING_DUMPFILE(0))) return FALSE; - if (lseek(info->fd_dumpfile, 0x0, SEEK_SET) < 0) { - ERRMSG("Can't seek a file(%s). %s\n", - info->name_dumpfile, strerror(errno)); - return FALSE; - } - if (write(info->fd_dumpfile, &dh, sizeof(dh)) != sizeof(dh)) { + if (pwrite(info->fd_dumpfile, &dh, sizeof(dh), 0x0) != sizeof(dh)) { ERRMSG("Can't write a file(%s). %s\n", info->name_dumpfile, strerror(errno)); return FALSE; @@ -8435,12 +8337,8 @@ reassemble_kdump_header(void) kh.start_pfn_64 = 0; kh.end_pfn_64 = 0; - if (lseek(info->fd_dumpfile, info->page_size, SEEK_SET) < 0) { - ERRMSG("Can't seek a file(%s). %s\n", - info->name_dumpfile, strerror(errno)); - return FALSE; - } - if (write(info->fd_dumpfile, &kh, sizeof(kh)) != sizeof(kh)) { + if (pwrite(info->fd_dumpfile, &kh, sizeof(kh), info->page_size) + != sizeof(kh)) { ERRMSG("Can't write a file(%s). %s\n", info->name_dumpfile, strerror(errno)); return FALSE; @@ -8623,22 +8521,12 @@ reassemble_kdump_pages(void) print_progress(PROGRESS_COPY, num_dumped, num_dumpable); - if (lseek(fd, offset_ph_org, SEEK_SET) < 0) { - ERRMSG("Can't seek a file(%s). %s\n", - SPLITTING_DUMPFILE(i), strerror(errno)); - goto out; - } - if (read(fd, &pd, sizeof(pd)) != sizeof(pd)) { + if (pread(fd, &pd, sizeof(pd)) != sizeof(pd), offset_ph_org) { ERRMSG("Can't read a file(%s). %s\n", SPLITTING_DUMPFILE(i), strerror(errno)); goto out; } - if (lseek(fd, pd.offset, SEEK_SET) < 0) { - ERRMSG("Can't seek a file(%s). %s\n", - SPLITTING_DUMPFILE(i), strerror(errno)); - goto out; - } - if (read(fd, data, pd.size) != pd.size) { + if (pread(fd, data, pd.size, pd.offset) != pd.size) { ERRMSG("Can't read a file(%s). %s\n", SPLITTING_DUMPFILE(i), strerror(errno)); goto out; @@ -8692,13 +8580,9 @@ reassemble_kdump_pages(void) SPLITTING_DUMPFILE(i), strerror(errno)); goto out; } - if (lseek(fd, SPLITTING_OFFSET_EI(i), SEEK_SET) < 0) { - ERRMSG("Can't seek a file(%s). %s\n", - SPLITTING_DUMPFILE(i), strerror(errno)); - goto out; - } - if (read(fd, data, SPLITTING_SIZE_EI(i)) != - SPLITTING_SIZE_EI(i)) { + if (pread(fd, data, SPLITTING_SIZE_EI(i), + SPLITTING_OFFSET_EI(i)) + != SPLITTING_SIZE_EI(i)) { ERRMSG("Can't read a file(%s). %s\n", SPLITTING_DUMPFILE(i), strerror(errno)); goto out; -- 1.8.4 _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 0/4] Replace lseek..write/read to pwrite/pread 2014-04-26 4:07 [PATCH 0/4] Replace lseek..write/read to pwrite/pread Wang Nan ` (3 preceding siblings ...) 2014-04-26 4:07 ` [PATCH 4/4] makedumpfile: use pread/pwrite to eliminate lseek Wang Nan @ 2014-04-30 11:41 ` HATAYAMA Daisuke 2014-04-30 11:53 ` Petr Tesarik 4 siblings, 1 reply; 13+ messages in thread From: HATAYAMA Daisuke @ 2014-04-30 11:41 UTC (permalink / raw) To: wangnan0; +Cc: ptesarik, kumagai-atsushi, kexec, sdu.liu, hui.geng From: Wang Nan <wangnan0@huawei.com> Subject: [PATCH 0/4] Replace lseek..write/read to pwrite/pread Date: Sat, 26 Apr 2014 12:07:05 +0800 > In original code there are many operations read from /write to specific > positions of a file. This series of patches replace such patterns to > pread/pwrite calls, reduces more than 100 lines of code. > I'm now writing pthread support patch set and it will naturally include pread/pwrite like this patch set. It sounds to me that using pread/pwrite only to reduce lseek code is weak in motivation. Is there another visible merit? For example, any kind of performance improvement. I guess it's small even if exists compared to I/O. -- Thanks. HATAYAMA, Daisuke _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/4] Replace lseek..write/read to pwrite/pread 2014-04-30 11:41 ` [PATCH 0/4] Replace lseek..write/read to pwrite/pread HATAYAMA Daisuke @ 2014-04-30 11:53 ` Petr Tesarik 2014-04-30 12:19 ` HATAYAMA Daisuke 0 siblings, 1 reply; 13+ messages in thread From: Petr Tesarik @ 2014-04-30 11:53 UTC (permalink / raw) To: HATAYAMA Daisuke; +Cc: wangnan0, kumagai-atsushi, kexec, sdu.liu, hui.geng On Wed, 30 Apr 2014 20:41:38 +0900 (JST) HATAYAMA Daisuke <d.hatayama@jp.fujitsu.com> wrote: > From: Wang Nan <wangnan0@huawei.com> > Subject: [PATCH 0/4] Replace lseek..write/read to pwrite/pread > Date: Sat, 26 Apr 2014 12:07:05 +0800 > > > In original code there are many operations read from /write to specific > > positions of a file. This series of patches replace such patterns to > > pread/pwrite calls, reduces more than 100 lines of code. > > > > I'm now writing pthread support patch set and it will naturally > include pread/pwrite like this patch set. > > It sounds to me that using pread/pwrite only to reduce lseek code is > weak in motivation. Is there another visible merit? For example, any > kind of performance improvement. I guess it's small even if exists > compared to I/O. There is no user-visible benefit just from applying the patch, that's right. The main benefit is that these pread/pwrite operations are atomic and do not move the file offset, so all subprocesses (or threads) can share the same file descriptor. This allows to remove reopen_dump_memory(), for example. Anyway, is improving code readability really so weak argument? Petr T _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/4] Replace lseek..write/read to pwrite/pread 2014-04-30 11:53 ` Petr Tesarik @ 2014-04-30 12:19 ` HATAYAMA Daisuke 2014-04-30 13:21 ` Petr Tesarik 0 siblings, 1 reply; 13+ messages in thread From: HATAYAMA Daisuke @ 2014-04-30 12:19 UTC (permalink / raw) To: ptesarik; +Cc: wangnan0, kumagai-atsushi, kexec, sdu.liu, hui.geng From: Petr Tesarik <ptesarik@suse.cz> Subject: Re: [PATCH 0/4] Replace lseek..write/read to pwrite/pread Date: Wed, 30 Apr 2014 13:53:04 +0200 > On Wed, 30 Apr 2014 20:41:38 +0900 (JST) > HATAYAMA Daisuke <d.hatayama@jp.fujitsu.com> wrote: > >> From: Wang Nan <wangnan0@huawei.com> >> Subject: [PATCH 0/4] Replace lseek..write/read to pwrite/pread >> Date: Sat, 26 Apr 2014 12:07:05 +0800 >> >> > In original code there are many operations read from /write to specific >> > positions of a file. This series of patches replace such patterns to >> > pread/pwrite calls, reduces more than 100 lines of code. >> > >> >> I'm now writing pthread support patch set and it will naturally >> include pread/pwrite like this patch set. >> >> It sounds to me that using pread/pwrite only to reduce lseek code is >> weak in motivation. Is there another visible merit? For example, any >> kind of performance improvement. I guess it's small even if exists >> compared to I/O. > > There is no user-visible benefit just from applying the patch, that's > right. > > The main benefit is that these pread/pwrite operations are atomic and do > not move the file offset, so all subprocesses (or threads) can share > the same file descriptor. This allows to remove reopen_dump_memory(), > for example. > > Anyway, is improving code readability really so weak argument? > > Petr T In the current code, parallelism occurs only when --split option is specified. It seems to me that reopen_dump_memory() is enough. So, it appears to me that this patch set just changes lseek(); read(); or lseek(); write() lines to pread() or pwrite(), so I don't see so big difference... -- Thanks. HATAYAMA, Daisuke _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/4] Replace lseek..write/read to pwrite/pread 2014-04-30 12:19 ` HATAYAMA Daisuke @ 2014-04-30 13:21 ` Petr Tesarik 0 siblings, 0 replies; 13+ messages in thread From: Petr Tesarik @ 2014-04-30 13:21 UTC (permalink / raw) To: HATAYAMA Daisuke; +Cc: wangnan0, kumagai-atsushi, kexec, sdu.liu, hui.geng On Wed, 30 Apr 2014 21:19:55 +0900 (JST) HATAYAMA Daisuke <d.hatayama@jp.fujitsu.com> wrote: > From: Petr Tesarik <ptesarik@suse.cz> > Subject: Re: [PATCH 0/4] Replace lseek..write/read to pwrite/pread > Date: Wed, 30 Apr 2014 13:53:04 +0200 > > > On Wed, 30 Apr 2014 20:41:38 +0900 (JST) > > HATAYAMA Daisuke <d.hatayama@jp.fujitsu.com> wrote: > > > >> From: Wang Nan <wangnan0@huawei.com> > >> Subject: [PATCH 0/4] Replace lseek..write/read to pwrite/pread > >> Date: Sat, 26 Apr 2014 12:07:05 +0800 > >> > >> > In original code there are many operations read from /write to specific > >> > positions of a file. This series of patches replace such patterns to > >> > pread/pwrite calls, reduces more than 100 lines of code. > >> > > >> > >> I'm now writing pthread support patch set and it will naturally > >> include pread/pwrite like this patch set. > >> > >> It sounds to me that using pread/pwrite only to reduce lseek code is > >> weak in motivation. Is there another visible merit? For example, any > >> kind of performance improvement. I guess it's small even if exists > >> compared to I/O. > > > > There is no user-visible benefit just from applying the patch, that's > > right. > > > > The main benefit is that these pread/pwrite operations are atomic and do > > not move the file offset, so all subprocesses (or threads) can share > > the same file descriptor. This allows to remove reopen_dump_memory(), > > for example. > > > > Anyway, is improving code readability really so weak argument? > > > > Petr T > > In the current code, parallelism occurs only when --split option is > specified. It seems to me that reopen_dump_memory() is enough. So, it > appears to me that this patch set just changes lseek(); read(); or > lseek(); write() lines to pread() or pwrite(), so I don't see so big > difference... True. But as you can see in patch 4/4, it also allowed to reduce the number of error paths. But I'm confused. Since you say you plan to replace lseek+read/write with pread/pwrite as part of a pthread-enabling patch series anyway, what is your point if you argue against replacing it now? Just trying to understand... Petr T _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2014-05-04 1:29 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-04-26 4:07 [PATCH 0/4] Replace lseek..write/read to pwrite/pread Wang Nan 2014-04-26 4:07 ` [PATCH 1/4] makedumpfile: redefine numerical limitaction macros Wang Nan 2014-04-28 14:23 ` Petr Tesarik 2014-04-28 22:21 ` Wang Nan 2014-04-26 4:07 ` [PATCH 2/4] makedumpfile: cleanup non-standard ULONGLONG_MAX macros Wang Nan 2014-04-26 4:07 ` [PATCH 3/4] makedumpfile: add -D_GNU_SOURCE to CFLAGS Wang Nan 2014-04-30 11:55 ` HATAYAMA Daisuke 2014-05-04 1:28 ` Wang Nan 2014-04-26 4:07 ` [PATCH 4/4] makedumpfile: use pread/pwrite to eliminate lseek Wang Nan 2014-04-30 11:41 ` [PATCH 0/4] Replace lseek..write/read to pwrite/pread HATAYAMA Daisuke 2014-04-30 11:53 ` Petr Tesarik 2014-04-30 12:19 ` HATAYAMA Daisuke 2014-04-30 13:21 ` Petr Tesarik
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox