Re: CVS commit: src/sys/kern

[Maxime Villard <max%M00nBSD.net@localhost>]

Le 22/07/2014 13:49, Greg Troxel a écrit :
> 
> "Maxime Villard" <maxv%netbsd.org@localhost> writes:
> 
>> Module Name: src
>> Committed By:        maxv
>> Date:                Tue Jul 22 07:38:41 UTC 2014
>>
>> Modified Files:
>>      src/sys/kern: subr_kmem.c
>>
>> Log Message:
>> Enable KMEM_REDZONE on DIAGNOSTIC. It will try to catch overflows.
>>
>> No comment on tech-kern@
> 
> I didn't see this on tech-kern
> 

But I did send it:

  http://mail-index.netbsd.org/tech-kern/2014/07/18/msg017369.html

> (nor did I see anything about defining
> KMEM_POISON), 

Who talks about KMEM_POISON? I did *not* enable KMEM_POISON.

> and now that I'm aware I object.

I'm sorry, but you seem to be completely misunderstanding the changes I made.

> DIAGNOSTIC, by longstanding tradition, is a lightweight and not have a
> significant performance hit.  Basically it's about KASSERT of things
> that must be true.  This is changing the size of memory allocations,
> which is far more far-reaching.

Ah. Well, you know, I secretly enabled KMEM_SIZE one month ago. KMEM_SIZE
*does always allocate one more memory chunk*. For one month -current has
been allocating slightly more memory, and I haven't seen anyone complaining.
Because there's no reason for complaining.

The *immediate* result was this:

   http://gnats.netbsd.org/cgi-bin/query-pr-single.pl?number=48963

This simple PR justifies by itself my choice about KMEM_SIZE. And KMEM_SIZE
is exactly the feature that would have detected the huge mess in mount() I
fixed some months ago. And it was a *security issue*, that affected releases.

> I am not claiming that KMEM_REDZONE is not useful.   Arguably it could
> be enabled under DEBUG, which is documented to be expensive (and
> unreasonable to use on a normal basis).

It *was already enabled under DEBUG*. Yes, the KMEM_XX stuff were
documented to be expensive, but as I stated in my mail on tech-kern@
- that nobody has read apparently - it is lighter now, and more
efficient.

KMEM_REDZONE *may* allocate one more memory chunk (8 bytes), depending
on the space left in the previous chunk.

It's all explained in my recent changes in /kern/subr_kmem.c.

> DIAGNOSTIC, on the other hand,
> I consider normal for systems that are being used (rather than only
> debug targets).

I totally disagree. The main idea behind enabling KMEM_REDZONE is to
catch bugs *before they reach the releases*. I think it's more important
to have some *basic features* that perform *basic checks* on -current
than taking the risk of releasing horse shit.

Yes, we should not enable big things that considerably slows down the
system, but as I said, me and lars@ have reduced the performance impact,
so that it is light enough to be enabled on DIAGNOSTIC.

The deal is simple: either we are serious, and we want to provide a working
system, and we enable some basic features on -current so that it gets
tested on many architectures/hardware, or we are not serious and we keep
all these good stuff disabled only because we want the in-development
version to be almost as fast as the release.

The small/nonexistent performance impact introduced by KMEM_REDZONE is
largely compensated by this performance improvement:

  http://mail-index.netbsd.org/source-changes/2014/07/22/msg056602.html

and this one *will be in the releases*. I didn't commit both at the same
time for nothing.

> The same goes for KMEM_POISON; these checks, while useful for debugging
> (and it would be nice to have regular anita runs with DEBUG), are too
> expensive for ordinary use.

That has nothing to do here, we are not talking about KMEM_POISON.

> For -current, GENERIC defines DIAGNOSTIC.

Yes I know, that's exactly for this reason that I did enable KMEM_REDZONE
on DIAGNOSTIC.

> Please revert the automatic definition of these or change to DEBUG.
> (I am not objecting to the separating and spiffing up of these features,
> just that they are enabled when DIAGNOSTIC is defined.)
> 

As I said it was already in DEBUG.

It's great to have bug-mitigation features. But if they are never enabled,
bugs and security issues appear.

So no, what you say does not convince me at all.

Regards,
Maxime