RE: [PATCH v10 2/2] examples/l3fwd-power: add PM QoS configuration

[Konstantin Ananyev <konstantin.ananyev@huawei.com>]

> >
> >>>> Add PM QoS configuration to declease the delay after sleep in case of
> >>>> entering deeper idle state.
> >>>>
> >>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> >>>> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> >>>> ---
> >>>>    examples/l3fwd-power/main.c | 24 ++++++++++++++++++++++++
> >>>>    1 file changed, 24 insertions(+)
> >>>>
> >>>> diff --git a/examples/l3fwd-power/main.c b/examples/l3fwd-power/main.c
> >>>> index 2bb6b092c3..b0ddb54ee2 100644
> >>>> --- a/examples/l3fwd-power/main.c
> >>>> +++ b/examples/l3fwd-power/main.c
> >>>> @@ -47,6 +47,7 @@
> >>>>    #include <rte_telemetry.h>
> >>>>    #include <rte_power_pmd_mgmt.h>
> >>>>    #include <rte_power_uncore.h>
> >>>> +#include <rte_power_qos.h>
> >>>>
> >>>>    #include "perf_core.h"
> >>>>    #include "main.h"
> >>>> @@ -2260,6 +2261,22 @@ init_power_library(void)
> >>>>    			return -1;
> >>>>    		}
> >>>>    	}
> >>>> +
> >>>> +	RTE_LCORE_FOREACH(lcore_id) {
> >>>> +		/*
> >>>> +		 * Set the worker lcore's to have strict latency limit to allow
> >>>> +		 * the CPU to enter the shallowest idle state.
> >>>> +		 */
> >>>> +		ret = rte_power_qos_set_cpu_resume_latency(lcore_id,
> >>>> +					RTE_POWER_QOS_STRICT_LATENCY_VALUE);
> >>> I wonder why it is set to all worker cores silently and unconditionally?
> >>> Wouldn't it be a change from current behavior of the power library?
> >> L3fwd-power uses Rx interrupt to receive packet.
> > AFAIK, not exactly.
> >  From what I remember l3fwd-power still runs RX in poll mode,
> > thought it counts number of idle rx bursts.
> > As that number goes above some threshold, it puts itself into
> > sleep with some timeout value.
> Exactly.
> >
> >> Do you mean this
> >> setting should be for the core of Rx queue, right?
> >> This setting doesn't change the behavior of l3fwd-power. It is just for
> >> getting low resume latency when worker core wakes up from sleeping.
> > As understand your patch - you force CPU to select more shallow C state
> > when entering such sleep.
> > Then it means that possible packet loss will be smaller,
> > but power consumption probably higher, correct?
> correct.
> > If so, then it looks like a change from current  behavior for that app,
> > and we probably need to document what will be an expected change.
> > Or probably as a better way - provider user with a way to choose,
> > new cmdline option or so.
> Yes.
> The power consumption may increase but the performance is better due to
> this patch if the platform enables cpuidle funtion.

Yes, that what I expect, and personally I am ok with that.
Though I suspect different users who use this sample as some test-app
might have different priorities in that tradeoff (power vs performance).

> After all, this is just a very little point. It is enough to document
> this change or impact in doc of this API. Just let it more clear for user.
> What do you think?

I think yes, probably just updating docs (rel-notes, SG ?) will be enough.
David Hunt, what are your thoughts here?

> >
> >>>> +		if (ret != 0) {
> >>>> +			RTE_LOG(ERR, L3FWD_POWER,
> >>>> +				"Failed to set strict resume latency on core%u.\n",
> >>>> +				lcore_id);
> >>>> +			return ret;
> >>>> +		}
> >>>> +	}
> >>>> +
> >>>>    	return ret;
> >>>>    }
> >>>>
> >>>> @@ -2299,6 +2316,13 @@ deinit_power_library(void)
> >>>>    			}
> >>>>    		}
> >>>>    	}
> >>>> +
> >>>> +	RTE_LCORE_FOREACH(lcore_id) {
> >>>> +		/* Restore the original value in kernel. */
> >>>> +		rte_power_qos_set_cpu_resume_latency(lcore_id,
> >>>> +				RTE_POWER_QOS_RESUME_LATENCY_NO_CONSTRAINT);
> >>>> +	}
> >>>> +
> >>>>    	return ret;
> >>>>    }
> >>>>
> >>>> --
> >>>> 2.22.0