Re: [PATCH] interconnect: Replace mutex with rt_mutex

["Aiqun(Maria) Yu" <quic_aiquny@quicinc.com>]

On 9/7/2022 10:59 PM, Mike Tipton wrote:
> On Wed, Sep 07, 2022 at 10:35:26AM +0300, Georgi Djakov wrote:
>> Hi Mike,
>>
>> Thanks for the patch!
>>
>> On 6.09.22 22:14, Mike Tipton wrote:
>>> Replace mutex with rt_mutex to prevent priority inversion between
>>> clients, which can cause unacceptable delays in some cases.
>>
>> It would be nice if you have any numbers to share in the commit text.
> 
> I can try to dig up some numbers, but mileage will vary tremendously of
> course. Improvement is really only seen in certain high-concurrency
> scenarios.

We need to revisit this thread because the issue has been reported again
recently.
Here is the data I can provide regarding the performance issue. Please
check if it is sufficient for the commit message to understand the change.
The CFS normal tasks holding the mutex lock were runnable for
approximately 40ms in a busy load scenario, causing the RT task to wait
for the mutex for about 40ms, which resulted in the RT task not being
'real-time' enough and causing janks. Changing the mutex to an RT mutex
helped the caller of the interface, such as icc_set_bw, to ensure that
RT tasks can deliver RT priority to the current RT mutex owner quickly,
thereby improving performance in this scenario.


*Before the change the scenario is like:
 +------------+           +-----------------+


  | RT Task A  |           |Normal cfs task B|
                                                +------------+
+-----------------+


                            mutex_lock(&icc_lock)


  call icc_set_bw()


  wait mutex_unlock(&icc_lock)


                            wait other high priority tasks


                            mutex_unlock(&icc_lock)


  get the lock





*After the change the solution will be like:




   +------------+           +-----------------+


   | RT Task A  |           |Normal cfs task B|


   +------------+           +-----------------+


                          rt_mutex_lock(&icc_lock)


                           wait other high priority task





   call icc_set_bw()


   rt_mutex_lock(&icc_lock)


   -->boost task_B prio


                            Get the chance to run


                             -->mutex_unlock(&icc_lock)


                             -->deboost task_B prio


   get the lock with RT prio

Please comment if more information is needed to apply the current
similar patch. If there are no objections, we can rebase and upload a
new patch set accordingly.
> 
>>
>>> Signed-off-by: Mike Tipton <quic_mdtipton@quicinc.com>
>>> ---
>>>
>>> We've run into a number of cases internally and from customers where
>>> high priority, RT clients (typically display) become blocked for long
>>> periods of time on lower priority, non-RT clients. Switching to rt_mutex
>>> has proven to help performance in these cases.
>>
>> I am wondering if avoiding the inversion on this specific lock is the right
>> solution, as there could be other locks that may cause similar issues. Do we
>> see similar issue with clocks for example or is it just with interconnects?

The current issue has been captured in multiple projects, while it has
only been reported in interconnects so far. We need to understand
whether specific mutex locks such as those from clocks, are being called
by RT tasks in any possible scenarios.
> 
> I raised these same concerns internally, since some of the clients
> experiencing delays also request clocks and regulators. However, I
> believe they primarily request interconnects in these critical paths and
> not clocks/regulators. At least not as frequently as they request
> interconnect. Additionally, I suspect the average interconnect latencies
> are higher than clock on our platforms, since interconnect will always
> result in blocking calls to RPM/RPMh. I also wouldn't be surprised if we
> have more consistent contention on interconnect, since certain clients
> update DDR BW quite frequently. I suppose at some point the same
> rt_mutex argument could be made for clock and regulator as well, but
> to-date we've only needed to change interconnect to see improvement.
> 
> I'm not sure what an alternative, generic solution would be. We have
> many clients requesting many different paths. Some are more
> latency-sensitive and higher priority than others. If these use cases
> overlap, then we're subject to these sorts of priority inversion issues.
> Bumping the priority of all clients to match the highest priority one
> isn't really possible.

Based on my understanding, rt_mutex is a good API to solve this type of
issue.

-- 
Thx and BRs,
Aiqun(Maria) Yu