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

Groups > linux.kernel > #1211557 > unrolled thread

Re: [PATCH 0/3] mm/vmalloc: Cache the /proc/meminfo vmalloc statistics

Back to article view | Back to linux.kernel

Contents

  Re: [PATCH 0/3] mm/vmalloc: Cache the /proc/meminfo vmalloc statistics "George Spelvin" <linux@horizon.com> - 2015-08-23 06:50 +0200
    Re: [PATCH 0/3] mm/vmalloc: Cache the /proc/meminfo vmalloc  statistics Ingo Molnar <mingo@kernel.org> - 2015-08-23 08:10 +0200
      Re: [PATCH 0/3] mm/vmalloc: Cache the /proc/meminfo vmalloc statistics "George Spelvin" <linux@horizon.com> - 2015-08-23 08:50 +0200
        [PATCH 3/3 v3] mm/vmalloc: Cache the vmalloc memory info Ingo Molnar <mingo@kernel.org> - 2015-08-23 10:20 +0200
          Re: [PATCH 3/3 v3] mm/vmalloc: Cache the vmalloc memory info Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-08-23 23:00 +0200
            Re: [PATCH 3/3 v3] mm/vmalloc: Cache the vmalloc memory info Ingo Molnar <mingo@kernel.org> - 2015-08-24 09:00 +0200
              Re: [PATCH 3/3 v3] mm/vmalloc: Cache the vmalloc memory info Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-08-24 10:40 +0200
          Re: [PATCH 3/3 v3] mm/vmalloc: Cache the vmalloc memory info Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-08-24 00:00 +0200
            Re: [PATCH 3/3 v3] mm/vmalloc: Cache the vmalloc memory info Ingo Molnar <mingo@kernel.org> - 2015-08-24 09:10 +0200
            Re: [PATCH 3/3 v3] mm/vmalloc: Cache the vmalloc memory info Linus Torvalds <torvalds@linux-foundation.org> - 2015-08-25 18:40 +0200
              Re: [PATCH 3/3 v3] mm/vmalloc: Cache the vmalloc memory info Linus Torvalds <torvalds@linux-foundation.org> - 2015-08-25 19:10 +0200
          Re: [PATCH 3/3 v3] mm/vmalloc: Cache the vmalloc memory info "George Spelvin" <linux@horizon.com> - 2015-08-24 03:10 +0200
            [PATCH 3/3 v4] mm/vmalloc: Cache the vmalloc memory info Ingo Molnar <mingo@kernel.org> - 2015-08-24 09:40 +0200
              Re: [PATCH 3/3 v4] mm/vmalloc: Cache the vmalloc memory info Ingo Molnar <mingo@kernel.org> - 2015-08-24 09:50 +0200
                [PATCH 3/3 v5] mm/vmalloc: Cache the vmalloc memory info Ingo Molnar <mingo@kernel.org> - 2015-08-24 10:00 +0200
                  Re: [PATCH 3/3 v5] mm/vmalloc: Cache the vmalloc memory info "George Spelvin" <linux@horizon.com> - 2015-08-24 15:00 +0200
                    [PATCH 3/3 v6] mm/vmalloc: Cache the vmalloc memory info Ingo Molnar <mingo@kernel.org> - 2015-08-25 12:00 +0200
                      Re: [PATCH 3/3 v6] mm/vmalloc: Cache the vmalloc memory info "George Spelvin" <linux@horizon.com> - 2015-08-25 12:40 +0200
                      Re: [PATCH 3/3 v6] mm/vmalloc: Cache the vmalloc memory info Peter Zijlstra <peterz@infradead.org> - 2015-08-25 15:10 +0200
                      Re: [PATCH 3/3 v6] mm/vmalloc: Cache the vmalloc memory info Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-08-25 16:30 +0200
                        Re: [PATCH 3/3 v6] mm/vmalloc: Cache the vmalloc memory info "George Spelvin" <linux@horizon.com> - 2015-08-25 17:20 +0200
              Re: [PATCH 3/3 v4] mm/vmalloc: Cache the vmalloc memory info "John Stoffel" <john@stoffel.org> - 2015-08-24 15:50 +0200
                Re: [PATCH 3/3 v4] mm/vmalloc: Cache the vmalloc memory info "George Spelvin" <linux@horizon.com> - 2015-08-24 17:20 +0200
                  Re: [PATCH 3/3 v4] mm/vmalloc: Cache the vmalloc memory info "John Stoffel" <john@stoffel.org> - 2015-08-24 18:00 +0200
          Re: [PATCH 3/3 v3] mm/vmalloc: Cache the vmalloc memory info Peter Zijlstra <peterz@infradead.org> - 2015-08-25 14:50 +0200

Page 2 of 2 — ← Prev page 1 [2]

#1213104 — Re: [PATCH 3/3 v6] mm/vmalloc: Cache the vmalloc memory info

>>> (I hope I'm not annoying you by bikeshedding this too much, although I
>>> think this is improving.)
>>
>> [ I don't mind, although I wish other, more critical parts of the kernel got this
>>   much attention as well ;-) ]

That's the problem with small, understandable problems: people *aren't*
scared to mess with them.

> It's been fun seeing this evolve, but overall, I tend to agree with
> Peter: It's a lot of complexity for little gain. If we're not going to
> just kill the Vmalloc* fields (which is probably too controversial)
> I'd prefer Linus' simpler version.

Are you sure you're not being affected by the number of iterations?

The final version is not actually a lot of code (although yes, more than
Linus's), and offers the advantage of peace of mind: there's not some
nasty-smelling code you can't entirely trust left behind.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1212205 — Re: [PATCH 3/3 v4] mm/vmalloc: Cache the vmalloc memory info

>>>>> "Ingo" == Ingo Molnar <mingo@kernel.org> writes:

Ingo> * George Spelvin <linux@horizon.com> wrote:

>> First, an actual, albeit minor, bug: initializing both vmap_info_gen
>> and vmap_info_cache_gen to 0 marks the cache as valid, which it's not.

Ingo> Ha! :-) Fixed.

>> vmap_info_gen should be initialized to 1 to force an initial
>> cache update.

Blech, it should be initialized with a proper #define
VMAP_CACHE_NEEDS_UPDATE 1, instead of more magic numbers.


Ingo> + */
Ingo> +static DEFINE_SPINLOCK(vmap_info_lock);
Ingo> +static int vmap_info_gen = 1;

   static int vmap_info_gen = VMAP_CACHE_NEEDS_UPDATE;

Ingo> +static int vmap_info_cache_gen;
Ingo> +static struct vmalloc_info vmap_info_cache;
Ingo> +#endif


This will help keep bugs like this out in the future... I hope!
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1212307 — Re: [PATCH 3/3 v4] mm/vmalloc: Cache the vmalloc memory info

John Stoffel <john@stoffel.org> wrote:
>> vmap_info_gen should be initialized to 1 to force an initial
>> cache update.

> Blech, it should be initialized with a proper #define
> VMAP_CACHE_NEEDS_UPDATE 1, instead of more magic numbers.

Er... this is a joke, right?

First, this number is used exactly once, and it's not part of a collection
of similar numbers.  And the definition would be adjacent to the use.

We have easier ways of accomplishing that, called "comments".


Second, your proposed name is misleading.  "needs update" is defined
as vmap_info_gen != vmap_info_cache_gen.  There is no particular value
of either that has this meaning.

For example, initializing vmap_info_cache_gen to -1 would do just as well.
(I actually considered that before deciding that +1 was "simpler" than -1.)

For some versions of the code, an *arbitrary* difference is okay.
You could set one ot 0xDEADBEEF and the other to 0xFEEDFACE.

For other versions, the magnitude matters, but not *too* much.
Initializing it to 42 would be perfectly correct, but waste time doing
42 cache updates before settling down.

Singling out the value 1 as VMAP_CACHE_NEEDS_UPDATE is actively misleading.


> This will help keep bugs like this out in the future... I hope!

And this is the punchline, right?

The problem was not realizing that non-default initialization was required;
what we *call* the non-default value is irrelevant.

I doubt it would ever have been a real (i.e. noticeable) bug, actually;
the first bit of vmap activity in very early boot would have invalidated
the cache.


(John, my apologies if I went over the top and am contributing to LKML's
reputation for flaming.  I *did* actually laugh, and *do* think it's a
dumb idea, but my annoyance is really directed at unpleasant memories of
mindless application of coding style guidelines.  In this case, I suspect
you just posted before reading carefully enough to see the subtle logic.)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1212324 — Re: [PATCH 3/3 v4] mm/vmalloc: Cache the vmalloc memory info

George> John Stoffel <john@stoffel.org> wrote:
>>> vmap_info_gen should be initialized to 1 to force an initial
>>> cache update.

>> Blech, it should be initialized with a proper #define
>> VMAP_CACHE_NEEDS_UPDATE 1, instead of more magic numbers.

George> Er... this is a joke, right?

Not really.  The comment made before was that by setting this variable
to zero, it wasn't properly initialized.  Which implies that either
the API is wrong... or we should be documenting it better.   I just
went in the direction of the #define instead of a comment. 

George> First, this number is used exactly once, and it's not part of
George> a collection of similar numbers.  And the definition would be
George> adjacent to the use.

George> We have easier ways of accomplishing that, called "comments".

Sure, that would be the better solution in this case.  

George> Second, your proposed name is misleading.  "needs update" is defined
George> as vmap_info_gen != vmap_info_cache_gen.  There is no particular value
George> of either that has this meaning.

George> For example, initializing vmap_info_cache_gen to -1 would do just as well.
George> (I actually considered that before deciding that +1 was "simpler" than -1.)

See, I just threw out a dumb suggestion without reading the patch
properly.  My fault.

George> (John, my apologies if I went over the top and am contributing to LKML's
George> reputation for flaming.  I *did* actually laugh, and *do* think it's a
George> dumb idea, but my annoyance is really directed at unpleasant memories of
George> mindless application of coding style guidelines.  In this case, I suspect
George> you just posted before reading carefully enough to see the subtle logic.)

Nope, I'm in the wrong here.  And your comment here is wonderful, I
really do appreciate how you handled my ham fisted attempt to
contribute.  But I've got thick skin and I'll keep trying in my free
time to comment on patches when I can.

John
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1213022 — Re: [PATCH 3/3 v3] mm/vmalloc: Cache the vmalloc memory info

On Sun, Aug 23, 2015 at 10:17:51AM +0200, Ingo Molnar wrote:
> +static u64 vmap_info_gen;
> +static u64 vmap_info_cache_gen;

> +void get_vmalloc_info(struct vmalloc_info *vmi)
> +{
> +	u64 gen = READ_ONCE(vmap_info_gen);
> +
> +	/*
> +	 * If the generation counter of the cache matches that of
> +	 * the vmalloc generation counter then return the cache:
> +	 */
> +	if (READ_ONCE(vmap_info_cache_gen) == gen) {

Why are those things u64? It has the obvious down-side that you still
get split loads on 32bit machines.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web