RE: [PATCH v3 08/33] net/ena/hal: exponential backoff exp limit

["Brandes, Shai" <shaibran@amazon.com>]

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@amd.com>
> Sent: Friday, March 8, 2024 7:24 PM
> To: Brandes, Shai <shaibran@amazon.com>
> Cc: dev@dpdk.org
> Subject: RE: [EXTERNAL] [PATCH v3 08/33] net/ena/hal: exponential backoff
> exp limit
> 
> CAUTION: This email originated from outside of the organization. Do not click
> links or open attachments unless you can confirm the sender and know the
> content is safe.
> 
> 
> 
> On 3/6/2024 12:24 PM, shaibran@amazon.com wrote:
> > From: Shai Brandes <shaibran@amazon.com>
> >
> > limits the exponent in the exponential backoff mechanism in order to
> > avoid the value overflowing.
> >
> 
> Is this a fix?
> 
> What was the impact of the overflowing if not limited? And is there a
> significance of the value 16, can you please elaborate?
> 
[Brandes, Shai] I will restructure this patch, since this likely hides a fix in hal.
It is originated from the HAL release, from which I took the patches one by one, but the commit messages there tend to be (too) concise. 

> 
> Also let me remind the patch subject format, (this may look insignificant but
> helps to have more unified commit messages for developers, and if not
> updated by author, maintainers update it and this brings more overhead to
> maintainers):
> "sub-module: verb object"
> 
> And we use verb 'fix' explicitly for all commits fixing something, and that
> something can't be referenced as 'error', 'failure', 'issue', 'problem', etc... but
> it should be detailed.
> 
> Most of the times better to document NOT from driver internal perspective,
> but impact of it, like "net/ena: set chain limit to 16" is NOT a good one, it
> explains driver internal perspective (making all up) but it can be something
> like:
> "net/ena: support big packets by increasing link limit"
> 
> For this one, I am not sure impact of the change so hard for me to propose an
> alternative, but just as example it can be something like:
> "net/ena/base: avoid collision by limiting backoff delay"
> 
> > Signed-off-by: Shai Brandes <shaibran@amazon.com>
> > Reviewed-by: Amit Bernstein <amitbern@amazon.com>
> > ---
> >  drivers/net/ena/hal/ena_com.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ena/hal/ena_com.c
> > b/drivers/net/ena/hal/ena_com.c index 6953a1fa33..31c37b0ab3 100644
> > --- a/drivers/net/ena/hal/ena_com.c
> > +++ b/drivers/net/ena/hal/ena_com.c
> > @@ -34,6 +34,8 @@
> >
> >  #define ENA_REGS_ADMIN_INTR_MASK 1
> >
> > +#define ENA_MAX_BACKOFF_DELAY_EXP 16U
> > +
> >  #define ENA_MIN_ADMIN_POLL_US 100
> >
> >  #define ENA_MAX_ADMIN_POLL_US 5000
> > @@ -545,8 +547,9 @@ static int ena_com_comp_status_to_errno(struct
> > ena_com_admin_queue *admin_queue,
> >
> >  static void ena_delay_exponential_backoff_us(u32 exp, u32 delay_us)
> > {
> > +     exp = ENA_MIN32(ENA_MAX_BACKOFF_DELAY_EXP, exp);
> >       delay_us = ENA_MAX32(ENA_MIN_ADMIN_POLL_US, delay_us);
> > -     delay_us = ENA_MIN32(delay_us * (1U << exp),
> ENA_MAX_ADMIN_POLL_US);
> > +     delay_us = ENA_MIN32(ENA_MAX_ADMIN_POLL_US, delay_us * (1U
> <<
> > + exp));
> >       ENA_USLEEP(delay_us);
> >  }
> >