[Crash-utility] 答复: [External Mail]Re: zram decompress support for gcore/crash-utility
Dave Anderson
anderson at redhat.com
Tue Apr 7 16:44:29 UTC 2020
----- Original Message -----
> Hi,Dave
>
> The latest patch have been attached,please check
>
> Thanks.
In diskdump.c, I should have noticed this before, but there a fundamental
problem with *all* of your readmem() calls, because you qualify each of them
with RETURN_ON_ERROR -- and then just continue on presuming that they were
successful. If RETURN_ON_ERROR is specified, you need to check the return
value of readmem(), and then proceed appropriately.
However, as far as I can tell, all of your readmem() calls should be FAULT_ON_ERROR.
That way, if the readmem() call fails, you will see the error message using your
supplied "type" string, and the command will be aborted immediately.
And speaking of your readmem() error message type strings, please remove
the "readmem " part of the strings. For example,
+ readmem(page + OFFSET(page_private), KVADDR, &zspage,
+ sizeof(void *), "readmem page_private", RETURN_ON_ERROR);
+ readmem(zspage, KVADDR, &zspage_s, sizeof(struct zspage), "readmem zspage", RETURN_ON_ERROR);
Just make them "page_private", "zspage", etc.
And re-thinking memory.c, there really is no point in displaying the
"zram decompress success" message. If try_zram_compress() fails,
you will see the reason why it failed based upon the error message
generated by the failing embedded/secondary readmem() call. If it
works OK, you will see the requested data. Why bother to show "success"?
+ cnt = try_zram_decompress(paddr, (unsigned char *)bufptr, cnt, PAGEOFFSET(addr));
+ if (cnt) {
+ if (CRASHDEBUG(2))
+ error(INFO, "0x%lx zram decompress success\n", addr);
+ bufptr += cnt;
+ addr += cnt;
+ size -= cnt;
+ continue;
+ }
Thanks,
Dave
> ________________________________________
> From: Dave Anderson <anderson at redhat.com>
> Sent: Monday, April 6, 2020 22:38
> To: 赵乾利
> Cc: d hatayama; Discussion list for crash utility usage, maintenance and
> development
> Subject: Re: 答复: [External Mail]Re: [Crash-utility] zram decompress support
> for gcore/crash-utility
>
> ----- Original Message -----
> > patch update
> > Found a better way translate pfn to page,PTOB.
> > Besides,fix a issue with low probability of decompression failure
> >
>
> The changes to defs.h and memory.c look good. My comments for diskdump.c
> are interspersed below, where many of them are redundant:
>
> +#ifdef LZO
> +static unsigned char *zram_object_addr(ulong pool, ulong handle, unsigned
> char *zram_buf)
> +{
> + ulong obj, off, class, page, zspage;
> + struct zspage zspage_s;
> + physaddr_t paddr;
> + unsigned int obj_idx, class_idx, size;
> + ulong pages[2], sizes[2];
> +
> + readmem(handle, KVADDR, &obj, sizeof(void *), "readmem address",
> RETURN_ON_ERROR);
>
> Can you make the error message type string something more helpful than
> "readmem address"?
>
> + obj >>= OBJ_TAG_BITS;
> + phys_to_page(PTOB(obj >> OBJ_INDEX_BITS), &page);
> + obj_idx = (obj & OBJ_INDEX_MASK);
> +
> + readmem(page + OFFSET(page_private), KVADDR, &zspage,
> + sizeof(void *), "readmem address", RETURN_ON_ERROR);
>
> Can you make the error message type string something more helpful than
> "readmem address"?
>
> + readmem(zspage, KVADDR, &zspage_s, sizeof(struct zspage), "readmem
> address",RETURN_ON_ERROR);
>
> Can you make the error message type string something more helpful than
> "readmem address"?
>
> +
> + class_idx = zspage_s.class;
> + if (zspage_s.magic != ZSPAGE_MAGIC)
> + error(FATAL, "zspage magic incorrect:0x%x\n",
> zspage_s.magic);
> +
> + class = pool + OFFSET(zspoll_size_class);
> + class += (class_idx * sizeof(void *));
> + readmem(class, KVADDR, &class, sizeof(void *), "readmem
> address",RETURN_ON_ERROR);
>
> Can you make the error message type string something more helpful than
> "readmem address"?
>
> + readmem(class + OFFSET(size_class_size), KVADDR,
> + &size, sizeof(unsigned int), "readmem address",
> RETURN_ON_ERROR);
>
> Can you make the error message type string something more helpful than
> "readmem address"?
>
> + off = (size * obj_idx) & (~machdep->pagemask);
> + if (off + size <= PAGESIZE()) {
> + if (!is_page_ptr(page, &paddr)) {
> + error(WARNING, "zspage not a page pointer:%lx\n",
> page);
> + return NULL;
> + }
> + readmem(paddr + off, PHYSADDR, zram_buf, size, "readmem zram
> buffer", RETURN_ON_ERROR);
> + goto out;
> + }
> +
> + pages[0] = page;
> + readmem(page + OFFSET(page_freelist), KVADDR, &pages[1],
> + sizeof(void *), "readmem address",RETURN_ON_ERROR);
>
> Can you make the error message type string something more helpful than
> "readmem address"?
>
> + sizes[0] = PAGESIZE() - off;
> + sizes[1] = size - sizes[0];+ readmem(page + OFFSET(page_private), KVADDR, &zspage,
+ sizeof(void *), "readmem page_private", RETURN_ON_ERROR);
+ readmem(zspage, KVADDR, &zspage_s, sizeof(struct zspage), "readmem zspage", RETURN_ON_ERROR);+ readmem(page + OFFSET(page_private), KVADDR, &zspage,
+ sizeof(void *), "readmem page_private", RETURN_ON_ERROR);
+ readmem(zspage, KVADDR, &zspage_s, sizeof(struct zspage), "readmem zspage", RETURN_ON_ERROR);
> + if (!is_page_ptr(pages[0], &paddr)) {
> + error(WARNING, "pages[0] not a page pointer\n");
>
> Maybe display the bogus value in pages[0] in the message?
>
> + return NULL;
> + }
> +
> + readmem(paddr + off, PHYSADDR, zram_buf, sizes[0], "readmem zram
> buffer", RETURN_ON_ERROR);
> + if (!is_page_ptr(pages[1], &paddr)) {
> + error(WARNING, "pages[1] not a page pointer\n");
>
> Maybe display the bogus value in pages[1] in the message?
>
> + return NULL;
> + }
> +
> + readmem(paddr, PHYSADDR, zram_buf + sizes[0], sizes[1], "readmem zram
> buffer", RETURN_ON_ERROR);
> +
> +out:
> + readmem(page, KVADDR, &obj, sizeof(void *), "readmem
> address",RETURN_ON_ERROR);
>
> Can you make the error message type string something more helpful than
> "readmem address"?
>
> + if (!(obj & (1<<10))) { //PG_OwnerPriv1 flag+ readmem(page + OFFSET(page_private), KVADDR, &zspage,
+ sizeof(void *), "readmem page_private", RETURN_ON_ERROR);
+ readmem(zspage, KVADDR, &zspage_s, sizeof(struct zspage), "readmem zspage", RETURN_ON_ERROR);+ readmem(page + OFFSET(page_private), KVADDR, &zspage,
+ sizeof(void *), "readmem page_private", RETURN_ON_ERROR);
+ readmem(zspage, KVADDR, &zspage_s, sizeof(struct zspage), "readmem zspage", RETURN_ON_ERROR);
> + return (zram_buf + ZS_HANDLE_SIZE);
> + }+ readmem(page + OFFSET(page_private), KVADDR, &zspage,
+ sizeof(void *), "readmem page_private", RETURN_ON_ERROR);
+ readmem(zspage, KVADDR, &zspage_s, sizeof(struct zspage), "readmem zspage", RETURN_ON_ERROR);+ readmem(page + OFFSET(page_private), KVADDR, &zspage,
+ sizeof(void *), "readmem page_private", RETURN_ON_ERROR);
+ readmem(zspage, KVADDR, &zspage_s, sizeof(struct zspage), "readmem zspage", RETURN_ON_ERROR);+ readmem(page + OFFSET(page_private), KVADDR, &zspage,
+ sizeof(void *), "readmem page_private", RETURN_ON_ERROR);
+ readmem(zspage, KVADDR, &zspage_s, sizeof(struct zspage), "readmem zspage", RETURN_ON_ERROR);
> +
> + return zram_buf;
> +}
> +
> +static unsigned char *lookup_swap_cache(ulong pte_val, unsigned char
> *zram_buf)
> +{
> + ulong swp_type, swp_offset, swp_space;
> + struct list_pair lp;
> + physaddr_t paddr;
> + swp_type = __swp_type(pte_val);
> + if (THIS_KERNEL_VERSION >= LINUX(2,6,0)) {
> + swp_offset = (ulonglong)__swp_offset(pte_val);
> + } else {
> + swp_offset = (ulonglong)SWP_OFFSET(pte_val);
> + }
> +
> + if (!symbol_exists("swapper_spaces"))
> + return NULL;
> + swp_space = symbol_value("swapper_spaces");
> + swp_space += swp_type * sizeof(void *);
> +
> + readmem(swp_space, KVADDR, &swp_space, sizeof(void *),
> + "readmem address",RETURN_ON_ERROR);
>
> Can you make the error message type string something more helpful than
> "readmem address"?
>
> + swp_space += (swp_offset >> SWAP_ADDRESS_SPACE_SHIFT) *
> SIZE(address_space);
> +
> + lp.index = swp_offset;
> + if (do_radix_tree(swp_space, RADIX_TREE_SEARCH, &lp)){+ readmem(page + OFFSET(page_private), KVADDR, &zspage,
+ sizeof(void *), "readmem page_private", RETURN_ON_ERROR);
+ readmem(zspage, KVADDR, &zspage_s, sizeof(struct zspage), "readmem zspage", RETURN_ON_ERROR);
> + fprintf(fp, "Find page in swap cache\n");
>
> I don't think you meant to leave this message, right?
>
> + if (!is_page_ptr((ulong)lp.value, &paddr)) {
> + error(WARNING, "radix page not a page pointer\n");
> + return NULL;
> + }
> + readmem(paddr, PHYSADDR, zram_buf, PAGESIZE(), "readmem zram
> buffer", RETURN_ON_ERROR);
> + return zram_buf;
> + }
> + return NULL;
> +}
> +
> +ulong (*decompressor)(unsigned char *in_addr, ulong in_size, unsigned char
> *out_addr, ulong *out_size, void *other/* NOT USED */);
> +/*
> +try_zram_decompress returns decompressed page data and data length
> +If userspace address was swaped out to zram,call this function to decompress
> this object.
> +*/
> +ulong try_zram_decompress(ulong pte_val, unsigned char *buf, ulong len,
> uint32_t off)
> +{
> + char name[32] = {0};
> + ulonglong swp_offset;
> + ulong swap_info, bdev, bd_disk, zram, zram_table_entry, sector,
> index, entry, flags, size, outsize;
> + unsigned char *obj_addr = NULL;
> + unsigned char *zram_buf = NULL;
> + unsigned char *outbuf = NULL;+ readmem(page + OFFSET(page_private), KVADDR, &zspage,
+ sizeof(void *), "readmem page_private", RETURN_ON_ERROR);
+ readmem(zspage, KVADDR, &zspage_s, sizeof(struct zspage), "readmem zspage", RETURN_ON_ERROR);
> +
> +
> + if (!symbol_exists("swap_info"))
> + return 0;
> +
> + swap_info = symbol_value("swap_info");
> +
> + swap_info_init();
> + if (vt->flags & SWAPINFO_V2) {
> + swap_info += (__swp_type(pte_val) * sizeof(void *));
> + readmem(swap_info, KVADDR, &swap_info,
> + sizeof(void *), "readmem address",
> RETURN_ON_ERROR);
>
> Can you make the error message type string something more helpful than
> "readmem address"?
>
> + } else {
> + swap_info += (SIZE(swap_info_struct) * __swp_type(pte_val));
> + }
> +
> + readmem(swap_info + OFFSET(swap_info_struct_bdev), KVADDR, &bdev,
> + sizeof(void *), "read swap_info_struct_bdev",
> RETURN_ON_ERROR);
> + readmem(bdev + OFFSET(block_device_bd_disk), KVADDR, &bd_disk,
> + sizeof(void *), "read block_device_bd_disk",
> RETURN_ON_ERROR);
> + readmem(bd_disk + OFFSET(gendisk_disk_name), KVADDR, name,
> + strlen("zram"), "read gendisk_disk_name",
> RETURN_ON_ERROR);
> + if (!strncmp(name, "zram", strlen("zram"))) {
> + if (CRASHDEBUG(2))
> + error(WARNING, "This page has swaped to zram\n");
> +
> + readmem(bd_disk + OFFSET(gendisk_private_data), KVADDR,
> &zram,
> + sizeof(void *), "gendisk_private_data",
> RETURN_ON_ERROR);
> +
> + readmem(zram + OFFSET(zram_compressor), KVADDR, name,
> + sizeof(name), "zram compressor", RETURN_ON_ERROR);
> + if (!strncmp(name, "lzo", strlen("lzo"))) {
> + if (!(dd->flags & LZO_SUPPORTED)) {
> + if (lzo_init() == LZO_E_OK)
> + dd->flags |= LZO_SUPPORTED;
> + else
> + return 0;
> + }
> + decompressor = (void *)lzo1x_decompress_safe;+ readmem(page + OFFSET(page_private), KVADDR, &zspage,
+ sizeof(void *), "readmem page_private", RETURN_ON_ERROR);
+ readmem(zspage, KVADDR, &zspage_s, sizeof(struct zspage), "readmem zspage", RETURN_ON_ERROR);+ readmem(page + OFFSET(page_private), KVADDR, &zspage,
+ sizeof(void *), "readmem page_private", RETURN_ON_ERROR);
+ readmem(zspage, KVADDR, &zspage_s, sizeof(struct zspage), "readmem zspage", RETURN_ON_ERROR);
> + } else {//todo,support more compressor
> + error(WARNING, "Only support lzo compressor\n");
> + return 0;
> + }
> +
> + if (THIS_KERNEL_VERSION >= LINUX(2, 6, 0)) {
> + swp_offset = (ulonglong)__swp_offset(pte_val);
> + } else {
> + swp_offset = (ulonglong)SWP_OFFSET(pte_val);
> + }
> +
> + if ((zram_buf = (unsigned char*)malloc(PAGESIZE())) == NULL)
> + error(FATAL, "cannot malloc zram_buf space.");
>
> Never use malloc/free for temporary buffers during runtime. Either use
> GETBUF()/FREEBUF()
> or just put it on the stack.
>
> +
> + /*lookup page from swap cache*/
> + obj_addr = lookup_swap_cache(pte_val, zram_buf);
> + if (obj_addr != NULL) {
> + memcpy(buf, obj_addr + off, len);
> + goto out;
> + }
> +
> + sector = swp_offset << (PAGESHIFT() - 9);
> + index = sector >> SECTORS_PER_PAGE_SHIFT;
> + readmem(zram, KVADDR, &zram_table_entry,
> + sizeof(void *), "readmem address",
> RETURN_ON_ERROR);
>
> Can you make the error message type string something more helpful than
> "readmem address"?
>
> + zram_table_entry += (index * SIZE(zram_table_entry));
> + readmem(zram_table_entry, KVADDR, &entry,
> + sizeof(void *), "readmem address",
> RETURN_ON_ERROR);
>
> Can you make the error message type string something more helpful than
> "readmem address"?
>
> + readmem(zram_table_entry + OFFSET(zram_table_flag), KVADDR,
> &flags,
> + sizeof(void *), "readmem address",
> RETURN_ON_ERROR);
>
> Can you make the error message type string something more helpful than
> "readmem address"?
>
> + if (!entry || (flags & ZRAM_FLAG_SAME_BIT)) {
> + memset(buf, entry, len);
> + goto out;
> + }
> + size = flags & (ZRAM_FLAG_SHIFT -1);
> + if (size == 0) {
> + len = 0;
> + goto out;
> + }
> +
> + readmem(zram + OFFSET(zram_mempoll), KVADDR, &zram,
> + sizeof(void *), "readmem address",
> RETURN_ON_ERROR);
>
> Can you make the error message type string something more helpful than
> "readmem address"?
>
> +
> + obj_addr = zram_object_addr(zram, entry, zram_buf);
> + if (obj_addr == NULL) {
> + len = 0;
> + goto out;
> + }
> +
> + if (size == PAGESIZE()) {
> + memcpy(buf, obj_addr + off, len);
> + } else {
> + if ((outbuf = (unsigned char*)malloc(PAGESIZE())) ==
> NULL)
> + error(FATAL, "cannot malloc outbuf space.");
>
> Never use malloc/free for temporary buffers during runtime. Either use
> GETBUF()/FREEBUF()
> or just put it on the stack.
>
> +
> + if (!decompressor(obj_addr, size, outbuf, &outsize,
> NULL))
> + memcpy(buf, outbuf + off, len);
> + else {
> + error(WARNING, "zram decompress error\n");
> + len = 0;
> + }
> + free(outbuf);
> + }
> + }
> +
> +out:
> + free(zram_buf);+ readmem(page + OFFSET(page_private), KVADDR, &zspage,
+ sizeof(void *), "readmem page_private", RETURN_ON_ERROR);
+ readmem(zspage, KVADDR, &zspage_s, sizeof(struct zspage), "readmem zspage", RETURN_ON_ERROR);
> + return len;
> +}
> +#else
> +ulong try_zram_decompress(ulong pte_val, unsigned char *buf, ulong len,
> uint32_t off)
> +{
> + return 0;
> +}
> +#endif
>
> #/******本邮件及其附件含有小米公司的保密信息，仅限于发送给上面地址中列出的个人或群组。禁止任何其他人以任何形式使用（包括但不限于全部或部分地泄露、复制、或散发）本邮件中的信息。如果您错收了本邮件，请您立即电话或邮件通知发件人并删除本邮件！
> This e-mail and its attachments contain confidential information from
> XIAOMI, which is intended only for the person or entity whose address is
> listed above. Any use of the information contained herein in any way
> (including, but not limited to, total or partial disclosure, reproduction,
> or dissemination) by persons other than the intended recipient(s) is
> prohibited. If you receive this e-mail in error, please notify the sender by
> phone or email immediately and delete it!******/#
>
More information about the Crash-utility
mailing list