Hi Kevin, Please find comments inline. > -----Original Message----- > From: Kevin Traynor [mailto:ktray...@redhat.com] > Sent: Friday, October 12, 2018 4:51 PM > To: Ophir Munk <ophi...@mellanox.com>; ovs-dev@openvswitch.org > Cc: Asaf Penso <as...@mellanox.com>; Sugesh Chandran > <sugesh.chand...@intel.com>; Ian Stokes <ian.sto...@intel.com>; Ben > Pfaff <b...@ovn.org>; Shahaf Shuler <shah...@mellanox.com>; Thomas > Monjalon <tho...@monjalon.net>; Olga Shern <ol...@mellanox.com> > 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 d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
