> -----Original Message-----
> From: Wu, Jingjing
> Sent: Thursday, March 15, 2018 3:41 AM
> To: Dai, Wei <wei....@intel.com>; Lu, Wenzhuo <wenzhuo...@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.
