Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1363185 > unrolled thread

Re: [PATCH V2 0/2] kexec: Make a pair of map/unmap reserved pages in error path

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH V2 0/2] kexec: Make a pair of map/unmap reserved pages in  error path Baoquan He <bhe@redhat.com> - 2016-03-23 04:00 +0100
    Re: [PATCH V2 0/2] kexec: Make a pair of map/unmap reserved pages in  error path Xunlei Pang <xpang@redhat.com> - 2016-03-23 04:40 +0100
      Re: [PATCH V2 0/2] kexec: Make a pair of map/unmap reserved pages in  error path Baoquan He <bhe@redhat.com> - 2016-03-23 09:30 +0100
        Re: [PATCH V2 0/2] kexec: Make a pair of map/unmap reserved pages in  error path Xunlei Pang <xpang@redhat.com> - 2016-03-23 11:00 +0100
          Re: [PATCH V2 0/2] kexec: Make a pair of map/unmap reserved pages in  error path Baoquan He <bhe@redhat.com> - 2016-03-23 13:40 +0100
            Re: [PATCH V2 0/2] kexec: Make a pair of map/unmap reserved pages in  error path Xunlei Pang <xpang@redhat.com> - 2016-03-24 09:40 +0100
    Re: [PATCH V2 0/2] kexec: Make a pair of map/unmap reserved pages in  error path Minfei Huang <mhuang@redhat.com> - 2016-03-26 16:20 +0100

#1363185 — Re: [PATCH V2 0/2] kexec: Make a pair of map/unmap reserved pages in error path

On 03/01/16 at 05:53pm, Xunlei Pang wrote:
> This is a bug fix.
> 
> After this, I will try to do a cleanup for crash_unmap/map_reserved_pages()
> (only used by S390) to consolidate it with arch_kexec_unprotect/protect_crashkres().

Hi Xunlei, Minfei,

I think you need discuss together about how to do clean up codes in this
place. From my point of view, arch_map/unmap_reserved_pages and
arch_kexec_protect/unprotect_crashkres() are for the same goal but by
different ways on different arch. So for Xunlei's patchset, you might
need to rethink your implementation, the name of function. I personally
think you just implement a x86 specific arch_map/unmap_reserved_pages.
It may need a more generic name, and then add your x86 arch specific
implementation. Sorry I can't see your patches on my mail client,
Xunlei. Since Andrew asked, I just checked these.

I am fine with Minfei's patch 1/2. But for patch 2/2, it's a little
comfortable to me. Is it really necessary to abstract code block from
kexec_load, then wrap them into a newly added function do_kexec_load()?
Without this wrapping is there a way to do your bug fix? Is there
possibility that do_kexec_load will be called in other places? What's
the benefit to wrap it into do_kexec_load against not wrapping?

Thanks
Baoquan

> 
> Regards,
> Xunlei
> 
> On 03/01/2016 at 04:02 PM, Minfei Huang wrote:
> > v1:
> > - Bisect the patch according to Andrew Morton's suggestion
> >
> > Minfei Huang (2):
> >   kexec: Make a pair of map/unmap reserved pages in error path
> >   kexec: Do a cleanup for function kexec_load
> >
> >  kernel/kexec.c | 112 ++++++++++++++++++++++++++++++++-------------------------
> >  1 file changed, 63 insertions(+), 49 deletions(-)
> >
> 
> 
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec

[toc] | [next] | [standalone]

#1363189

On 2016/03/23 at 10:48, Baoquan He wrote:
> On 03/01/16 at 05:53pm, Xunlei Pang wrote:
>> This is a bug fix.
>>
>> After this, I will try to do a cleanup for crash_unmap/map_reserved_pages()
>> (only used by S390) to consolidate it with arch_kexec_unprotect/protect_crashkres().
> Hi Xunlei, Minfei,
>
> I think you need discuss together about how to do clean up codes in this
> place. From my point of view, arch_map/unmap_reserved_pages and
> arch_kexec_protect/unprotect_crashkres() are for the same goal but by
> different ways on different arch. So for Xunlei's patchset, you might
> need to rethink your implementation, the name of function. I personally
> think you just implement a x86 specific arch_map/unmap_reserved_pages.
> It may need a more generic name, and then add your x86 arch specific
> implementation. Sorry I can't see your patches on my mail client,

Like what you said, I think arch_kexec_unprotect/protect_crashkres() are
generic enough, but any other better name is welcome :-)

It also covered the newly-added kexec file path, and we can easily transfer
arch_map/unmap_reserved_pages into this new interface.

I was planning doing that, but sick recently, I will try to send a patch
doing that later.

Regards,
Xunlei

> Xunlei. Since Andrew asked, I just checked these.
>
> I am fine with Minfei's patch 1/2. But for patch 2/2, it's a little
> comfortable to me. Is it really necessary to abstract code block from
> kexec_load, then wrap them into a newly added function do_kexec_load()?
> Without this wrapping is there a way to do your bug fix? Is there
> possibility that do_kexec_load will be called in other places? What's
> the benefit to wrap it into do_kexec_load against not wrapping?
>
> Thanks
> Baoquan
>
>> Regards,
>> Xunlei
>>
>> On 03/01/2016 at 04:02 PM, Minfei Huang wrote:
>>> v1:
>>> - Bisect the patch according to Andrew Morton's suggestion
>>>
>>> Minfei Huang (2):
>>>   kexec: Make a pair of map/unmap reserved pages in error path
>>>   kexec: Do a cleanup for function kexec_load
>>>
>>>  kernel/kexec.c | 112 ++++++++++++++++++++++++++++++++-------------------------
>>>  1 file changed, 63 insertions(+), 49 deletions(-)
>>>
>>
>> _______________________________________________
>> kexec mailing list
>> kexec@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/kexec

[toc] | [prev] | [next] | [standalone]

#1363239

On 03/23/16 at 11:32am, Xunlei Pang wrote:
> On 2016/03/23 at 10:48, Baoquan He wrote:
> > On 03/01/16 at 05:53pm, Xunlei Pang wrote:
> >> This is a bug fix.
> >>
> >> After this, I will try to do a cleanup for crash_unmap/map_reserved_pages()
> >> (only used by S390) to consolidate it with arch_kexec_unprotect/protect_crashkres().
> > Hi Xunlei, Minfei,
> >
> > I think you need discuss together about how to do clean up codes in this
> > place. From my point of view, arch_map/unmap_reserved_pages and
> > arch_kexec_protect/unprotect_crashkres() are for the same goal but by
> > different ways on different arch. So for Xunlei's patchset, you might
> > need to rethink your implementation, the name of function. I personally
> > think you just implement a x86 specific arch_map/unmap_reserved_pages.
> > It may need a more generic name, and then add your x86 arch specific
> > implementation. Sorry I can't see your patches on my mail client,
> 
> Like what you said, I think arch_kexec_unprotect/protect_crashkres() are
> generic enough, but any other better name is welcome :-)
> 
> It also covered the newly-added kexec file path, and we can easily transfer
> arch_map/unmap_reserved_pages into this new interface.

I don't know the status of your patchset. If possible I think the 1st
patch in your patchset shoule rename arch_map/unmap_reserved_pages to
arch_kexec_protect/unprotect_crashkres, 2nd patch is to add your x86
specific patch.

> 
> I was planning doing that, but sick recently, I will try to send a patch
> doing that later.

Yeah, totally understand. This is not urgent, please take care of
yourself.

> 
> Regards,
> Xunlei
> 
> > Xunlei. Since Andrew asked, I just checked these.
> >
> > I am fine with Minfei's patch 1/2. But for patch 2/2, it's a little
> > comfortable to me. Is it really necessary to abstract code block from
> > kexec_load, then wrap them into a newly added function do_kexec_load()?
> > Without this wrapping is there a way to do your bug fix? Is there
> > possibility that do_kexec_load will be called in other places? What's
> > the benefit to wrap it into do_kexec_load against not wrapping?
> >
> > Thanks
> > Baoquan
> >
> >> Regards,
> >> Xunlei
> >>
> >> On 03/01/2016 at 04:02 PM, Minfei Huang wrote:
> >>> v1:
> >>> - Bisect the patch according to Andrew Morton's suggestion
> >>>
> >>> Minfei Huang (2):
> >>>   kexec: Make a pair of map/unmap reserved pages in error path
> >>>   kexec: Do a cleanup for function kexec_load
> >>>
> >>>  kernel/kexec.c | 112 ++++++++++++++++++++++++++++++++-------------------------
> >>>  1 file changed, 63 insertions(+), 49 deletions(-)
> >>>
> >>
> >> _______________________________________________
> >> kexec mailing list
> >> kexec@lists.infradead.org
> >> http://lists.infradead.org/mailman/listinfo/kexec
> 
> 
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec

[toc] | [prev] | [next] | [standalone]

#1363279

On 2016/03/23 at 16:23, Baoquan He wrote:
> On 03/23/16 at 11:32am, Xunlei Pang wrote:
>> On 2016/03/23 at 10:48, Baoquan He wrote:
>>> On 03/01/16 at 05:53pm, Xunlei Pang wrote:
>>>> This is a bug fix.
>>>>
>>>> After this, I will try to do a cleanup for crash_unmap/map_reserved_pages()
>>>> (only used by S390) to consolidate it with arch_kexec_unprotect/protect_crashkres().
>>> Hi Xunlei, Minfei,
>>>
>>> I think you need discuss together about how to do clean up codes in this
>>> place. From my point of view, arch_map/unmap_reserved_pages and
>>> arch_kexec_protect/unprotect_crashkres() are for the same goal but by
>>> different ways on different arch. So for Xunlei's patchset, you might
>>> need to rethink your implementation, the name of function. I personally
>>> think you just implement a x86 specific arch_map/unmap_reserved_pages.
>>> It may need a more generic name, and then add your x86 arch specific
>>> implementation. Sorry I can't see your patches on my mail client,
>> Like what you said, I think arch_kexec_unprotect/protect_crashkres() are
>> generic enough, but any other better name is welcome :-)
>>
>> It also covered the newly-added kexec file path, and we can easily transfer
>> arch_map/unmap_reserved_pages into this new interface.
> I don't know the status of your patchset. If possible I think the 1st
> patch in your patchset shoule rename arch_map/unmap_reserved_pages to
> arch_kexec_protect/unprotect_crashkres, 2nd patch is to add your x86
> specific patch.

Yes, actually when I filed my patchset, I didn't notice arch_map/unmap_reserved_pages,
too much back then, s390 is its only user, and hard to get the purpose from its name.

But from other point of view, they are a bit different, crash_map_reserved_pages()
is also called by crash_shrink_memory(), it is a bit more complex(and needs some
s390 arch code modification) than just simply renaming/consolidating them, so I think
it's  ok to provide a new generic mechanism first and then put renaming/consolidating
arch work back a little as a separate patch.

Regards,
Xunlei

>
>> I was planning doing that, but sick recently, I will try to send a patch
>> doing that later.
> Yeah, totally understand. This is not urgent, please take care of
> yourself.
>
>> Regards,
>> Xunlei
>>
>>> Xunlei. Since Andrew asked, I just checked these.
>>>
>>> I am fine with Minfei's patch 1/2. But for patch 2/2, it's a little
>>> comfortable to me. Is it really necessary to abstract code block from
>>> kexec_load, then wrap them into a newly added function do_kexec_load()?
>>> Without this wrapping is there a way to do your bug fix? Is there
>>> possibility that do_kexec_load will be called in other places? What's
>>> the benefit to wrap it into do_kexec_load against not wrapping?
>>>
>>> Thanks
>>> Baoquan
>>>
>>>> Regards,
>>>> Xunlei
>>>>
>>>> On 03/01/2016 at 04:02 PM, Minfei Huang wrote:
>>>>> v1:
>>>>> - Bisect the patch according to Andrew Morton's suggestion
>>>>>
>>>>> Minfei Huang (2):
>>>>>   kexec: Make a pair of map/unmap reserved pages in error path
>>>>>   kexec: Do a cleanup for function kexec_load
>>>>>
>>>>>  kernel/kexec.c | 112 ++++++++++++++++++++++++++++++++-------------------------
>>>>>  1 file changed, 63 insertions(+), 49 deletions(-)
>>>>>
>>>> _______________________________________________
>>>> kexec mailing list
>>>> kexec@lists.infradead.org
>>>> http://lists.infradead.org/mailman/listinfo/kexec
>>
>> _______________________________________________
>> kexec mailing list
>> kexec@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/kexec

[toc] | [prev] | [next] | [standalone]

#1363396

On 03/23/16 at 05:59pm, Xunlei Pang wrote:
> On 2016/03/23 at 16:23, Baoquan He wrote:
> > On 03/23/16 at 11:32am, Xunlei Pang wrote:
> >> On 2016/03/23 at 10:48, Baoquan He wrote:
> >>> On 03/01/16 at 05:53pm, Xunlei Pang wrote:
> >>>> This is a bug fix.
> >>>>
> >>>> After this, I will try to do a cleanup for crash_unmap/map_reserved_pages()
> >>>> (only used by S390) to consolidate it with arch_kexec_unprotect/protect_crashkres().
> >>> Hi Xunlei, Minfei,
> >>>
> >>> I think you need discuss together about how to do clean up codes in this
> >>> place. From my point of view, arch_map/unmap_reserved_pages and
> >>> arch_kexec_protect/unprotect_crashkres() are for the same goal but by
> >>> different ways on different arch. So for Xunlei's patchset, you might
> >>> need to rethink your implementation, the name of function. I personally
> >>> think you just implement a x86 specific arch_map/unmap_reserved_pages.
> >>> It may need a more generic name, and then add your x86 arch specific
> >>> implementation. Sorry I can't see your patches on my mail client,
> >> Like what you said, I think arch_kexec_unprotect/protect_crashkres() are
> >> generic enough, but any other better name is welcome :-)
> >>
> >> It also covered the newly-added kexec file path, and we can easily transfer
> >> arch_map/unmap_reserved_pages into this new interface.
> > I don't know the status of your patchset. If possible I think the 1st
> > patch in your patchset shoule rename arch_map/unmap_reserved_pages to
> > arch_kexec_protect/unprotect_crashkres, 2nd patch is to add your x86
> > specific patch.
> 
> Yes, actually when I filed my patchset, I didn't notice arch_map/unmap_reserved_pages,
> too much back then, s390 is its only user, and hard to get the purpose from its name.
> 
> But from other point of view, they are a bit different, crash_map_reserved_pages()
> is also called by crash_shrink_memory(), it is a bit more complex(and needs some
> s390 arch code modification) than just simply renaming/consolidating them, so I think
> it's  ok to provide a new generic mechanism first and then put renaming/consolidating
> arch work back a little as a separate patch.

OK, sounds good, I am fine with this.

How do you think about Minfei's patch? You pick it up into your patchset
in next post with his author, or just wait for him to repost? Apparently
his patch has conflict with yours.

> 
> Regards,
> Xunlei
> 
> >
> >> I was planning doing that, but sick recently, I will try to send a patch
> >> doing that later.
> > Yeah, totally understand. This is not urgent, please take care of
> > yourself.
> >
> >> Regards,
> >> Xunlei
> >>
> >>> Xunlei. Since Andrew asked, I just checked these.
> >>>
> >>> I am fine with Minfei's patch 1/2. But for patch 2/2, it's a little
> >>> comfortable to me. Is it really necessary to abstract code block from
> >>> kexec_load, then wrap them into a newly added function do_kexec_load()?
> >>> Without this wrapping is there a way to do your bug fix? Is there
> >>> possibility that do_kexec_load will be called in other places? What's
> >>> the benefit to wrap it into do_kexec_load against not wrapping?
> >>>
> >>> Thanks
> >>> Baoquan
> >>>
> >>>> Regards,
> >>>> Xunlei
> >>>>
> >>>> On 03/01/2016 at 04:02 PM, Minfei Huang wrote:
> >>>>> v1:
> >>>>> - Bisect the patch according to Andrew Morton's suggestion
> >>>>>
> >>>>> Minfei Huang (2):
> >>>>>   kexec: Make a pair of map/unmap reserved pages in error path
> >>>>>   kexec: Do a cleanup for function kexec_load
> >>>>>
> >>>>>  kernel/kexec.c | 112 ++++++++++++++++++++++++++++++++-------------------------
> >>>>>  1 file changed, 63 insertions(+), 49 deletions(-)
> >>>>>
> >>>> _______________________________________________
> >>>> kexec mailing list
> >>>> kexec@lists.infradead.org
> >>>> http://lists.infradead.org/mailman/listinfo/kexec
> >>
> >> _______________________________________________
> >> kexec mailing list
> >> kexec@lists.infradead.org
> >> http://lists.infradead.org/mailman/listinfo/kexec
> 

[toc] | [prev] | [next] | [standalone]

#1364008

On 2016/03/23 at 20:32, Baoquan He wrote:
> On 03/23/16 at 05:59pm, Xunlei Pang wrote:
>> On 2016/03/23 at 16:23, Baoquan He wrote:
>>> On 03/23/16 at 11:32am, Xunlei Pang wrote:
>>>> On 2016/03/23 at 10:48, Baoquan He wrote:
>>>>> On 03/01/16 at 05:53pm, Xunlei Pang wrote:
>>>>>> This is a bug fix.
>>>>>>
>>>>>> After this, I will try to do a cleanup for crash_unmap/map_reserved_pages()
>>>>>> (only used by S390) to consolidate it with arch_kexec_unprotect/protect_crashkres().
>>>>> Hi Xunlei, Minfei,
>>>>>
>>>>> I think you need discuss together about how to do clean up codes in this
>>>>> place. From my point of view, arch_map/unmap_reserved_pages and
>>>>> arch_kexec_protect/unprotect_crashkres() are for the same goal but by
>>>>> different ways on different arch. So for Xunlei's patchset, you might
>>>>> need to rethink your implementation, the name of function. I personally
>>>>> think you just implement a x86 specific arch_map/unmap_reserved_pages.
>>>>> It may need a more generic name, and then add your x86 arch specific
>>>>> implementation. Sorry I can't see your patches on my mail client,
>>>> Like what you said, I think arch_kexec_unprotect/protect_crashkres() are
>>>> generic enough, but any other better name is welcome :-)
>>>>
>>>> It also covered the newly-added kexec file path, and we can easily transfer
>>>> arch_map/unmap_reserved_pages into this new interface.
>>> I don't know the status of your patchset. If possible I think the 1st
>>> patch in your patchset shoule rename arch_map/unmap_reserved_pages to
>>> arch_kexec_protect/unprotect_crashkres, 2nd patch is to add your x86
>>> specific patch.
>> Yes, actually when I filed my patchset, I didn't notice arch_map/unmap_reserved_pages,
>> too much back then, s390 is its only user, and hard to get the purpose from its name.
>>
>> But from other point of view, they are a bit different, crash_map_reserved_pages()
>> is also called by crash_shrink_memory(), it is a bit more complex(and needs some
>> s390 arch code modification) than just simply renaming/consolidating them, so I think
>> it's  ok to provide a new generic mechanism first and then put renaming/consolidating
>> arch work back a little as a separate patch.
> OK, sounds good, I am fine with this.
>
> How do you think about Minfei's patch? You pick it up into your patchset
> in next post with his author, or just wait for him to repost? Apparently
> his patch has conflict with yours.

Essentially, Minfei's patchset has nothing to do with mine, his is a bug fix,
I think bugs have priority. Conflicts are commonplace in upstream patches,
should not be a problem. Just my two cents.

Regards,
Xunlei

>
>> Regards,
>> Xunlei
>>
>>>> I was planning doing that, but sick recently, I will try to send a patch
>>>> doing that later.
>>> Yeah, totally understand. This is not urgent, please take care of
>>> yourself.
>>>
>>>> Regards,
>>>> Xunlei
>>>>
>>>>> Xunlei. Since Andrew asked, I just checked these.
>>>>>
>>>>> I am fine with Minfei's patch 1/2. But for patch 2/2, it's a little
>>>>> comfortable to me. Is it really necessary to abstract code block from
>>>>> kexec_load, then wrap them into a newly added function do_kexec_load()?
>>>>> Without this wrapping is there a way to do your bug fix? Is there
>>>>> possibility that do_kexec_load will be called in other places? What's
>>>>> the benefit to wrap it into do_kexec_load against not wrapping?
>>>>>
>>>>> Thanks
>>>>> Baoquan
>>>>>
>>>>>> Regards,
>>>>>> Xunlei
>>>>>>
>>>>>> On 03/01/2016 at 04:02 PM, Minfei Huang wrote:
>>>>>>> v1:
>>>>>>> - Bisect the patch according to Andrew Morton's suggestion
>>>>>>>
>>>>>>> Minfei Huang (2):
>>>>>>>   kexec: Make a pair of map/unmap reserved pages in error path
>>>>>>>   kexec: Do a cleanup for function kexec_load
>>>>>>>
>>>>>>>  kernel/kexec.c | 112 ++++++++++++++++++++++++++++++++-------------------------
>>>>>>>  1 file changed, 63 insertions(+), 49 deletions(-)
>>>>>>>
>>>>>> _______________________________________________
>>>>>> kexec mailing list
>>>>>> kexec@lists.infradead.org
>>>>>> http://lists.infradead.org/mailman/listinfo/kexec
>>>> _______________________________________________
>>>> kexec mailing list
>>>> kexec@lists.infradead.org
>>>> http://lists.infradead.org/mailman/listinfo/kexec

[toc] | [prev] | [next] | [standalone]

#1364951

On 03/23/16 at 10:48am, Baoquan He wrote:
> On 03/01/16 at 05:53pm, Xunlei Pang wrote:
> > This is a bug fix.
> > 
> > After this, I will try to do a cleanup for crash_unmap/map_reserved_pages()
> > (only used by S390) to consolidate it with arch_kexec_unprotect/protect_crashkres().
> 
> Hi Xunlei, Minfei,
> 
> I think you need discuss together about how to do clean up codes in this
> place. From my point of view, arch_map/unmap_reserved_pages and
> arch_kexec_protect/unprotect_crashkres() are for the same goal but by
> different ways on different arch. So for Xunlei's patchset, you might
> need to rethink your implementation, the name of function. I personally
> think you just implement a x86 specific arch_map/unmap_reserved_pages.
> It may need a more generic name, and then add your x86 arch specific
> implementation. Sorry I can't see your patches on my mail client,
> Xunlei. Since Andrew asked, I just checked these.
> 
> I am fine with Minfei's patch 1/2. But for patch 2/2, it's a little
> comfortable to me. Is it really necessary to abstract code block from
> kexec_load, then wrap them into a newly added function do_kexec_load()?
> Without this wrapping is there a way to do your bug fix? Is there
> possibility that do_kexec_load will be called in other places? What's
> the benefit to wrap it into do_kexec_load against not wrapping?

Hi, Bao.

There is a suggestion from Vivek to wrap a new function do_kexec_load to
fix this issue, since there are a lot of logic handling in function
kexec_load. And this issue doesn't conflict with xlpang@'s patchset,
except for code confliction.

Thanks
Minfei

> 
> Thanks
> Baoquan
> 
> > 
> > Regards,
> > Xunlei
> > 
> > On 03/01/2016 at 04:02 PM, Minfei Huang wrote:
> > > v1:
> > > - Bisect the patch according to Andrew Morton's suggestion
> > >
> > > Minfei Huang (2):
> > >   kexec: Make a pair of map/unmap reserved pages in error path
> > >   kexec: Do a cleanup for function kexec_load
> > >
> > >  kernel/kexec.c | 112 ++++++++++++++++++++++++++++++++-------------------------
> > >  1 file changed, 63 insertions(+), 49 deletions(-)
> > >
> > 
> > 
> > _______________________________________________
> > kexec mailing list
> > kexec@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/kexec

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web