Re: [PATCH 3/3] zram: add mem_used_max via sysfs

[David Horner]

On Thu, Aug 14, 2014 at 3:11 PM, Dan Streetman <ddstreet@xxxxxxxx> wrote:
> On Thu, Aug 14, 2014 at 12:23 PM, David Horner <ds2horner@xxxxxxxxx> wrote:
>> On Thu, Aug 14, 2014 at 11:32 AM, David Horner <ds2horner@xxxxxxxxx> wrote:
>>> On Thu, Aug 14, 2014 at 11:09 AM, Dan Streetman <ddstreet@xxxxxxxx> wrote:
>>>> On Wed, Aug 13, 2014 at 9:12 PM, Minchan Kim <minchan@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/