Re: [PATCH 2/2] [RFC] fadvise: Add _VOLATILE,_ISVOLATILE, and _NONVOLATILE flags

John Stultz
2012-03-20T00:19:41Z

On 03/17/2012 09:21 AM, Dmitry Adamushko wrote:
> Hi John,
>
> [ ... ]
>
>> +/*
>> + * Mark a region as volatile, allowing dirty pages to be purged
>> + * under memory pressure
>> + */
>> +long mapping_range_volatile(struct address_space *mapping,
>> + pgoff_t start_index, pgoff_t end_index)
>> +{
>> + struct volatile_range *new;
>> + struct range_tree_node *node;
>> +
>> + u64 start, end;
>> + int purged = 0;
>> + start = (u64)start_index;
>> + end = (u64)end_index;
>> +
>> + new = vrange_alloc();
>> + if (!new)
>> + return -ENOMEM;
>> +
>> + mutex_lock(&volatile_mutex);
>> +
>> + node = range_tree_in_range_adjacent(&mapping->volatile_root,
>> + start, end);
>> + while (node) {
>> + struct volatile_range *vrange;
>> +
>> + /* Already entirely marked volatile, so we're done */
>> + if (node->start< start&& node->end> end) {
>> + /* don't need the allocated value */
>> + kfree(new);
>> + goto out;
>> + }
>> +
>> + /* Grab containing volatile range */
>> + vrange = container_of(node, struct volatile_range, range_node);
>> +
>> + /* resize range */
>> + start = min_t(u64, start, node->start);
>> + end = max_t(u64, end, node->end);
>> + purged |= vrange->purged;
>> +
>> +
>> + vrange_del(vrange);
>> +
>> + /* get the next possible overlap */
>> + node = range_tree_in_range(&mapping->volatile_root, start, end);
> I guess range_tree_in_range_adjacent() should be used here again.
> There can be 2 adjacent regions (left and right), and we'll miss one
> of them with range_tree_in_range().
Good catch, thank you!

> Also (as I had already mentioned before), I think that new ranges must
> not be merged with the existing "vrange->purged == 1" ranges.
> Otherwise, for some use cases, the whole idea of 'volatility' gets
> broken. For example, when an application is processing a big buffer in
> small consequent chunks (marking a chunk as volatile when done with
> it), and the range gets 'purged' by the kernel early in this process
> (when it's still small).
>
I agree that this seems like a much more intelligent way coalesce
regions. I hadn't yet implemented it, as I was hoping for some comment
from the Android folks if there was a specific use for the design they
selected for ashmem, but I suspect there isn't.

I'll go ahead and integrate this for the next revision.

Thanks again for the feedback!
-john