[ovs-dev] [dpdk-howl PATCH v5 1/2] netdev-dpdk: Upgrade to dpdk v18.08

Ophir Munk ophirmu at mellanox.com
Thu Oct 25 13:17:53 UTC 2018


Hi Kevin,
Please find comments inline.

> -----Original Message-----
> From: Kevin Traynor [mailto:ktraynor at redhat.com]
> Sent: Friday, October 12, 2018 4:51 PM
> To: Ophir Munk <ophirmu at mellanox.com>; ovs-dev at openvswitch.org
> Cc: Asaf Penso <asafp at mellanox.com>; Sugesh Chandran
> <sugesh.chandran at intel.com>; Ian Stokes <ian.stokes at intel.com>; Ben
> Pfaff <blp at ovn.org>; Shahaf Shuler <shahafs at mellanox.com>; Thomas
> Monjalon <thomas at monjalon.net>; Olga Shern <olgas at 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



More information about the dev mailing list