| Message ID | 1cf82d1527078e71d7a64b4f8caa7c5adad680c7.1739339173.git.gerickson@nuovations.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add QMI Device Service Request Rate-limit Option | expand |
Hi Grant, 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. > 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? > + 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. transport->last_req_sent_time_us = now; } > req = l_queue_pop_head(transport->req_queue); > if (!req) > return false; Regards, -Denis
On Feb 12, 2025, at 11:29 AM, Denis Kenzior <denkenz@gmail.com> 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
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) { + 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; @@ -677,6 +692,8 @@ static bool can_write_data(struct l_io *io, void *user_data) return false; } + transport->last_req_sent_time_us = now; + if (l_queue_length(transport->req_queue) > 0) return true;