RE: [PATCH v2 3/3] l3fwd-power: add option to call uncore API

["Pattan, Reshma" <reshma.pattan@intel.com>]

> -----Original Message-----
> From: Kearney, Tadhg <tadhg.kearney@intel.com>

<snip>

> +*   -i (frequency index): optional, sets uncore frequency to frequency index
> value, by setting min and max values to be the same.
> +

This is not optional argument. Need to remove optional in the help text of the parameter.


> +``-i``
> +  This will allow you to set the specific uncore frequency index that
> +you want, by setting
> +  minimum and maximum values to be the same. Frequency index's are set
> +100000Hz apart from
> +  maximum to minimum.
> +  Frequency index values are in descending order, ie, index 0 is maximum
> frequency index.
> +

I would say by setting the uncore frequency  to a frequency pointed by index . The frequencies at each index differ by 100MHz. The value you have mentioned in 100Khz.


<snip>
>  #define IPv6_BYTES_FMT "%02x%02x:%02x%02x:%02x%02x:%02x%02x:"\
> -                       "%02x%02x:%02x%02x:%02x%02x:%02x%02x"
> +

This change looks to be accidental. Undo this change.

>  	printf( "IP dst = " IPv6_BYTES_FMT ", IP src = " IPv6_BYTES_FMT ", "
> -	        "port dst = %d, port src = %d, proto = %d\n",
> -	        IPv6_BYTES(key.ip_dst), IPv6_BYTES(key.ip_src),
> -	        key.port_dst, key.port_src, key.proto);
> +			"port dst = %d, port src = %d, proto = %d\n",
> +			IPv6_BYTES(key.ip_dst), IPv6_BYTES(key.ip_src),
> +			key.port_dst, key.port_src, key.proto);
>  }
> 

No Change here , so should you undo this change.

>  static inline uint16_t
> @@ -674,9 +692,9 @@ parse_ptype_one(struct rte_mbuf *m)
> 
>  static uint16_t
>  cb_parse_ptype(uint16_t port __rte_unused, uint16_t queue __rte_unused,
> -	       struct rte_mbuf *pkts[], uint16_t nb_pkts,
> -	       uint16_t max_pkts __rte_unused,
> -	       void *user_param __rte_unused)
> +		   struct rte_mbuf *pkts[], uint16_t nb_pkts,
> +		   uint16_t max_pkts __rte_unused,
> +		   void *user_param __rte_unused)

No change here, so should undo this change.

> -			     uint16_t queue_id)
> +				 uint16_t port_id,
> +				 uint16_t queue_id)

No change here, so should undo this change.

>  {
>  	uint32_t rxq_count = rte_eth_rx_queue_count(port_id, queue_id);
>  /**
> @@ -1051,7 +1069,7 @@ static int main_intr_loop(__rte_unused void
> *dummy)
>  			 * less as possible.
>  			 */
>  			for (i = 1,
> -			    lcore_idle_hint = qconf-
> >rx_queue_list[0].idle_hint;
> +				lcore_idle_hint = qconf-
> >rx_queue_list[0].idle_hint;

No change here, so should undo this change.


> @@ -1616,6 +1634,9 @@ print_usage(const char *prgname)
>  		"  [--max-pkt-len PKTLEN]\n"
>  		"  -p PORTMASK: hexadecimal bitmask of ports to
> configure\n"
>  		"  -P: enable promiscuous mode\n"
> +		"  -u: set min frequency for uncore\n"
> +		"  -U: set max frequency for uncore\n"
> +		"  -i (frequency index): set frequency index for uncore\n"

Might be eidt help text a bit, " specify the uncore frequency index that uncore should be set to."

> +	d = opendir(UNCORE_FREQUENCY_DIR);
> +	if (!d) {
> +		RTE_LOG(ERR, EAL, "Unable to open uncore frequency
> directory");

Log is for L3FWD_POWER not EAL. So need to fix this.  d == NULL should be checked rather !d. Also print error string returned by the opendir in the log.

> +		return -1;
> +	}
> +
> +	else {

Else should start in the same line where if ends. Here in other places. 
You don't need else perhaps. 


<snip>

> @@ -1861,10 +1988,12 @@ parse_args(int argc, char **argv)
>  		{CMD_LINE_OPT_SCALE_FREQ_MAX, 1, 0, 0},
>  		{NULL, 0, 0, 0}
>  	};
> +	const char *min = "min";
> +	const char *max = "max";

Instead of using strings to specify if user option is min or max. You can use either of the below options

1) Use global Integer value  and set that to 1 in U case . And set to 0 in u case. 
2)OR have parse_uncore_min_max() function with  one default argument set to 0. When the argument is passed to 1 assume it to be max freq to set else min freq to set.

Thanks,
Reshma