10:08 p.m.
Hi Kazu,
On Fri, Apr 21, 2023 at 10:41 AM HAGIO KAZUHITO(萩尾 一仁)
<k-hagio-ab(a)nec.com> 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(a)nec.com> 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(a)redhat.com>
>>> ---
>>>
>>> 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(a)nec.com>
>>
> 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;
>>> }