[PATCH] makedumpfile: clean up draft patch

[Atsushi Kumagai]

Hello Baoquan,

Sorry for delay response, I entered a vacation just before
you posted this.

On 2014/01/07 10:51:29, kexec <kexec-bounces at lists.infradead.org> wrote:
> Hi Atsushi,
> 
> Could you please review this draft patch? As we discussed before,
> function update_cyclic_region not only update the cyclic region, but
> also handle the 1st bitmap and 2nd bitmap. For better understanding and
> process, it need be cleaned up. In this patch, struct cycle is
> introduced and can be passed down into functions, but not global. I
> think this clean up is helpful, and no risk.
> 
> What'a your opinion?

Thanks for your work, this idea looks good to me.
However, I found a mistake:

> +static void update_cycle(unsigned long long max, struct cycle *cycle)
> +{
> +	cycle->start_pfn= cycle->end_pfn + 1;
> +	cycle->end_pfn=  cycle->start_pfn + info->pfn_cyclic;
> +
> +	if (cycle->end_pfn > max)
> +		cycle->end_pfn = max;
> +}

According to this function, you must treat a range of a cycle
as below:

   |<----------------- a cycle ------------------->|
---+-----------+-----------+-----------+-----------+----
   |   pfn:N   |  pfn:N+1  |  pfn:N+2  |  pfn:N+3  | ...
---+-----------+-----------+-----------+-----------+----
         A                                   A
         |                                   |
     start_pfn                            end_pfn

However, the existing code treat it as:

   |<------------ a cycle ------------>|
---+-----------+-----------+-----------+-----------+----
   |   pfn:N   |  pfn:N+1  |  pfn:N+2  |  pfn:N+3  | ...
---+-----------+-----------+-----------+-----------+----
         A                                   A
         |                                   |
     start_pfn                            end_pfn

This can be implied from the current iterator like below:

     for (pfn = start_pfn; pfn < end_pfn; pfn++) {
                           ^^^^^^^^^^^^^^
In short, the existing code will not work correctly.

You may feel this design is strange.
This came from the fact that the end of the region's *address*
is the head of pfn:N+1 while it can also be said the tail of pfn:N.
(And this came from the initial version.)

BTW, info->cyclic_start_pfn and info->cyclic_end_pfn are no longer
used, we can get rid of those from makedumpfile.h.


Thanks
Atsushi Kumagai

> Baoquan
> Thanks
> 
> 
> On 12/27/13 at 08:12pm, Baoquan He wrote:
> > Introduce the struct cycle as below just as Hatamaya expected.
> > struct cycle {
> >         uint64_t start_pfn;
> >         uint64_t end_pfn;
> > };
> > 
> >   for (first_cycle(start, max, C); !end_cycle(max, C); \
> >        update_cycle(max, C))
> > 
> > Hi Atsushi and HATAYAMA,
> > 
> > Please help review this draft patch and check if there's any potentia
> > risk. If you think this is in the right way, I can post a formal
> > patchset. I just test the normal operation on kdump and elf format,
> > it works well.
> > 
> > Signed-off-by: Baoquan He <bhe at redhat.com>
> > ---
> 
> _______________________________________________
> kexec mailing list
> kexec at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec