Re: [PATCH v3 2/2] soc: fsl: qbman: Use raw spinlock for cgr_lock

[Sean Anderson]

Hi Crystal,

On 4/4/23 12:04, Sean Anderson wrote:
> On 4/4/23 11:33, Crystal Wood wrote:
>> On Tue, 2023-04-04 at 10:55 -0400, Sean Anderson wrote:
>> 
>>> @@ -1456,11 +1456,11 @@ static void tqm_congestion_task(struct work_struct
>>> *work)
>>>         union qm_mc_result *mcr;
>>>         struct qman_cgr *cgr;
>>>  
>>> -       spin_lock_irq(&p->cgr_lock);
>>> +       raw_spin_lock_irq(&p->cgr_lock);
>>>         qm_mc_start(&p->p);
>>>         qm_mc_commit(&p->p, QM_MCC_VERB_QUERYCONGESTION);
>>>         if (!qm_mc_result_timeout(&p->p, &mcr)) {
>>> -               spin_unlock_irq(&p->cgr_lock);
>>> +               raw_spin_unlock_irq(&p->cgr_lock);
>> 
>> qm_mc_result_timeout() spins with a timeout of 10 ms which is very
>> inappropriate for a raw lock.  What is the actual expected upper bound?
> 
> Hm, maybe we can move this qm_mc stuff outside cgr_lock? In most other
> places they're called without cgr_lock, which implies that its usage
> here is meant to synchronize against some other function.

Do you have any suggestions here? I think this should really be handled
in a follow-up patch. If you think this code is waiting too long in a raw
spinlock, the existing code can wait just as long with IRQs disabled.
This patch doesn't change existing system responsiveness.

--Sean

>>>                 dev_crit(p->config->dev, "QUERYCONGESTION timeout\n");
>>>                 qman_p_irqsource_add(p, QM_PIRQ_CSCI);
>>>                 return;
>>> @@ -1476,7 +1476,7 @@ static void qm_congestion_task(struct work_struct
>>> *work)
>>>         list_for_each_entry(cgr, &p->cgr_cbs, node)
>>>                 if (cgr->cb && qman_cgrs_get(&c, cgr->cgrid))
>>>                         cgr->cb(p, cgr, qman_cgrs_get(&rr, cgr->cgrid));
>>> -       spin_unlock_irq(&p->cgr_lock);
>>> +       raw_spin_unlock_irq(&p->cgr_lock);
>>>         qman_p_irqsource_add(p, QM_PIRQ_CSCI);
>>>  }
>> 
>> The callback loop is also a bit concerning...
> 
> The callbacks (in .../dpaa/dpaa_eth.c and .../caam/qi.c) look OK. The
> only thing which might take a bit is dpaa_eth_refill_bpools, which
> allocates memory (from the atomic pool).
> 
> --Sean