Re: [PATCH v1 3/3] examples/l3fwd-power: enable PMD power mgmt on Arm

[David Marchand <david.marchand@redhat.com>]

On Mon, Aug 29, 2022 at 2:48 PM Hunt, David <david.hunt@intel.com> wrote:
> On 25/08/2022 07:42, Feifei Wang wrote:
> > For Arm aarch, power monitor uses WFE instruction to enable, which can
> > not exit automatically within the time limit. This means
> > 'rte_power_monitor_wakeup' API needs to be called to wake up sleep cores
> > if there is no store operation to monitored address.
> >
> > Furthermore, we disable power monitor feature on the main core so that it
> > can be used to wake up other sleeping cores when it receives SIGINT
> > siginal.
> >
> > Signed-off-by: Feifei Wang <feifei.wang2@arm.com>
> > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > ---
> >   examples/l3fwd-power/main.c | 30 +++++++++++++++++++++++++++++-
> >   1 file changed, 29 insertions(+), 1 deletion(-)
> >
> > diff --git a/examples/l3fwd-power/main.c b/examples/l3fwd-power/main.c
> > index 887c6eae3f..2bd0d700f0 100644
> > --- a/examples/l3fwd-power/main.c
> > +++ b/examples/l3fwd-power/main.c
> > @@ -432,8 +432,16 @@ static void
> >   signal_exit_now(int sigtype)
> >   {
> >
> > -     if (sigtype == SIGINT)
> > +     if (sigtype == SIGINT) {
> > +#if defined(RTE_ARCH_ARM64)

Having a arch specific behavior in the application shows that there is
something wrong either in the API, or in the Arm implementation of the
API.
I don't think this is a good solution.

Can't we find a better alternative? By changing the API probably?


> > +     /**
> > +      * wake_up api does not need input parameter on Arm,
> > +      * so 0 is meaningless here.
> > +      */
> > +             rte_power_monitor_wakeup(0);
> > +#endif
> >               quit_signal = true;
> > +     }
> >
> >   }
> >
> > @@ -2885,6 +2893,25 @@ main(int argc, char **argv)
> >                                               "Error setting scaling freq max: err=%d, lcore %d\n",
> >                                                       ret, lcore_id);
> >
> > +#if defined(RTE_ARCH_ARM64)
> > +                             /* Ensure the main lcore does not enter the power-monitor state,
> > +                              * so that it can be used to wake up other lcores on ARM.
> > +                              * This is due to WFE instruction has no timeout wake-up mechanism,
> > +                              * and if users want to exit actively, the main lcore is needed
> > +                              * to send SEV instruction to wake up other lcores.
> > +                              */
> > +                             unsigned int main_lcore = rte_get_main_lcore();
> > +                             if (lcore_id != main_lcore ||
> > +                                             pmgmt_type != RTE_POWER_MGMT_TYPE_MONITOR) {
> > +                                     ret = rte_power_ethdev_pmgmt_queue_enable(
> > +                                                     lcore_id, portid, queueid,
> > +                                                     pmgmt_type);
> > +                                     if (ret < 0)
> > +                                             rte_exit(EXIT_FAILURE,
> > +                                                     "rte_power_ethdev_pmgmt_queue_enable: err=%d, port=%d\n",
> > +                                                     ret, portid);
> > +                             }
> > +#else
> >                               ret = rte_power_ethdev_pmgmt_queue_enable(
> >                                               lcore_id, portid, queueid,
> >                                               pmgmt_type);
> > @@ -2892,6 +2919,7 @@ main(int argc, char **argv)
> >                                       rte_exit(EXIT_FAILURE,
> >                                               "rte_power_ethdev_pmgmt_queue_enable: err=%d, port=%d\n",
> >                                                       ret, portid);
> > +#endif
> >                       }
> >               }
> >       }
>
>
> Hi Feifei,
>
> Acked-by: David Hunt <david.hunt@intel.com>


-- 
David Marchand