Re: [PATCH v2 4/7] qmi: Implement QMI service request rate limiting in 'can_write_data'.

On Feb 12, 2025, at 6:07 PM, Denis Kenzior <[email protected]> wrote:
> On 2/12/25 6:33 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.
>> Adds the following data members to 'qmi_transport':
>>   * Minimum request period
>>     - The minimum period, in microseconds, for back-to-back QMI
>>       service requests.
>>   * Last request time
>>     - Time, in microseconds, when the last QMI service request was
>>       sent.
>> to support the implementation.
>> ---
>> @@ -653,8 +665,25 @@ static bool can_write_data(struct l_io *io, void *user_data)
>>  {
>>   struct qmi_transport *transport = user_data;
>>   struct qmi_request *req;
>> + const bool throttle_enabled = transport->min_req_period_us > 0;
>> + uint64_t now;
>> + uint64_t delta;
>>   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 (throttle_enabled && transport->last_req_sent_time_us != 0) {
>> + now = l_time_now();
>> + delta = l_time_diff(now, transport->last_req_sent_time_us);
>> +
>> + if (delta < transport->min_req_period_us)
>> + return true;
>> + }
>> +
>>   req = l_queue_pop_head(transport->req_queue);
>>   if (!req)
>>   return false;
>> @@ -665,6 +694,9 @@ static bool can_write_data(struct l_io *io, void *user_data)
>>   return false;
>>   }
>>  + if (throttle_enabled)
>> + transport->last_req_sent_time_us = now;
> 
> Pretty sure this won't work and gcc will complain.

Surprisingly, GCC 8.2.0 didn’t squawk about it:

% ${SYSROOT}usr/bin/arm-dey-linux-gnueabi/arm-dey-linux-gnueabi-gcc --version
arm-dey-linux-gnueabi-gcc (GCC) 8.2.0
Copyright (C) 2018 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

% ${SYSROOT}usr/bin/arm-dey-linux-gnueabi/arm-dey-linux-gnueabi-gcc
-DHAVE_CONFIG_H -I. -I${PROJECTROOT}third_party/ofono/repo -I./include -I./src
-I${PROJECTROOT}third_party/ofono/repo/src
-I${PROJECTROOT}third_party/ofono/repo/gdbus
-I${PROJECTROOT}third_party/ofono/repo/gisi
-I${PROJECTROOT}third_party/ofono/repo/gatchat
-I${PROJECTROOT}third_party/ofono/repo/gril --sysroot=${PROJECTROOT}rootfs
-isystem ${SYSROOT}usr/include
-I${PROJECTROOT}results/${BUILDPRODUCT}/digi/dey/8.2.0/${BUILDCONFIG}/third_party/linux/linux-dey/include
-I${PROJECTROOT}results/${BUILDPRODUCT}/digi/dey/8.2.0/${BUILDCONFIG}/third_party/dbus/usr/include/dbus-1.0
-I${PROJECTROOT}results/${BUILDPRODUCT}/digi/dey/8.2.0/${BUILDCONFIG}/third_party/dbus/usr/lib/dbus-1.0/include
-I${PROJECTROOT}results/${BUILDPRODUCT}/digi/dey/8.2.0/${BUILDCONFIG}/third_party/glib/usr/include/gio-unix-2.0
-I${PROJECTROOT}results/${BUILDPRODUCT}/digi/dey/8.2.0/${BUILDCONFIG}/third_party/glib/usr/include/glib-2.0
-I${PROJECTROOT}results/${BUILDPRODUCT}/digi/dey/8.2.0/${BUILDCONFIG}/third_party/glib/usr/lib/glib-2.0/include
-I${PROJECTROOT}results/${BUILDPRODUCT}/digi/dey/8.2.0/${BUILDCONFIG}/third_party/ell/usr/include
-I${PROJECTROOT}results/${BUILDPRODUCT}/digi/dey/8.2.0/${BUILDCONFIG}/third_party/eudev/usr/include
-DOFONO_PLUGIN_BUILTIN -DPLUGINDIR=\""/usr/lib/ofono/plugins"\"
-DUNITDIR=\""./unit/"\" --sysroot=${PROJECTROOT}rootfs -mcpu=cortex-a8
-mfloat-abi=hard -mfpu=neon -fno-omit-frame-pointer -fno-strict-aliasing
-O2 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -MT drivers/qmimodem/qmi.o -MD -MP
-MF $depbase.Tpo -c -o drivers/qmimodem/qmi.o
${PROJECTROOT}third_party/ofono/repo/drivers/qmimodem/qmi.c && mv -f
$depbase.Tpo $depbase.Po

I’ve another evaluation branch with GCC 12.3.1. I’ll cross-check there.

>  Compilers are still not great at tying together multiple conditionals...  You might still be better off keeping this inside the if() you added above...

There are two separate actions to be taken:

1. Performing the rate limit check before the request is dequeued and sent.
2. Setting the last sent time after the request is dequeued and successfully sent.

So, there need to be two, separate conditional blocks for each.

> Also, don't you need last_req_sent_time_us to be initialized to be non-zero somehow?  Otherwise it is dumb luck that last_req_sent_time_us is set to anything the first time (probably garbage data)

I thought about zero-initializing it in ‘qmi_qmux_device_new’; however, diving into the implementation of ‘l_new’, it appears as though all memory is zeroed out, so the zero-initialization seemed redundant.

I’ll have a v3 shortly based on the feedback from GCC 12.3.1.

Best,

Grant

-- 
Principal
Nuovations

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