[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAFdhcLQ+LrVyqeBeXf++sV2RddBn2Rn6w7ZRX1szU+XW8+SPXA@mail.gmail.com>
Date: Thu, 14 Aug 2014 16:07:39 -0400
From: David Horner <ds2horner@...il.com>
To: Dan Streetman <ddstreet@...e.org>
Cc: Minchan Kim <minchan@...nel.org>,
Andrew Morton <akpm@...ux-foundation.org>,
Linux-MM <linux-mm@...ck.org>,
linux-kernel <linux-kernel@...r.kernel.org>,
Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
Jerome Marchand <jmarchan@...hat.com>, juno.choi@....com,
seungho1.park@....com, Luigi Semenzato <semenzato@...gle.com>,
Nitin Gupta <ngupta@...are.org>,
Seth Jennings <sjennings@...iantweb.net>
Subject: Re: [PATCH 3/3] zram: add mem_used_max via sysfs
On Thu, Aug 14, 2014 at 3:11 PM, Dan Streetman <ddstreet@...e.org> wrote:
> On Thu, Aug 14, 2014 at 12:23 PM, David Horner <ds2horner@...il.com> wrote:
>> On Thu, Aug 14, 2014 at 11:32 AM, David Horner <ds2horner@...il.com> wrote:
>>> On Thu, Aug 14, 2014 at 11:09 AM, Dan Streetman <ddstreet@...e.org> wrote:
>>>> On Wed, Aug 13, 2014 at 9:12 PM, Minchan Kim <minchan@...nel.org> wrote:
>>>>> - if (zram->limit_bytes &&
>>>>> - zs_get_total_size_bytes(meta->mem_pool) > zram->limit_bytes) {
>>>>> + total_bytes = zs_get_total_size_bytes(meta->mem_pool);
>>>>> + if (zram->limit_bytes && total_bytes > zram->limit_bytes) {
>>>>
>>>> do you need to take the init_lock to read limit_bytes here? It could
>>>> be getting changed between these checks...
>>>
>>> There is no real danger in freeing with an error.
>>> It is more timing than a race.
>> I probably should explain my reasoning.
>>
>> any changes between getting the total value and the limit test are not
>> problematic (From race perspective).
>>
>> 1) If the actual total increases and the value returned under rates it, then
>> a) if this.total exceeds the limit - no problem it is rolled back as
>> it would if the actual total were used.
>> b) if this.total <= limit OK - as other process will be dinged (it
>> will see its own allocation)
>>
>> 2) If the actual total decreases and the value returned overrates
>> rates it, then
>> a) if this.value <= limit then allocation great (actual has even more room)
>> b) if this.value > max it will be rolled back (as the other might be
>> as well) and process can compete again.
>
for completeness I should have mentioned the normal decrease case of
deallocation
and not roll back.
(and of course it is also not a problem and does not race).
Are these typical situations in documentation folder
(I know the related memory barriers is)
It would be so much better to say scenario 23 is a potential problem
rather than rewriting the essays.
> actually I wasn't thinking of total_bytes changing, i think it's ok to
> check the total at that specific point in time, for the reasons you
> point out above.
>
> I was thinking about zram->limit_bytes changing, especially if it's
> possible to disable the limit (i.e. set it to 0), e.g.:
>
> assume currently total_bytes == 1G and limit_bytes == 2G, so there is
> not currently any danger of going over the limit. Then:
>
>
> thread 1 : if (zram->limit_bytes ...this is true
>
> thread 2 : zram->limit_bytes = limit; ...where limit == 0
>
> thread 1 : && total_bytes > zram->limit_bytes) { ...this is now also true
>
> thread 1 : incorrectly return -ENOMEM failure
>
> It's very unlikely, and a single failure isn't a big deal here since
> the caller must be prepared to handle a failure. And of course the
> compiler might reorder those checks. And if it's not possible to
> disable the limit by setting it to 0 (besides a complete reset of the
> zram device, which wouldn't happen while this function is running),
> then there's not an issue here (although, I think being able to
> disable the limit without having to reset the zram device is useful).
agreed on 7 of 8 assertions
(not yet sure about reset not happening while function running).
That issue then arises in [PATCH 2/2] zram: limit memory size for zram
and as you mention reordering the zero check after the limit comparison
in the if statement could be reordered by the compiler
As I see it this is also a timing issue - as you explained, and not a race.
Perhaps we name it scenario 24?
And especially I agree with allowing zero limit reset without device reset.
The equivalent is currently possible (for all practical purposes)
anyway by setting
the limit to max_u64.
So allowing zero is cleaner.
>
>
> Also for setting the max_used_bytes, isn't non-atomically setting a
> u64 value dangerous (on <64 bit systems) when it's not synchronized
> between threads?
perhaps it needs an atomic function - I will think some more on it.
>
> That is, unless the entire zram_bvec_write() function or this section
> is already thread-safe, and i missed it (which i may have :-)
nor have I.checked.(on the to do).
>
>
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists