Hi Kazu,
On Fri, Apr 21, 2023 at 10:41 AM HAGIO KAZUHITO(萩尾 一仁)
<k-hagio-ab@xxxxxxx> wrote:
>
> On 2023/04/21 10:57, Tao Liu wrote:
> > Hi Kazu,
> >
> > On Fri, Apr 21, 2023 at 9:48 AM HAGIO KAZUHITO(萩尾 一仁)
> > <k-hagio-ab@xxxxxxx> wrote:
> >>
> >> 2023/04/20 19:25, Tao Liu wrote:
> >>> Previously crash use lseek/read for kcore and vmcore reading, this involves 2
> >>> syscalls. And we can replace them with pread, only 1 syscall is needed for
> >>> kcore/vmcore reading, and we can have a better performance. Please note there
> >>> are plenty of places in crash using lseek/read, this patch doesn't modify all
> >>> of them, just the most commonly used kcore and diskdump vmcore reading.
> >>>
> >>> Signed-off-by: Tao Liu <ltao@xxxxxxxxxx>
> >>> ---
> >>>
> >>> v1 -> v2: add offset check before pread.
> >>
> >> Thanks for the v2, looks good to me.
> >> also confirmed that 'search abcd' for 64GB /proc/kcore became faster:
> >>
> >> real 0m56.733s real 0m47.446s
> >> user 0m28.187s --> user 0m26.663s
> >> sys 0m28.592s sys 0m20.834s
> >>
> >> Acked-by: Kazuhito Hagio <k-hagio-ab@xxxxxxx>
> >>
> > Thanks for the test data, and the previous suggestions which make the
> > patch better!
>
> Good to help :)
>
> Just an idea on another optimization,
>
> SEARCHMASK(si->s_parms.s_ulong.value[si->val])
>
> in search_ulong() might be pre-processed somewhere.
> For loops, reducing machine instructions is often effective.
>
Do you mean change the code like the following in search_ulong()?
int value = SEARCHMASK(si->s_parms.s_ulong.value[si->val]);
for (i = 0; i < longcnt; i++, bufptr++, addr += sizeof(long)) {
for (si->val = 0; si->val < si->vcnt; si->val++) {
if (SEARCHMASK(*bufptr) == value)) {
....
}
}
}
In this way, we don't need to recalculate the value every time in the loop.
Thanks,
Tao Liu
> Thanks,
> Kazu
>
> >
> > Thanks,
> > Tao Liu
> >
> >> Thanks,
> >> Kazu
> >>
> >>>
> >>> ---
> >>> diskdump.c | 31 +++++++++++++++++++++++++------
> >>> netdump.c | 19 ++++++++++++++-----
> >>> 2 files changed, 39 insertions(+), 11 deletions(-)
> >>>
> >>> diff --git a/diskdump.c b/diskdump.c
> >>> index cf5f5d9..6ecd08a 100644
> >>> --- a/diskdump.c
> >>> +++ b/diskdump.c
> >>> @@ -515,16 +515,24 @@ arm_kdump_header_adjust(int header_version)
> >>> static int
> >>> read_pd(int fd, off_t offset, page_desc_t *pd)
> >>> {
> >>> - const off_t failed = (off_t)-1;
> >>> + int ret;
> >>>
> >>> if (FLAT_FORMAT()) {
> >>> if (!read_flattened_format(fd, offset, pd, sizeof(*pd)))
> >>> return READ_ERROR;
> >>> } else {
> >>> - if (lseek(fd, offset, SEEK_SET) == failed)
> >>> + if (offset < 0) {
> >>> + if (CRASHDEBUG(8)) {
> >>> + fprintf(fp, "read_pd: invalid offset: %lx\n", offset);
> >>> + }
> >>> return SEEK_ERROR;
> >>> - if (read(fd, pd, sizeof(*pd)) != sizeof(*pd))
> >>> + }
> >>> + if ((ret = pread(fd, pd, sizeof(*pd), offset)) != sizeof(*pd)) {
> >>> + if (ret == -1 && CRASHDEBUG(8)) {
> >>> + fprintf(fp, "read_pd: pread error: %s\n", strerror(errno));
> >>> + }
> >>> return READ_ERROR;
> >>> + }
> >>> }
> >>>
> >>> return 0;
> >>> @@ -1125,7 +1133,6 @@ cache_page(physaddr_t paddr)
> >>> off_t seek_offset;
> >>> page_desc_t pd;
> >>> const int block_size = dd->block_size;
> >>> - const off_t failed = (off_t)-1;
> >>> ulong retlen;
> >>> #ifdef ZSTD
> >>> static ZSTD_DCtx *dctx = NULL;
> >>> @@ -1190,10 +1197,22 @@ cache_page(physaddr_t paddr)
> >>> return PAGE_INCOMPLETE;
> >>> }
> >>> } else {
> >>> - if (lseek(dd->dfd, pd.offset, SEEK_SET) == failed)
> >>> + if (pd.offset < 0) {
> >>> + if (CRASHDEBUG(8)) {
> >>> + fprintf(fp,
> >>> + "read_diskdump/cache_page: "
> >>> + "invalid offset: %lx\n", pd.offset);
> >>> + }
> >>> return SEEK_ERROR;
> >>> - if (read(dd->dfd, dd->compressed_page, pd.size) != pd.size)
> >>> + }
> >>> + if ((ret = pread(dd->dfd, dd->compressed_page, pd.size, pd.offset)) != pd.size) {
> >>> + if (ret == -1 && CRASHDEBUG(8)) {
> >>> + fprintf(fp,
> >>> + "read_diskdump/cache_page: "
> >>> + "pread error: %s\n", strerror(errno));
> >>> + }
> >>> return READ_ERROR;
> >>> + }
> >>> }
> >>>
> >>> if (pd.flags & DUMP_DH_COMPRESSED_ZLIB) {
> >>> diff --git a/netdump.c b/netdump.c
> >>> index 01af145..696f5fd 100644
> >>> --- a/netdump.c
> >>> +++ b/netdump.c
> >>> @@ -4336,7 +4336,7 @@ no_nt_prstatus_exists:
> >>> int
> >>> read_proc_kcore(int fd, void *bufptr, int cnt, ulong addr, physaddr_t paddr)
> >>> {
> >>> - int i;
> >>> + int i, ret;
> >>> size_t readcnt;
> >>> ulong kvaddr;
> >>> Elf32_Phdr *lp32;
> >>> @@ -4436,11 +4436,20 @@ read_proc_kcore(int fd, void *bufptr, int cnt, ulong addr, physaddr_t paddr)
> >>> if (offset == UNINITIALIZED)
> >>> return SEEK_ERROR;
> >>>
> >>> - if (lseek(fd, offset, SEEK_SET) != offset)
> >>> - perror("lseek");
> >>> -
> >>> - if (read(fd, bufptr, readcnt) != readcnt)
> >>> + if (offset < 0) {
> >>> + if (CRASHDEBUG(8)) {
> >>> + fprintf(fp, "read_proc_kcore: "
> >>> + "invalid offset: %lx\n", offset);
> >>> + }
> >>> + return SEEK_ERROR;
> >>> + }
> >>> + if ((ret = pread(fd, bufptr, readcnt, offset)) != readcnt) {
> >>> + if (ret == -1 && CRASHDEBUG(8)) {
> >>> + fprintf(fp, "read_proc_kcore: "
> >>> + "pread error: %s\n", strerror(errno));
> >>> + }
> >>> return READ_ERROR;
> >>> + }
> >>>
> >>> return cnt;
> >>> }
--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/crash-utility
Contribution Guidelines: https://github.com/crash-utility/crash/wiki
