Hi Kevin, Please find comments inline. > -----Original Message----- > From: Kevin Traynor [mailto:[email protected]] > Sent: Friday, October 12, 2018 4:51 PM > To: Ophir Munk <[email protected]>; [email protected] > Cc: Asaf Penso <[email protected]>; Sugesh Chandran > <[email protected]>; Ian Stokes <[email protected]>; Ben > Pfaff <[email protected]>; Shahaf Shuler <[email protected]>; Thomas > Monjalon <[email protected]>; Olga Shern <[email protected]> > Subject: Re: [dpdk-howl PATCH v5 1/2] netdev-dpdk: Upgrade to dpdk v18.08 > > > - > > - rss = xmalloc(sizeof(*rss) + sizeof(uint16_t) * netdev->n_rxq); > > - /* > > - * Setting it to NULL will let the driver use the default RSS > > - * configuration we have set: &port_conf.rx_adv_conf.rss_conf. > > - */ > > - rss->rss_conf = NULL; > > - rss->num = netdev->n_rxq; > > + struct action_rss_data *rss_data; > > + > > + rss_data = xmalloc(sizeof(struct action_rss_data) + > > + sizeof(uint16_t) * netdev->n_rxq); > > + *rss_data = (struct action_rss_data) { > > + .conf = (struct rte_flow_action_rss) { > > + .func = RTE_ETH_HASH_FUNCTION_DEFAULT, > > + .level = 0, > > + .types = ETH_RSS_IP, > > Elsewhere when rss types are set, they are masked against device info to > avoid a failure. Does that need to be done here ? or it is enough that, in > this > unlikely event, it may fail elsewhere (like rte_flow_create).
Actually since .func equals RTE_ETH_HASH_FUNCTION_DEFAULT I think we should assign .types = 0 then each device will know internally what are its default actual types. Will update in v6 > > @@ end_proto_check: > > > > flow = rte_flow_create(dev->port_id, &flow_attr, patterns.items, > > actions.actions, &error); > > - free(rss); > > + void *rss_cont; > > + rss_cont = container_of(rss, struct action_rss_data, conf); > > + free(rss_cont); > > I think it needs a comment to explain why you are doing this, as it takes a > bit > of digging into add_flow_rss_action() to figure out. > > Also, there is a CONTAINER_OF() in util.h used elsewhere in the file, so you > should probably use that. With a brief comment to explain what you are > doing perhaps the variable is not needed i.e. free(CONTAINER_OF(...)), but > it's up to you. Will update in v6 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
