DPDK patches and discussions
 help / color / mirror / Atom feed
From: "David Harton (dharton)" <dharton@cisco.com>
To: "Michał Krawczyk" <mk@semihalf.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>, Marcin Wojtas <mw@semihalf.com>,
	"Tzalik, Guy" <gtzalik@amazon.com>,
	"Schmeilin, Evgeny" <evgenys@amazon.com>
Subject: Re: [dpdk-dev] [PATCH] net/ena: Fix admin cq polling for 32-bit apps
Date: Fri, 28 Jun 2019 15:46:37 +0000	[thread overview]
Message-ID: <BN6PR11MB39552FA28E9B242B7ED729D3AAFC0@BN6PR11MB3955.namprd11.prod.outlook.com> (raw)
In-Reply-To: <CAJMMOfM=j=2jTo+aYRxo7L41dQ0rzfrsYXvLJ+_kVrw2bWOr2A@mail.gmail.com>



> -----Original Message-----
> From: Michał Krawczyk <mk@semihalf.com>
> Sent: Friday, June 28, 2019 11:03 AM
> To: David Harton (dharton) <dharton@cisco.com>
> Cc: dev@dpdk.org; Marcin Wojtas <mw@semihalf.com>; Tzalik, Guy
> <gtzalik@amazon.com>; Schmeilin, Evgeny <evgenys@amazon.com>
> Subject: Re: [PATCH] net/ena: Fix admin cq polling for 32-bit apps
> 
> Hi,
> 
> sorry for the late reply.
> 
> śr., 29 maj 2019 o 23:01 David Harton <dharton@cisco.com> napisał(a):
> >
> > Recent modifications to admin command queue polling logic did not
> > support 32-bit applications.  Updated the driver to work for 32 or 64
> > bit applications as well as avoiding roll-over possibility.
> >
> > Fixes: 3adcba9a89 ("net/ena: update HAL to the newer version")
> >
> > Signed-off-by: David Harton <dharton@cisco.com>
> > ---
> >  drivers/net/ena/base/ena_com.c       | 10 +++++++---
> >  drivers/net/ena/base/ena_plat_dpdk.h |  6 +-----
> >  2 files changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/net/ena/base/ena_com.c
> > b/drivers/net/ena/base/ena_com.c index b688067f7..b96adde3c 100644
> > --- a/drivers/net/ena/base/ena_com.c
> > +++ b/drivers/net/ena/base/ena_com.c
> > @@ -547,10 +547,13 @@ static int
> ena_com_wait_and_process_admin_cq_polling(struct ena_comp_ctx *comp_c
> >                                                      struct
> > ena_com_admin_queue *admin_queue)  {
> >         unsigned long flags = 0;
> > -       unsigned long timeout;
> > +       u32 timeout_ms;
> >         int ret;
> >
> > -       timeout = ENA_GET_SYSTEM_TIMEOUT(admin_queue-
> >completion_timeout);
> > +       /* Calculate ms granularity timeout from us completion_timeout
> > +        * making sure we retry once if we have at least 1ms
> > +        */
> > +       timeout_ms = (admin_queue->completion_timeout / 1000) +
> > + (ENA_POLL_MS - 1);
> >
> >         while (1) {
> >                  ENA_SPINLOCK_LOCK(admin_queue->q_lock, flags); @@
> > -560,7 +563,7 @@ static int
> ena_com_wait_and_process_admin_cq_polling(struct ena_comp_ctx *comp_c
> >                  if (comp_ctx->status != ENA_CMD_SUBMITTED)
> >                         break;
> >
> > -               if (ENA_TIME_EXPIRE(timeout)) {
> > +               if (timeout_ms < ENA_POLL_MS) {
> >                         ena_trc_err("Wait for completion (polling)
> timeout\n");
> >                         /* ENA didn't have any completion */
> >                         ENA_SPINLOCK_LOCK(admin_queue->q_lock, flags);
> > @@ -573,6 +576,7 @@ static int
> ena_com_wait_and_process_admin_cq_polling(struct ena_comp_ctx *comp_c
> >                 }
> >
> >                 ENA_MSLEEP(ENA_POLL_MS);
> > +               timeout_ms -= ENA_POLL_MS;
> 
> This part can be problematic at the very overloaded systems - in that case
> the ENA_MSLEEP can take a much longer than ENA_POLL_MS and in this
> situation the time spent in this function can't be determined.
> That's why we were checking time spent in sleep every ENA_TIME_EXPIRE
> macro.
> The issue can be observed especially in the kernel drivers, and ena_com is
> common file for all ENA drivers.

I don't understand the comment/concern.

The previous macros calculate the future cycle count based on a us timeout value (assuming 64 bit apps) and repeat the loop until the command is "submitted" or the current cycle count is greater than the calculated cycle count value sleeping ENA_POLL_MS between each iteration.

The new method accomplishes the same thing but instead of using a "cycle count" it uses the number of ms which the poll and sleep actions are based upon.

The differences with the new method are:
 - it uses less instructions
 - not susceptible to cycle count overrun (admittedly highyl unlikely)
 - (most importantly) works equally well for 32 or 64 bit apps 

Can you elaborate on your concern?

Thanks,
Dave


  reply	other threads:[~2019-06-28 15:46 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-29 21:01 David Harton
2019-06-27 15:37 ` Ferruh Yigit
2019-06-28 15:03 ` Michał Krawczyk
2019-06-28 15:46   ` David Harton (dharton) [this message]
2019-07-01  7:24     ` Michał Krawczyk
2019-07-01 12:00       ` David Harton (dharton)
2019-07-01 12:52         ` Michał Krawczyk
2019-07-12 17:35 ` [dpdk-dev] [PATCH v2] " David Harton
2019-07-17 14:29   ` Michał Krawczyk
2019-07-17 15:15     ` Ferruh Yigit

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=BN6PR11MB39552FA28E9B242B7ED729D3AAFC0@BN6PR11MB3955.namprd11.prod.outlook.com \
    --to=dharton@cisco.com \
    --cc=dev@dpdk.org \
    --cc=evgenys@amazon.com \
    --cc=gtzalik@amazon.com \
    --cc=mk@semihalf.com \
    --cc=mw@semihalf.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).