Re: [dpdk-dev] [PATCH v2 1/2] app/testpmd: add commands to test new Rx offload API

["Dai, Wei" <wei.dai@intel.com>]

> -----Original Message-----
> From: Wu, Jingjing
> Sent: Thursday, March 15, 2018 3:41 AM
> To: Dai, Wei <wei.dai@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Cc: dev@dpdk.org
> Subject: RE: [PATCH v2 1/2] app/testpmd: add commands to test new Rx
> offload API
> 
> <...>
> 
> > +static int
> > +config_rx_offload(const char *name, uint64_t *offload, int on) {
> > +	uint64_t local = *offload;
> > +
> > +	if (!strcmp(name, "vlan_strip")) {
> > +		if (on)
> > +			local |= DEV_RX_OFFLOAD_VLAN_STRIP;
> > +		else
> > +			local &= ~DEV_RX_OFFLOAD_VLAN_STRIP;
> Would it decrease the lines if move the "on" checking to the end of this
> function, just set the BIT which you want to set or mask here?

Thanks, will follow your suggestion in v3 patch set.

> <...>
> 
> > +static void
> > +cmd_config_per_port_rx_offload_parsed(void *parsed_result,
> > +				__attribute__((unused)) struct cmdline *cl,
> > +				__attribute__((unused)) void *data) {
> > +	struct cmd_config_per_port_rx_offload_result *res = parsed_result;
> > +	portid_t port_id = res->port_id;
> > +	struct rte_port *port = &ports[port_id];
> > +	int on;
> > +
> > +	if (port->port_status != RTE_PORT_STOPPED) {
> > +		printf("Error: Can't config offload when Port %d "
> > +		       "is not stopped\n", port_id);
> > +		return;
> > +	}
> > +
> > +	if (!strcmp(res->en_dis, "enable"))
> > +		on = 1;
> > +	else if (!strcmp(res->en_dis, "disable"))
> > +		on = 0;
> > +	else {
> > +		printf("Unknown parameter: %s\n", res->en_dis);
> > +		return;
> The en_dis is already defined as "enable#disable", so the else is useless here.
Thanks, the second if and second else can be removed and will change in v3 patch set.

> 
> <...>
> 
> > +static void
> > +cmd_config_per_queue_rx_offload_parsed(void *parsed_result,
> > +				__attribute__((unused)) struct cmdline *cl,
> > +				__attribute__((unused)) void *data) {
> > +	struct cmd_config_per_queue_rx_offload_result *res = parsed_result;
> > +	struct rte_eth_dev_info dev_info;
> > +	portid_t port_id = res->port_id;
> > +	uint16_t queue_id = res->queue_id;
> > +	struct rte_port *port = &ports[port_id];
> > +	int on;
> > +
> > +	if (port->port_status != RTE_PORT_STOPPED) {
> > +		printf("Error: Can't config offload when Port %d "
> > +		       "is not stopped\n", port_id);
> > +		return;
> > +	}
> > +
> > +	if (!strcmp(res->en_dis, "enable"))
> > +		on = 1;
> > +	else if (!strcmp(res->en_dis, "disable"))
> > +		on = 0;
> > +	else {
> > +		printf("Unknown parameter: %s\n", res->en_dis);
> > +		return;
> Same comments as above.
> <...>
> 
> > @@ -707,6 +708,17 @@ init_config(void)
> >  			}
> >  		}
> >
> > +		port->rx_offloads = rte_zmalloc("testpmd: port->rx_offloads",
> > +						sizeof(port->rx_offloads[0]) *
> > +						port->dev_info.max_rx_queues,
> > +						sizeof(port->rx_offloads[0]));
> 
> When will you free it?
I will free port->rx_offloads and port->tx_offloads when testpmd is existed.
> 
> > +		if (port->rx_offloads == NULL)
> > +			rte_exit(EXIT_FAILURE,
> > +				 "rte_zmalloc(%d port->rx_offload[0]) "
> > +				 "failed\n", port->dev_info.max_rx_queues);
> > +		for (k = 0; k < port->dev_info.max_rx_queues; k++)
> > +			port->rx_offloads[k] = port->dev_conf.rxmode.offloads;
> > +
> In the get configure command, the port->rx_offloads is printed.
> Here you init it to be port configuration, when will you update it?
This init_config() is only run once in the initialization stage in the main().
Port->rx_offloads[] can be changed using new testpmd command in this patch set.
Testpmd> rx_offload enable/disable per_queue <offload> <port_id> <queue_id>
And will be applied when the command port start <port_id> is run.