Hi Kazuhito,

The first step in the optimization of is_page_ptr() is checked in:

  
https://github.com/crash-utility/crash/commit/d586679b861fafc99e96c863105826d30de630a7

Thanks,
  Dave

----- Original Message -----
> Hi Dave,
> 
> On 2/27/2018 4:45 PM, Kazuhito Hagio wrote:
> [...]
> >> First, the mem_section numbers are ascending.  They may not necessarily
> >> start
> >> with 0, and there may be holes, but they are ascending.  That being the
> >> case,
> >> there is no need for is_page_ptr() to walk through NR_MEM_SECTIONS() worth
> >> of entries, because there will be an ending number that's typically much
> >> smaller.  Even on a 256GB dumpfile I have on hand, which has a
> >> NR_MEM_SECTIONS()
> >> value of 524288, the largest valid section number is 2055. (What is the
> >> smallest
> >> and largest number that you see on whatever large-memory system that you
> >> are
> >> testing with?)
> >>
> >> In any case, let's store the largest section number during initialization
> >> in
> >> the vm_table, and use it as a delimeter in is_page_ptr().
> > 
> > I agree with you.  This will improve the worst case of the loop.  Also,
> > if the binary search is implemented in the future, it could be utilized.
> > (The largest valid section numbers of each architecture in my test logs
> > are 1543 on a 192GB x86_64 and 2047 on a 32GB ppc64.)
> 
> I checked and tested the former patch you proposed below as it is
> and I didn't find any problem.  Could you merge this?
> (or is there anything I should do?)
> 
> > 
> >> diff --git a/defs.h b/defs.h
> >> index aa17792..8768fd5 100644
> >> --- a/defs.h
> >> +++ b/defs.h
> >> @@ -2369,6 +2369,7 @@ struct vm_table {                /* kernel
> >> VM-related data */
> >>            ulong mask;
> >>            char *name;
> >>    } *pageflags_data;
> >> +  ulong max_mem_section_nr;
> >>  };
> >>  
> >>  #define NODES                       (0x1)
> >> diff --git a/memory.c b/memory.c
> >> index 0df8ecc..6770937 100644
> >> --- a/memory.c
> >> +++ b/memory.c
> >> @@ -255,7 +255,7 @@ static void PG_reserved_flag_init(void);
> >>  static void PG_slab_flag_init(void);
> >>  static ulong nr_blockdev_pages(void);
> >>  void sparse_mem_init(void);
> >> -void dump_mem_sections(void);
> >> +void dump_mem_sections(int);
> >>  void list_mem_sections(void);
> >>  ulong sparse_decode_mem_map(ulong, ulong);
> >>  char *read_mem_section(ulong);
> >> @@ -13350,7 +13350,7 @@ is_page_ptr(ulong addr, physaddr_t *phys)
> >>    physaddr_t section_paddr;
> >>  
> >>    if (IS_SPARSEMEM()) {
> >> -          nr_mem_sections = NR_MEM_SECTIONS();
> >> +          nr_mem_sections = vt->max_mem_section_nr+1;
> >>            for (nr = 0; nr < nr_mem_sections ; nr++) {
> >>                    if ((sec_addr = valid_section_nr(nr))) {
> >>                            coded_mem_map = section_mem_map_addr(sec_addr);
> >> @@ -13668,6 +13668,7 @@ dump_vm_table(int verbose)
> >>    fprintf(fp, "   swap_info_struct: %lx\n", (ulong)vt->swap_info_struct);
> >>    fprintf(fp, "            mem_sec: %lx\n", (ulong)vt->mem_sec);
> >>    fprintf(fp, "        mem_section: %lx\n", (ulong)vt->mem_section);
> >> +  fprintf(fp, " max_mem_section_nr: %ld\n", vt->max_mem_section_nr);
> >>    fprintf(fp, "       ZONE_HIGHMEM: %d\n", vt->ZONE_HIGHMEM);
> >>    fprintf(fp, "node_online_map_len: %d\n", vt->node_online_map_len);
> >>    if (vt->node_online_map_len) {
> >> @@ -16295,8 +16296,8 @@ dump_memory_nodes(int initialize)
> >>            vt->numnodes = n;
> >>    }
> >>  
> >> -  if (!initialize && IS_SPARSEMEM())
> >> -          dump_mem_sections();
> >> +  if (IS_SPARSEMEM())
> >> +          dump_mem_sections(initialize);
> >>  }
> >>  
> >>  /*
> >> @@ -17128,9 +17129,9 @@ pfn_to_map(ulong pfn)
> >>  }
> >>  
> >>  void
> >> -dump_mem_sections(void)
> >> +dump_mem_sections(int initialize)
> >>  {
> >> -  ulong nr,addr;
> >> +  ulong nr, max, addr;
> >>    ulong nr_mem_sections;
> >>    ulong coded_mem_map, mem_map, pfn;
> >>    char buf1[BUFSIZE];
> >> @@ -17140,6 +17141,15 @@ dump_mem_sections(void)
> >>  
> >>    nr_mem_sections = NR_MEM_SECTIONS();
> >>  
> >> +  if (initialize) {
> >> +          for (nr = max = 0; nr < nr_mem_sections ; nr++) {
> >> +                  if (valid_section_nr(nr))
> >> +                          max = nr;
> >> +          }
> >> +          vt->max_mem_section_nr = max;
> >> +          return;
> >> +  }
> >> +
> >>    fprintf(fp, "\n");
> >>    pad_line(fp, BITS32() ? 59 : 67, '-');
> >>          fprintf(fp, "\n\nNR  %s  %s  %s  PFN\n",
> >>
> >>
> >> Now, with respect to the architecture-specific, VMEMMAP-only, part
> >> that is of most interest to you, let's do it with an architecture
> >> specific callback.  You can post it for x86_64, and the other architecture
> >> maintainers can write their own version.  For example, add a new
> >> callback function to the machdep_table structure, i.e., like this:
> >>
> >>   struct machdep_table {
> >>           ulong flags;
> >>           ulong kvbase;
> >>   ...
> >>           void (*get_irq_affinity)(int);
> >>           void (*show_interrupts)(int, ulong *);
> >> +         int is_page_ptr(ulong, physaddr_t *);
> >>   };
> >>   
> >> Write the x86_64_is_page_ptr() function that works for VMEMMAP
> >> kernels, and returns FALSE otherwise.  And add the call to the top
> >> of is_page_ptr() like this:
> >>
> >>   int
> >>   is_page_ptr(ulong addr, physaddr_t *phys)
> >>   {
> >>           int n;
> >>           ulong ppstart, ppend;
> >>           struct node_table *nt;
> >>           ulong pgnum, node_size;
> >>           ulong nr, sec_addr;
> >>           ulong nr_mem_sections;
> >>           ulong coded_mem_map, mem_map, end_mem_map;
> >>           physaddr_t section_paddr;
> >>
> >> +    if (machdep->is_page_ptr(addr, phys))
> >> +            return TRUE;
> >>   
> >>           if (IS_SPARSEMEM()) {
> >>   ...
> >>   
> >> To avoid having to check whether the machdep->is_page_ptr function
> >> exists, write a generic_is_page_ptr() function that just returns
> >> FALSE, and initialize machdep->is_page_ptr to generic_is_page_ptr in
> >> the setup_environment() function.  Later on, individual architectures
> >> can overwrite it when machdep_init(SETUP_ENV) is called.
> >>
> >> How's that sound?
> > 
> > It looks readable and refined.
> > 
> > If an incoming address is not a page address, the IS_SPARSEMEM() section
> > is also executed, but I think that it does not matter because it is rare
> > that the situation occurs many times at once and it is likely that the code
> > will become ugly to avoid it.
> > 
> > So I'll prepare the x86_64 part based on the above.
> 
> I thought that you would merge the common part, but is it wrong?
> Could I post it with the x86_64 part?
> 
> Sorry I didn't understand well how to proceed with this.
> And thank you very much for your help with this issue!
> 
> Kazuhito Hagio
> 
> > 
> > Thanks,
> > Kazuhito Hagio
> > 
> >>
> >> Dave
> >>
> >> --
> >> Crash-utility mailing list
> >> Crash-utility@redhat.com
> >> https://www.redhat.com/mailman/listinfo/crash-utility
> >>
> > 
> > --
> > Crash-utility mailing list
> > Crash-utility@redhat.com
> > https://www.redhat.com/mailman/listinfo/crash-utility
> > 
> 
> --
> Crash-utility mailing list
> Crash-utility@redhat.com
> https://www.redhat.com/mailman/listinfo/crash-utility
> 

--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility

Reply via email to