Re: [PATCH 08/10] qmi: Implement QMI service request rate limiting in 'can_write_data'.

On Feb 12, 2025, at 11:29 AM, Denis Kenzior <[email protected]> wrote:
> On 2/11/25 11:52 PM, Grant Erickson wrote:
>> Determine if we need to rate-limit QMI services requests to a
>> transport-specific minimum request period. If so, return true so that
>> the queue can be retried again later.
>> ---
>>  drivers/qmimodem/qmi.c | 17 +++++++++++++++++
>>  1 file changed, 17 insertions(+)
> 
> Patches 5, 6 and 8 should belong in the same commit.  Prefer to group new member definitions with their first use.

Noted. I’ll coalesce these in a v2 patch set.

>> diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
>> index 00086578da10..591129c252e3 100644
>> --- a/drivers/qmimodem/qmi.c
>> +++ b/drivers/qmimodem/qmi.c
>> @@ -665,8 +665,23 @@ static bool can_write_data(struct l_io *io, void *user_data)
>>  {
>>   struct qmi_transport *transport = user_data;
>>   struct qmi_request *req;
>> + const uint64_t now = l_time_now();
>> + uint64_t delta = 0;
>>   int r;
>>  + /*
>> + * Determine if we need to rate-limit commands to a
>> + * transport-specific minimum request period. If so,
>> + * return true so that the queue can be retried again
>> + * later.
>> + */
>> + if (transport->last_req_sent_time_us != 0) {
> 
> Shouldn't this check min_req_period_us first, before incurring l_time_now overhead?

I’d originally had this in a pre-submit draft. That’s a good optimization; noted for v2.

>> + delta = l_time_diff(now, transport->last_req_sent_time_us);
>> +
>> + if (delta < transport->min_req_period_us)
>> + return true;
>> + }
>> +
> 
> Something like:
> if (throttling_enabled) {
> now = l_time_now();
> delta = l_time_diff(now, transport->last_req_sent_time_us);
> 
> if (delta < min_req_period_us)
> return true;
> ^^^^ Note, that this is a poor approach since it just busy-loops the event-loop until the throttle time gate expires.  Much better would be to use some sort of timeout.

Noted. I’d debated a timeout / timer approach; however, it seemed much more complicated in terms of its impact on the send / dequeue path, so I went with this approach first to validate the concept.

Best,

Grant

-- 
Principal
Nuovations

[email protected]
https://www.nuovations.com/