On 24.6.2010, at 12.09, Alan Cox wrote:
>> size_t memrar_allocator_largest_free_area(struct memrar_allocator *allocator)
>> {
>> - if (allocator == NULL)
>> - return 0;
>> - return allocator->largest_free_area;
>> + size_t tmp = 0;
>> +
>> + if (allocator != NULL) {
>> + mutex_lock(&allocator->lock);
>> + tmp = allocator->largest_free_area;
>> + mutex_unlock(&allocator->lock);
>
> This doesn't seem to make any sense (in either version). The moment you
> drop the lock the value in "tmp" becomes stale as the allocator could
> change it. ?
>
The idea was proposed by Ossama Othman in his earlier reply.
Begin forwarded message:
> From: "Othman, Ossama" <[email protected]>
> To: Henri Häkkinen <[email protected]>, "[email protected]" <[email protected]>,
> "[email protected]" <[email protected]>, "[email protected]"
> <[email protected]>
> Cc: "[email protected]" <[email protected]>,
> "[email protected]" <[email protected]>
> Subject: RE: [PATCH] Staging: memrar: Moved memrar_allocator struct into
> memrar_allocator.c
>
> Hi,
>
>> Forward declared memrar_allocator in memrar_allocator.h and moved it
>> to memrar_allocator.c file. Implemented memrar_allocator_capacity(),
>> memrar_allocator_largest_free_area(), memrar_allocoator_lock() and
>> memrar_allocator_unlock().
> ...
>> - mutex_lock(&allocator->lock);
>> - r->largest_block_size = allocator->largest_free_area;
>> - mutex_unlock(&allocator->lock);
>> + memrar_allocator_lock(allocator);
>> + r->largest_block_size =
>> memrar_allocator_largest_free_area(allocator);
>> + memrar_allocator_unlock(allocator);
>
> I don't think it's necessary to expose the allocator lock. Why not just grab
> the lock in memrar_allocator_largest_free_area() while the underlying struct
> field is being accessed and then unlock it before that function returns?
> That would allow the allocator lock to remain an internal implementation
> detail. We only need to ensure access to the struct field itself is
> synchronized, e.g.:
>
> size_t memrar_allocator_largest_free_area(struct memrar_allocator *allocator)
> {
> size_t tmp = 0;
>
> if (allocator != NULL) {
> mutex_lock(&allocator->lock);
> tmp = allocator->largest_free_area;
> mutex_unlock(&allocator->lock);
> }
>
> return tmp;
> }
>
> Certainly the allocator->largest_free_area value could be updated after the
> lock is released and by the time it is returned to the user (for statistical
> purposes), but at least the internal allocator state would remain consistent
> in the presences of multiple threads.
>
> HTH,
> -Ossama
>
_______________________________________________
devel mailing list
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
