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

Groups > linux.kernel > #1740812 > unrolled thread

Re: [PATCH] mm: kill kmemcheck again

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] mm: kill kmemcheck again Michal Hocko <mhocko@kernel.org> - 2017-09-27 17:10 +0200
    Re: [PATCH] mm: kill kmemcheck again "Levin, Alexander (Sasha Levin)" <alexander.levin@verizon.com> - 2017-09-27 17:30 +0200
      Re: [PATCH] mm: kill kmemcheck again ebiederm@xmission.com (Eric W. Biederman) - 2017-09-27 19:40 +0200
        Re: [PATCH] mm: kill kmemcheck again "Levin, Alexander (Sasha Levin)" <alexander.levin@verizon.com> - 2017-09-28 00:10 +0200
    Re: [PATCH] mm: kill kmemcheck again Steven Rostedt <rostedt@goodmis.org> - 2017-09-30 11:50 +0200
      Re: [PATCH] mm: kill kmemcheck again "Levin, Alexander (Sasha Levin)" <alexander.levin@verizon.com> - 2017-09-30 22:10 +0200
        Re: [PATCH] mm: kill kmemcheck again Michal Hocko <mhocko@kernel.org> - 2017-10-02 09:30 +0200

#1740812 — Re: [PATCH] mm: kill kmemcheck again

[Multipart message — attachments visible in raw view] — view raw

On Wed 27-09-17 11:27:40, Sasha Levin wrote:
> 2 Years ago I proposed to kill kmemcheck:
> 
> > As discussed on LSF/MM, kill kmemcheck.
> >
> > KASan is a replacement that is able to work without the limitation of
> > kmemcheck (single CPU, slow). KASan is already upstream.
> >
> > We are also not aware of any users of kmemcheck (or users who don't consider
> > KASan as a suitable replacement).
> 
> The only objection was that since KASAN wasn't supported by all GCC
> versions provided by distros at that time we should hold off for 2
> years, and try again.
> 
> Now that 2 years have passed, and all distros provide gcc that supports
> KASAN, kill kmemcheck again for the very same reasons.

This is just too large to review manually. How have you generated the
patch?

My compile test batery failed for i386 allyesconfig for some reason
which is not entirely clear to me (see attached).  I have applied on top
of dc972a67cc54585bd83ad811c4e9b6ab3dcd427e and that one compiles fine.

> Cc: Steven Rostedt (VMware) <rostedt@goodmis.org>
> Cc: David S. Miller <davem@davemloft.net>
> Signed-off-by: Sasha Levin <alexander.levin@verizon.com>

Anyway I fully support this removal. It is a lot of rarely used code and
KASAN is much more usable.
-- 
Michal Hocko
SUSE Labs

[toc] | [next] | [standalone]

#1740833

On Wed, Sep 27, 2017 at 05:02:07PM +0200, Michal Hocko wrote:
>This is just too large to review manually. How have you generated the
>patch?

Manualy. Note that most of it (~95%) is the result of 'rm arch/x86/mm/kmemcheck'.

Otherwise, I just removed all uses of __GFP_NOWARN/SLAB_NOWARN, and calls to
various annotations throughout the code.

I'm not sure about i386 breakage, will take a look, doesn't seem to be too obvious.

-- 

Thanks,
Sasha

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

#1740915

"Levin, Alexander (Sasha Levin)" <alexander.levin@verizon.com> writes:

> On Wed, Sep 27, 2017 at 05:02:07PM +0200, Michal Hocko wrote:
>>This is just too large to review manually. How have you generated the
>>patch?
>
> Manualy. Note that most of it (~95%) is the result of 'rm arch/x86/mm/kmemcheck'.
>
> Otherwise, I just removed all uses of __GFP_NOWARN/SLAB_NOWARN, and calls to
> various annotations throughout the code.

Do you mean GFP_NOTRACK? GFP_NOWARN has a different meaning.

> I'm not sure about i386 breakage, will take a look, doesn't seem to be too obvious.

Eric

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

#1741062

On Wed, Sep 27, 2017 at 12:36:27PM -0500, Eric W. Biederman wrote:
>"Levin, Alexander (Sasha Levin)" <alexander.levin@verizon.com> writes:
>
>> On Wed, Sep 27, 2017 at 05:02:07PM +0200, Michal Hocko wrote:
>>>This is just too large to review manually. How have you generated the
>>>patch?
>>
>> Manualy. Note that most of it (~95%) is the result of 'rm arch/x86/mm/kmemcheck'.
>>
>> Otherwise, I just removed all uses of __GFP_NOWARN/SLAB_NOWARN, and calls to
>> various annotations throughout the code.
>
>Do you mean GFP_NOTRACK? GFP_NOWARN has a different meaning.

uh, yes, thanks Eric!

__GFP_NOTRACK and SLAB_NOTRACK.

-- 

Thanks,
Sasha

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

#1742580

On Wed, 27 Sep 2017 17:02:07 +0200
Michal Hocko <mhocko@kernel.org> wrote:

> > Now that 2 years have passed, and all distros provide gcc that supports
> > KASAN, kill kmemcheck again for the very same reasons.  
> 
> This is just too large to review manually. How have you generated the
> patch?

I agree. This needs to be taken out piece by piece, not in one go,
where there could be unexpected fallout.

> 
> My compile test batery failed for i386 allyesconfig for some reason
> which is not entirely clear to me (see attached).  I have applied on top
> of dc972a67cc54585bd83ad811c4e9b6ab3dcd427e and that one compiles fine.
> 
> > Cc: Steven Rostedt (VMware) <rostedt@goodmis.org>
> > Cc: David S. Miller <davem@davemloft.net>
> > Signed-off-by: Sasha Levin <alexander.levin@verizon.com>  
> 
> Anyway I fully support this removal. It is a lot of rarely used code and
> KASAN is much more usable.

Now that my default compilers support KASAN, I'm fine with this removal.

-- Steve

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

#1742687

On Sat, Sep 30, 2017 at 03:57:27PM +0200, Vegard Nossum wrote:
>On 30 September 2017 at 11:48, Steven Rostedt <rostedt@goodmis.org> wrote:
>> On Wed, 27 Sep 2017 17:02:07 +0200
>> Michal Hocko <mhocko@kernel.org> wrote:
>>
>>> > Now that 2 years have passed, and all distros provide gcc that supports
>>> > KASAN, kill kmemcheck again for the very same reasons.
>>>
>>> This is just too large to review manually. How have you generated the
>>> patch?
>>
>> I agree. This needs to be taken out piece by piece, not in one go,
>> where there could be unexpected fallout.
>
>I have a patch from earlier this year that starts by removing the core
>code and defining all the helpers/flags as no-ops so they can be
>removed bit by bit at a later time. See the attachment. Pekka signed
>off on it too.
>e
>I never actually submitted this because I was waiting for MSAN to be

I'm not sure how much value there is in doing it this way. I agree that the patch is big, but most of it is simply removing code under arch/x86/mm/kmemcheck.

The difference between Vegard's patch and mine is about 300 lines (out of 2800+), where those 300 lines are simply removing calls to kmemcheck. There are no logic changes. (so something very similar to 's/*kmemcheck*//g' would do the trick).

--

Thanks,
Sasha

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

#1742989

On Sat 30-09-17 20:02:41, Sasha Levin wrote:
> On Sat, Sep 30, 2017 at 03:57:27PM +0200, Vegard Nossum wrote:
> >On 30 September 2017 at 11:48, Steven Rostedt <rostedt@goodmis.org> wrote:
> >> On Wed, 27 Sep 2017 17:02:07 +0200
> >> Michal Hocko <mhocko@kernel.org> wrote:
> >>
> >>> > Now that 2 years have passed, and all distros provide gcc that supports
> >>> > KASAN, kill kmemcheck again for the very same reasons.
> >>>
> >>> This is just too large to review manually. How have you generated the
> >>> patch?
> >>
> >> I agree. This needs to be taken out piece by piece, not in one go,
> >> where there could be unexpected fallout.
> >
> >I have a patch from earlier this year that starts by removing the core
> >code and defining all the helpers/flags as no-ops so they can be
> >removed bit by bit at a later time. See the attachment. Pekka signed
> >off on it too.
> >e
> >I never actually submitted this because I was waiting for MSAN to be
> 
> I'm not sure how much value there is in doing it this way. I agree
> that the patch is big, but most of it is simply removing code under
> arch/x86/mm/kmemcheck.
>
> The difference between Vegard's patch and mine is about 300 lines
> (out of 2800+), where those 300 lines are simply removing calls to
> kmemcheck. There are no logic changes. (so something very similar to
> 's/*kmemcheck*//g' would do the trick).

Maybe splitting the patch into three: 1) remove all callers of kmemleak
API and 2) remove arch/x86/mm/kmemcheck/ and 3) remove leftovers would
be slightly easier to review. Maybe 2 and 3 would have some dependencies
so they would have to end up in the same path.

Just my 2c

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web