On 25/02/2016 15:34, Parav Pandit wrote:
> On Thu, Feb 25, 2016 at 5:33 PM, Haggai Eran <haggaie@xxxxxxxxxxxx> wrote:
>>>>> +retry:
>>>>> + spin_lock(&cg->rpool_list_lock);
>>>>> + rpool = find_cg_rpool_locked(cg, device);
>>>>> + if (!rpool) {
>>>>> + spin_unlock(&cg->rpool_list_lock);
>>>>> + ret = alloc_cg_rpool(cg, device);
>>>>> + if (ret)
>>>>> + goto err;
>>>>> + else
>>>>> + goto retry;
>>>> Instead of retrying after allocation of a new rpool, why not just return the
>>>> newly allocated rpool (or the existing one) from alloc_cg_rpool?
>>>
>>> It can be done, but locking semantics just becomes difficult to
>>> review/maintain with that where alloc_cg_rpool will unlock and lock
>>> conditionally later on.
>> Maybe I'm missing something, but couldn't you simply lock rpool_list_lock
>> inside alloc_cg_rpool()? It already does that around its call to
>> find_cg_rpool_locked() and the insertion to cg_list.
>
> No. ref_count and usage counters are updated at level where lock is
> taken in charge_cg_resource().
> If I move locking rpool_list_lock inside alloc_cg_rpool, unlocking
> will continue outside, alloc_cg_rpool() when its found or allocated.
> As you acknowledged in below comment that this makes confusing to
> lock/unlock from different context, I think current implementation
> achieves both.
> (a) take lock from single context
> (b) keep functionality of find and alloc in two separate individual functions
Okay, fair enough.
>> I thought that was about functions that only locked the lock, called the
>> find function, and released the lock. What I'm suggesting is to have one
>> function that does "lock + find + allocate if needed + unlock",
>
> I had similar function in past which does,
> "lock + find + allocate if needed + + inc_ref_cnt + unlock", (get_cg_rpool)
> update usage_counter atomically, because other thread/process might update too.
> check atomic_dec_cnt - on reaching zero, "lock + del_entry + unlock + free".
>
> Tejun asked to simplify this to,
>
> "lock + find + allocate if needed + inc_ref_cnt_without_atomic" + unlock".
> which I did in this patch v6.
Okay.
>> and another
>> function that does (under caller's lock) "check ref count + check max count +
>> release rpool".
> This can be done. Have one dumb basic question for thiat.
> Can we call kfree() with spin_lock held? All these years I tend to
> avoid doing so.
>
I think so. This is an old link but I think it still applies:
https://lkml.org/lkml/2004/11/21/130
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
