Re: [dpdk-dev] [PATCH v3 6/7] power: support monitoring multiple Rx queues

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

> >> Use the new multi-monitor intrinsic to allow monitoring multiple ethdev
> >> Rx queues while entering the energy efficient power state. The multi
> >> version will be used unconditionally if supported, and the UMWAIT one
> >> will only be used when multi-monitor is not supported by the hardware.
> >>
> >> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> >> ---
> >>   doc/guides/prog_guide/power_man.rst |  9 ++--
> >>   lib/power/rte_power_pmd_mgmt.c      | 76 ++++++++++++++++++++++++++++-
> >>   2 files changed, 80 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/doc/guides/prog_guide/power_man.rst b/doc/guides/prog_guide/power_man.rst
> >> index fac2c19516..3245a5ebed 100644
> >> --- a/doc/guides/prog_guide/power_man.rst
> >> +++ b/doc/guides/prog_guide/power_man.rst
> >> @@ -221,13 +221,16 @@ power saving whenever empty poll count reaches a certain number.
> >>   The "monitor" mode is only supported in the following configurations and scenarios:
> >>
> >>   * If ``rte_cpu_get_intrinsics_support()`` function indicates that
> >> +  ``rte_power_monitor_multi()`` function is supported by the platform, then
> >> +  monitoring multiple Ethernet Rx queues for traffic will be supported.
> >> +
> >> +* If ``rte_cpu_get_intrinsics_support()`` function indicates that only
> >>     ``rte_power_monitor()`` is supported by the platform, then monitoring will be
> >>     limited to a mapping of 1 core 1 queue (thus, each Rx queue will have to be
> >>     monitored from a different lcore).
> >>
> >> -* If ``rte_cpu_get_intrinsics_support()`` function indicates that the
> >> -  ``rte_power_monitor()`` function is not supported, then monitor mode will not
> >> -  be supported.
> >> +* If ``rte_cpu_get_intrinsics_support()`` function indicates that neither of the
> >> +  two monitoring functions are supported, then monitor mode will not be supported.
> >>
> >>   * Not all Ethernet devices support monitoring, even if the underlying
> >>     platform may support the necessary CPU instructions. Please refer to
> >> diff --git a/lib/power/rte_power_pmd_mgmt.c b/lib/power/rte_power_pmd_mgmt.c
> >> index 7762cd39b8..aab2d4f1ee 100644
> >> --- a/lib/power/rte_power_pmd_mgmt.c
> >> +++ b/lib/power/rte_power_pmd_mgmt.c
> >> @@ -155,6 +155,24 @@ queue_list_remove(struct pmd_core_cfg *cfg, const union queue *q)
> >>        return 0;
> >>   }
> >>
> >> +static inline int
> >> +get_monitor_addresses(struct pmd_core_cfg *cfg,
> >> +             struct rte_power_monitor_cond *pmc)
> >> +{
> >> +     const struct queue_list_entry *qle;
> >> +     size_t i = 0;
> >> +     int ret;
> >> +
> >> +     TAILQ_FOREACH(qle, &cfg->head, next) {
> >> +             struct rte_power_monitor_cond *cur = &pmc[i];
> >
> > Looks like you never increment 'i' value inside that function.
> > Also it probably will be safer to add 'num' parameter to check that
> > we will never over-run pmc[] boundaries.
> 
> Will fix in v4, good catch!
> 
> >
> >> +             const union queue *q = &qle->queue;
> >> +             ret = rte_eth_get_monitor_addr(q->portid, q->qid, cur);
> >> +             if (ret < 0)
> >> +                     return ret;
> >> +     }
> >> +     return 0;
> >> +}
> >> +
> >>   static void
> >>   calc_tsc(void)
> >>   {
> >> @@ -183,6 +201,48 @@ calc_tsc(void)
> >>        }
> >>   }
> >>
> >> +static uint16_t
> >> +clb_multiwait(uint16_t port_id, uint16_t qidx,
> >> +             struct rte_mbuf **pkts __rte_unused, uint16_t nb_rx,
> >> +             uint16_t max_pkts __rte_unused, void *addr __rte_unused)
> >> +{
> >> +     const unsigned int lcore = rte_lcore_id();
> >> +     const union queue q = {.portid = port_id, .qid = qidx};
> >> +     const bool empty = nb_rx == 0;
> >> +     struct pmd_core_cfg *q_conf;
> >> +
> >> +     q_conf = &lcore_cfg[lcore];
> >> +
> >> +     /* early exit */
> >> +     if (likely(!empty)) {
> >> +             q_conf->empty_poll_stats = 0;
> >> +     } else {
> >> +             /* do we care about this particular queue? */
> >> +             if (!queue_is_power_save(q_conf, &q))
> >> +                     return nb_rx;
> >
> > I still don't understand the need of 'special' power_save queue here...
> > Why we can't just have a function:
> >
> > get_number_of_queues_whose_sequential_empty_polls_less_then_threshold(struct pmd_core_cfg *lcore_cfg),
> > and then just:
> >
> > /* all queues have at least EMPTYPOLL_MAX sequential empty polls */
> > if (get_number_of_queues_whose_sequential_empty_polls_less_then_threshold(q_conf) == 0) {
> >      /* go into power-save mode here */
> > }
> 
> Okay, let's go through this step by step :)
> 
> Let's suppose we have three queues - q0, q1 and q2. We want to sleep
> whenever there's no traffic on *all of them*, however we cannot know
> that until we have checked all of them.
> 
> So, let's suppose that q0, q1 and q2 were empty all this time, but now
> some traffic arrived at q2 while we're still checking q0. We see that q0
> is empty, and all of the queues were empty for the last N polls, so we
> think we will be safe to sleep at q0 despite the fact that traffic has
> just arrived at q2.
> This is not an issue with MONITOR mode because we will be able to see if
> current Rx ring descriptor is busy or not via the NIC callback, *but
> this is not possible* with PAUSE and SCALE modes, because they don't
> have the sneaky lookahead function of MONITOR! So, with PAUSE and SCALE
> modes, it is possible to end up in a situation where you *think* you
> don't have any traffic, but you actually do, you just haven't checked
> the relevant queue yet.

I think such situation is unavoidable.
Yes, traffic can arrive to *any* queue at *any* time.
With your example above - user choose q2 as 'special' queue, but
traffic actually arrives on q0 or q1. 
And yes, if user choose PAUSE or SCALE methods he *can* miss the traffic,   
because as you said for these methods there is no notification mechanisms.
I think there are just unavoidable limitations with these power-save methods. 
 
> In order to prevent this from happening, we do not sleep on every queue,
> instead we sleep *once* per loop. 

Yes, totally agree we shouldn't sleep on *every* queue.
We need to go to sleep when there is no traffic on *any* of queues we monitor. 

> That is, we check q0, check q1, check
> q2, and only then we decide whether we want to sleep or not.

> Of course, with such scheme it is still possible to e.g. sleep in q2
> while there's traffic waiting in q0,

Yes, exactly.

> but worst case is less bad with
> this scheme, because we'll be doing at worst 1 extra sleep.

Hmm, I think it would be one extra sleep anyway.

> Whereas with what you're suggesting, if we had e.g. 10 queues to poll,
> and we checked q1 but traffic has just arrived at q0, we'll be sleeping
> at q1, then we'll be sleeping at q2, then we'll be sleeping at q3, then
> we'll be sleeping at q4, then we'll be sleeping at q5.... and 9 sleeps
> later we finally reach q0 and find out after all this time that we
> shouldn't have slept in the first place.

Ah ok, I think I understand now what you are saying.
Sure, to avoid such situation, we'll need to maintain extra counters and
update them properly when we go to sleep.   
I should state it clearly at the beginning.
It might be easier to explain what I meant by code snippet:

lcore_conf needs 2 counters:
uint64_t   nb_queues_ready_to_sleep;
uint64_t   nb_sleeps;
 
Plus each queue needs 2 counters:
uint64_t nb_empty_polls;
uint64_t nb_sleeps;

Now, at rx_callback():

/* check did sleep happen since previous call,
     if yes, then reset queue counters */
if (queue->nb_sleeps != lcore_conf->nb_sleeps) {
    queue->nb_sleeps = lcore_conf->nb_sleeps;
    queue->nb_empty_polls = 0;
}

 /* packet arrived, reset counters */
 if (nb_rx != 0) {
   /* queue is not 'ready_to_sleep' any more */
   if (queue->nb_empty_polls > EMPTYPOLL_MAX)
       lcore_conf-> nb_queues_ready_to_sleep--;
   queue->nb_empty_polls = 0;

/* empty poll */
} else {
    /* queue reaches EMPTYPOLL_MAX threshold, mark it as 'ready_to_sleep' */ 
    if (queue->nb_empty_polls == EMPTYPOLL_MAX)
       lcore_conf-> nb_queues_ready_to_sleep++;
    queue->nb_empty_polls++;
}

   /* no traffic on any queue for at least EMPTYPOLL_MAX iterations */
   if (lcore_conf-> nb_queues_ready_to_sleep == lcore_conf->n_queues) {
      /* update counters and sleep */
      lcore_conf->nb_sleeps++;
      lcore_conf-> nb_queues_ready_to_sleep = 0;
      goto_sleep();
   }
}

> Hopefully you get the point now :)
> 
> So, the idea here is, for any N queues, sleep only once, not N times.
> 
> >
> >> +
> >> +             /*
> >> +              * we can increment unconditionally here because if there were
> >> +              * non-empty polls in other queues assigned to this core, we
> >> +              * dropped the counter to zero anyway.
> >> +              */
> >> +             q_conf->empty_poll_stats++;
> >> +             if (unlikely(q_conf->empty_poll_stats > EMPTYPOLL_MAX)) {
> >> +                     struct rte_power_monitor_cond pmc[RTE_MAX_ETHPORTS];
> >
> > I think you need here:
> > struct rte_power_monitor_cond pmc[q_conf->n_queues];
> 
> I think VLA's are generally agreed upon to be unsafe, so i'm avoiding
> them here.

Wonder why?
These days DPDK uses VLA in dozens of places...
But if you'd like to avoid VLA - you can use alloca(),
or have lcore_conf->pmc[] and realloc() it when new queue is
added/removed from the list.

> 
> >
> >
> >> +                     uint16_t ret;
> >> +
> >> +                     /* gather all monitoring conditions */
> >> +                     ret = get_monitor_addresses(q_conf, pmc);
> >> +
> >> +                     if (ret == 0)
> >> +                             rte_power_monitor_multi(pmc,
> >> +                                     q_conf->n_queues, UINT64_MAX);
> >> +             }
> >> +     }
> >> +
> >> +     return nb_rx;
> >> +}
> >> +
> >>   static uint16_t
> >>   clb_umwait(uint16_t port_id, uint16_t qidx, struct rte_mbuf **pkts __rte_unused,
> >>                uint16_t nb_rx, uint16_t max_pkts __rte_unused,
> >> @@ -348,14 +408,19 @@ static int
> >>   check_monitor(struct pmd_core_cfg *cfg, const union queue *qdata)
> >>   {
> >>        struct rte_power_monitor_cond dummy;
> >> +     bool multimonitor_supported;
> >>
> >>        /* check if rte_power_monitor is supported */
> >>        if (!global_data.intrinsics_support.power_monitor) {
> >>                RTE_LOG(DEBUG, POWER, "Monitoring intrinsics are not supported\n");
> >>                return -ENOTSUP;
> >>        }
> >> +     /* check if multi-monitor is supported */
> >> +     multimonitor_supported =
> >> +                     global_data.intrinsics_support.power_monitor_multi;
> >>
> >> -     if (cfg->n_queues > 0) {
> >> +     /* if we're adding a new queue, do we support multiple queues? */
> >> +     if (cfg->n_queues > 0 && !multimonitor_supported) {
> >>                RTE_LOG(DEBUG, POWER, "Monitoring multiple queues is not supported\n");
> >>                return -ENOTSUP;
> >>        }
> >> @@ -371,6 +436,13 @@ check_monitor(struct pmd_core_cfg *cfg, const union queue *qdata)
> >>        return 0;
> >>   }
> >>
> >> +static inline rte_rx_callback_fn
> >> +get_monitor_callback(void)
> >> +{
> >> +     return global_data.intrinsics_support.power_monitor_multi ?
> >> +             clb_multiwait : clb_umwait;
> >> +}
> >> +
> >>   int
> >>   rte_power_ethdev_pmgmt_queue_enable(unsigned int lcore_id, uint16_t port_id,
> >>                uint16_t queue_id, enum rte_power_pmd_mgmt_type mode)
> >> @@ -434,7 +506,7 @@ rte_power_ethdev_pmgmt_queue_enable(unsigned int lcore_id, uint16_t port_id,
> >>                if (ret < 0)
> >>                        goto end;
> >>
> >> -             clb = clb_umwait;
> >> +             clb = get_monitor_callback();
> >>                break;
> >>        case RTE_POWER_MGMT_TYPE_SCALE:
> >>                /* check if we can add a new queue */
> >> --
> >> 2.25.1
> >
> 
> 
> --
> Thanks,
> Anatoly