>>> On 2023/03/25 13:12, 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. >>> >>> This is nice as an optimization, but can we distinguish a seek error and >>> read error with errno as currently we can do? >>> >> >> Sorry I didn't understand your question... >> >> The functions which I modified in this patch didn't use a global errno >> to distinguish error types, they used a returned value instead. >> Currently if read error, READ_ERROR will be returned, same as seek >> error returns SEEK_ERROR. Do you want to distinguish pread error by >> introducing a new error number as PREAD_ERROR? > > No, I would like to know which causes an error in pread(), seek or read. > In other words, is there any way to know which is wrong, the specified > offset or file/media? > > Good questions. In fact, the SEEK_ERROR is simply returned to the top-level call, but it seems that there is no corresponding processing for the type of errors at the top-level code. Only saw two cases(please correct me if I missed anything): [1] readmem() ... case SEEK_ERROR: if (PRINT_ERROR_MESSAGE) error(INFO, SEEK_ERRMSG, memtype_string(memtype, 0), addr, type); goto readmem_error; case READ_ERROR: if (PRINT_ERROR_MESSAGE) error(INFO, READ_ERRMSG, memtype_string(memtype, 0), addr, type); if ((pc->flags & DEVMEM) && (kt->flags & PRE_KERNEL_INIT) && !(error_handle & NO_DEVMEM_SWITCH) && devmem_is_restricted() && switch_to_proc_kcore()) { error_handle &= ~QUIET; return(readmem(addr, memtype, bufptr, size, type, error_handle)); } goto readmem_error;.. [2] read_diskdump() ... if ((ret = cache_page(curpaddr)) < 0) { if (CRASHDEBUG(8)) fprintf(fp, "read_diskdump: " "%s: cannot cache page: %llx\n", ret == SEEK_ERROR ? "SEEK_ERROR" : "READ_ERROR", (ulonglong)curpaddr); return ret; } ... Given that, only recognizing the type of errors, it can not do anything. Would it really make sense to distinguish the type of errors(seek/read errors)?

BTW: Could it be helpful to print an actual error when the pread() fails? For example: + ssize_t prdcnt = pread(fd, bufptr, readcnt, offset); + if (prdcnt != readcnt) { + if (prdcnt == -1) + error(WARNING, "/proc/kcore: %s\n", strerror(errno)); return READ_ERROR; + }

Thank you for sharing your thoughts, Kazu. On Thu, Apr 13, 2023 at 9:06 AM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab(a)nec.com&gt; wrote: > On 2023/04/12 18:04, lijiang wrote: >>>>> On 2023/03/25 13:12, 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. >>>>> >>>>> This is nice as an optimization, but can we distinguish a seek error > and >>>>> read error with errno as currently we can do? >>>>> >>>> >>>> Sorry I didn't understand your question... >>>> >>>> The functions which I modified in this patch didn't use a global errno >>>> to distinguish error types, they used a returned value instead. >>>> Currently if read error, READ_ERROR will be returned, same as seek >>>> error returns SEEK_ERROR. Do you want to distinguish pread error by >>>> introducing a new error number as PREAD_ERROR? >>> >>> No, I would like to know which causes an error in pread(), seek or read. >>> In other words, is there any way to know which is wrong, the specified >>> offset or file/media? >>> >>> >> Good questions. >> >> In fact, the SEEK_ERROR is simply returned to the top-level call, but it >> seems that there is no corresponding processing for the type of errors at >> the top-level code. Only saw two cases(please correct me if I missed >> anything): >> [1] readmem() >> ... >> case SEEK_ERROR: >> if (PRINT_ERROR_MESSAGE) >> error(INFO, SEEK_ERRMSG, >> memtype_string(memtype, 0), addr, type); >> goto readmem_error; >> >> case READ_ERROR: >> if (PRINT_ERROR_MESSAGE) >> error(INFO, READ_ERRMSG, >> memtype_string(memtype, 0), addr, type); >> if ((pc->flags & DEVMEM) && (kt->flags & >> PRE_KERNEL_INIT) && >> !(error_handle & NO_DEVMEM_SWITCH) && >> devmem_is_restricted() && >> switch_to_proc_kcore()) { >> error_handle &= ~QUIET; >> return(readmem(addr, memtype, bufptr, > size, >> type, error_handle)); >> } >> goto readmem_error;.. >> >> [2] read_diskdump() >> ... >> >> if ((ret = cache_page(curpaddr)) < 0) { >> if (CRASHDEBUG(8)) >> fprintf(fp, "read_diskdump: " >> "%s: cannot cache page: %llx\n", >> ret == SEEK_ERROR ? >> "SEEK_ERROR" : "READ_ERROR", >> (ulonglong)curpaddr); >> return ret; >> } >> ... >> >> Given that, only recognizing the type of errors, it can not do anything. >> Would it really make sense to distinguish the type of errors(seek/read >> errors)? > > I can see some seek errors on GitHub issues [1] and I don't think that > it's useless at all for the first step of debugging. > > [1] https://github.com/crash-utility/crash/issues?q=is%3Aissue+seek+error > > But according to the man page and some experiments, lseek() allows an > offset beyond the end of the file for normal dumpfiles (a vmcore, For this case, crash tool may leave a hole(gap) in a file after writing any data, but usually crash tool won't change the vmcore data.

> /proc/kcore and etc.) and just does not allow a negative offset. > So, my concern will be mostly solved by adding a negative offset check, > for example, > > if (offset < 0) { > if (CRASHDEBUG(8)) > fprintf(fp, " ... offset: %llx\n", offset); > return SEEK_ERROR; > } > > How about adding something like this, instead of just removing lseek? > > I would tend to replace the lseek()/read() with the pread() here, and print the errno when the pread() failed, that is close to the real error, because pread() may fail and set errno to any error specified for read() or lseek().

Otherwise, users have to still suffer from significant performance overhead for the following events: 24.89% crash [kernel.vmlinux] [k] syscall_exit_to_user_mode 12.62% crash [kernel.vmlinux] [k] syscall_return_via_sysret 10.76% crash [kernel.vmlinux] [k] entry_SYSCALL_64 3.43% crash [kernel.vmlinux] [k] read_kcore ... But anyway, this needs more trade-offs, and depends on how we make good choices based on the debugging demands. In other words, which one is more important to users. Actually, the performance issues have been bothering us for a long time. Thanks. Lianbo > >> BTW: Could it be helpful to print an actual error when the pread() fails? >> For example: >> >> + ssize_t prdcnt = pread(fd, bufptr, readcnt, offset); >> + if (prdcnt != readcnt) { >> + if (prdcnt == -1) >> + error(WARNING, "/proc/kcore: %s\n", >> strerror(errno)); >> return READ_ERROR; >> + } > > Yes, it's also sounds good to print errno for better debugging. > I would prefer it with CRASHDEBUG(8) same as the other error messages. > > Thanks, > Kazu >

On Fri, Apr 14, 2023 at 3:12 PM HAGIO KAZUHITO(萩尾　一仁) <k-hagio-ab(a)nec.com&gt; wrote: > > On 2023/04/14 15:34, lijiang wrote: > > Thank you for sharing your thoughts, Kazu. > > > > On Thu, Apr 13, 2023 at 9:06 AM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab(a)nec.com&gt; > > wrote: > > > >> On 2023/04/12 18:04, lijiang wrote: > >>>>>> On 2023/03/25 13:12, 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. > >>>>>> > >>>>>> This is nice as an optimization, but can we distinguish a seek error > >> and > >>>>>> read error with errno as currently we can do? > >>>>>> > >>>>> > >>>>> Sorry I didn't understand your question... > >>>>> > >>>>> The functions which I modified in this patch didn't use a global errno > >>>>> to distinguish error types, they used a returned value instead. > >>>>> Currently if read error, READ_ERROR will be returned, same as seek > >>>>> error returns SEEK_ERROR. Do you want to distinguish pread error by > >>>>> introducing a new error number as PREAD_ERROR? > >>>> > >>>> No, I would like to know which causes an error in pread(), seek or read. > >>>> In other words, is there any way to know which is wrong, the specified > >>>> offset or file/media? > >>>> > >>>> > >>> Good questions. > >>> > >>> In fact, the SEEK_ERROR is simply returned to the top-level call, but it > >>> seems that there is no corresponding processing for the type of errors at > >>> the top-level code. Only saw two cases(please correct me if I missed > >>> anything): > >>> [1] readmem() > >>> ... > >>> case SEEK_ERROR: > >>> if (PRINT_ERROR_MESSAGE) > >>> error(INFO, SEEK_ERRMSG, > >>> memtype_string(memtype, 0), addr, type); > >>> goto readmem_error; > >>> > >>> case READ_ERROR: > >>> if (PRINT_ERROR_MESSAGE) > >>> error(INFO, READ_ERRMSG, > >>> memtype_string(memtype, 0), addr, type); > >>> if ((pc->flags & DEVMEM) && (kt->flags & > >>> PRE_KERNEL_INIT) && > >>> !(error_handle & NO_DEVMEM_SWITCH) && > >>> devmem_is_restricted() && > >>> switch_to_proc_kcore()) { > >>> error_handle &= ~QUIET; > >>> return(readmem(addr, memtype, bufptr, > >> size, > >>> type, error_handle)); > >>> } > >>> goto readmem_error;.. > >>> > >>> [2] read_diskdump() > >>> ... > >>> > >>> if ((ret = cache_page(curpaddr)) < 0) { > >>> if (CRASHDEBUG(8)) > >>> fprintf(fp, "read_diskdump: " > >>> "%s: cannot cache page: %llx\n", > >>> ret == SEEK_ERROR ? > >>> "SEEK_ERROR" : "READ_ERROR", > >>> (ulonglong)curpaddr); > >>> return ret; > >>> } > >>> ... > >>> > >>> Given that, only recognizing the type of errors, it can not do anything. > >>> Would it really make sense to distinguish the type of errors(seek/read > >>> errors)? > >> > >> I can see some seek errors on GitHub issues [1] and I don't think that > >> it's useless at all for the first step of debugging. > >> > >> [1] https://github.com/crash-utility/crash/issues?q=is%3Aissue+seek+error > >> > >> But according to the man page and some experiments, lseek() allows an > >> offset beyond the end of the file for normal dumpfiles (a vmcore, > > > > > > For this case, crash tool may leave a hole(gap) in a file after writing any > > data, but usually crash tool won't change the vmcore data. > > hmm, sorry, I might lack words.. > > I meant that the check below can return a seek error mostly like lseek() > does, because lseek() doesn't return an error for positive values. > > > > > > >> /proc/kcore and etc.) and just does not allow a negative offset. > >> > > So, my concern will be mostly solved by adding a negative offset check, > >> for example, > >> > >> if (offset < 0) { > >> if (CRASHDEBUG(8)) > >> fprintf(fp, " ... offset: %llx\n", offset); > >> return SEEK_ERROR; > >> } > >> > >> How about adding something like this, instead of just removing lseek? > >> > >> > > I would tend to replace the lseek()/read() with the pread() here, and print > > the errno when the pread() failed, that is close to the real error, because > > pread() may fail and set errno to any error specified for read() or lseek(). > > With the reason above, I meant to replace the lseek() and read() with > the light offset check above and pread(). The check will not affect the > performance. > > And yes, we can also add the errno message. Thank you for the explanation, Kazu. This sounds good to me. Could you please improve and post it again as a separate patch based on the above suggestions?Tao. (Or Kazu can help to merge this patch according to our discussion.)

BTW: for the multi thread patches, I would focus on this issue: Is it possible to make the multi thread code as a common library to serve for startup and other crash commands?(But anyway, still need to be discussed.)

Thanks. Lianbo > > > > > Otherwise, users have to still suffer from significant performance overhead > > for the following events: > > > > 24.89% crash [kernel.vmlinux] [k] syscall_exit_to_user_mode > > 12.62% crash [kernel.vmlinux] [k] syscall_return_via_sysret > > 10.76% crash [kernel.vmlinux] [k] entry_SYSCALL_64 > > 3.43% crash [kernel.vmlinux] [k] read_kcore > > ... > > > > But anyway, this needs more trade-offs, and depends on how we make good > > choices based on the debugging demands. In other words, which one is more > > important to users. Actually, the performance issues have been bothering us > > for a long time. > > > > Thanks. > > Lianbo > > > >> > >>> BTW: Could it be helpful to print an actual error when the pread() fails? > >>> For example: > >>> > >>> + ssize_t prdcnt = pread(fd, bufptr, readcnt, offset); > >>> + if (prdcnt != readcnt) { > >>> + if (prdcnt == -1) > >>> + error(WARNING, "/proc/kcore: %s\n", > >>> strerror(errno)); > >>> return READ_ERROR; > >>> + } > >> > >> Yes, it's also sounds good to print errno for better debugging. > >> I would prefer it with CRASHDEBUG(8) same as the other error messages. > >> > >> Thanks, > >> Kazu > >>