[Devel] [RFC PATCH vz9 v6 06/62] dm-ploop: reduce the time lock is hold, taking it only to protect data

[Alexander Atanasov]

On 16.01.25 11:31, Pavel Tikhomirov wrote:
> 
> 
> On 1/14/25 20:05, Alexander Atanasov wrote:
>> On 14.01.25 4:27, Pavel Tikhomirov wrote:
>>>
>>>
>>> On 1/14/25 00:27, Alexander Atanasov wrote:
>>>> On 9.01.25 5:52, Pavel Tikhomirov wrote:
>>>>>
>>>>>
>>>>> On 12/6/24 05:55, Alexander Atanasov wrote:
>>>>>> Currently lock is taken once for all pios submitted(code block),
>>>>>> move it to protect only the list insertion (data).
>>>>>> The goal is to reduce lock contention on the deferred lock as
>>>>>> it is in the top of lock stats.
>>>>>>
>>>>>> https://virtuozzo.atlassian.net/browse/VSTOR-91820
>>>>>> Signed-off-by: Alexander Atanasov <alexander.atanasov at virtuozzo.com>
>>>>>> ---
>>>>>>   drivers/md/dm-ploop-map.c | 8 ++++----
>>>>>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/md/dm-ploop-map.c b/drivers/md/dm-ploop-map.c
>>>>>> index 4b9e000e0f98..a0fe92a592a1 100644
>>>>>> --- a/drivers/md/dm-ploop-map.c
>>>>>> +++ b/drivers/md/dm-ploop-map.c
>>>>>> @@ -340,8 +340,9 @@ static void ploop_dispatch_pio(struct ploop 
>>>>>> *ploop, struct pio *pio,
>>>>>>                      bool *is_data, bool *is_flush)
>>>>>>   {
>>>>>>       struct list_head *list = &ploop->pios[pio->queue_list_id];
>>>>>> +    unsigned long flags;
>>>>>> -    lockdep_assert_held(&ploop->deferred_lock);
>>>>>> +    lockdep_assert_not_held(&ploop->deferred_lock);
>>>>>>       WARN_ON_ONCE(pio->queue_list_id >= PLOOP_LIST_COUNT);
>>>>>>       if (pio->queue_list_id == PLOOP_LIST_FLUSH)
>>>>>> @@ -349,23 +350,22 @@ static void ploop_dispatch_pio(struct ploop 
>>>>>> *ploop, struct pio *pio,
>>>>>>       else
>>>>>>           *is_data = true;
>>>>>> +    spin_lock_irqsave(&ploop->deferred_lock, flags);
>>>>>>       list_add_tail(&pio->list, list);
>>>>>> +    spin_unlock_irqrestore(&ploop->deferred_lock, flags);
>>>>>
>>>>> This patch can be merged to "[RFC PATCH vz9 v6 10/62] dm-ploop: 
>>>>> convert the rest of the lists to use llist variant".
>>>>
>>>> I try to extra careful with list conversions, they bring hard to 
>>>> find issues. So i prefer conversions to be in small steps.
>>>>
>>>>>
>>>>> Also I think that having one lock around the whole loop introduces 
>>>>> much less lock contention than doing multiple take/release on each 
>>>>> iteration of the loop.
>>>>
>>>>
>>>> This lock is gone later with move to llist.
>>>
>>> That is the main reason why it should be merged. There should be no 
>>> reverts inside one series, moving lock unnecessarily and then 
>>> removing it completely only brings unneeded confusion for the future 
>>> reader of the code. [1]
>>
>> I checked various guides, do you have a reference for this ?
>>
>> The requirements listed are that:
>> - each patch is separate logical change
> 
>  > Each patch should be justifiable on its own merits.
> 
> https://www.kernel.org/doc/html/v5.1/process/submitting-patches.html#separate-your-changes
> 
> I don't see how adding code in one commit and then removing this code in 
> another is justifiable, especially when adding code does not help in any 
> way in the patch which removes it. I see it just as excess code.
> 

It is not adding and then removing. It is moving a lock from one 
function to another(logical change) then this lock is no longer required 
and removed(logical change). Doing this in one pass is a shortcut.

>> - each patch compiles to a working kernel
>>
>> For me locking data instead of code is a logical change,
>> ,changing list usage is separate logical change and so on.
>>
>> If lock and list change are in one place it is easier to see.
>>
>> https://www.kernel.org/doc/html/v4.11/process/coding-style.html#codingstyle
>> https://www.ozlabs.org/~akpm/stuff/tpp.txt
>> https://halobates.de/on-submitting-patches.pdf
>>
>>>
>>>   Usually there is just one
>>>> entry in the list most users dispatch single pio - locking  the list 
>>>> when it is necessary reduces the time lock is held - locking data 
>>>> instead of locking code .
>>>
>>> This kind of optimization does not feel right to me. We should either 
>>> optimize general case or have explicit optimizations for edge cases, 
>>> e.g. in ploop_dispatch_pios "if (likely(pio)) 
>>> ploop_dispatch_pio_locked()", explicitly marking function which takes 
>>> lock by itself with _locked suffix.
>>
>> This is not a optimisation. General case is one pio, exception is many 
>> pios. difference is neglectable, in the end this function is only used 
>> for more than one pio one some abnormal condition. So i do not see a 
>> point to optimise it. Fine graine locking on data is better than 
>> locking a 100 lines function code, just like your example for the 
>> locks - you can loose sense of why a lock is necessary.
>>
>>>
>>> But all this is unimportant due to [1], should be just merged.
>>
>> Ok. i will check.
>>
>>>
>>>>
>>>>>
>>>>>>   }
>>>>>>   void ploop_dispatch_pios(struct ploop *ploop, struct pio *pio,
>>>>>>                struct list_head *pio_list)
>>>>>>   {
>>>>>>       bool is_data = false, is_flush = false;
>>>>>> -    unsigned long flags;
>>>>>> -    spin_lock_irqsave(&ploop->deferred_lock, flags);
>>>>>>       if (pio)
>>>>>>           ploop_dispatch_pio(ploop, pio, &is_data, &is_flush);
>>>>>>       if (pio_list) {
>>>>>>           while ((pio = ploop_pio_list_pop(pio_list)) != NULL)
>>>>>>               ploop_dispatch_pio(ploop, pio, &is_data, &is_flush);
>>>>>>       }
>>>>>> -    spin_unlock_irqrestore(&ploop->deferred_lock, flags);
>>>>>>       if (is_data)
>>>>>>           queue_work(ploop->wq, &ploop->worker);
>>>>>
>>>>
>>>
>>
> 

-- 
Regards,
Alexander Atanasov