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

[Alexander Atanasov]

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 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