[ovs-dev] [dpdk-howl PATCH v3] netdev-dpdk: Upgrade to dpdk v18.08

[Ophir Munk]

Hi Ilya,
Please find comment inline

> 
> On 11.09.2018 02:04, Ophir Munk wrote:
> > 1. Enable compilation and linkage with dpdk 18.08.0 The following dpdk
> > commits which were introduced after dpdk 17.11.x require OVS updates
> > +    conf.rxmode.offloads |= ((dev->hw_ol_features &
> > +                             NETDEV_RX_CHECKSUM_OFFLOAD) != 0) ?
> > +                             DEV_RX_OFFLOAD_CHECKSUM : 0;
> 
> IMHO, it's better to use 'if'. This thing became too complex.

Done in v5

> 
> >
> > +        if (pci_dev) {
> > +            smap_add_format(args, "pci-vendor_id", "0x%u",
> > +                            pci_dev->id.vendor_id);
> > +             smap_add_format(args, "pci-device_id", "0x%x",
> > +                             pci_dev->id.device_id);
> 
> %u --> %x.
> Also, something happened with indents.
> 

Fixed in v5

> > -    for (i = 0; i < rss->num; i++) {
> > -        rss->queue[i] = i;
> > +    rss_data = xmalloc(sizeof(struct action_rss_data));
> > +    *rss_data = (struct action_rss_data){
> > +        .conf = (struct rte_flow_action_rss){
> 
> Some spaces needed between the type and '{'.
> 

Fixed in v5

> 
> It's better to use 'xzalloc' instead of manual memory initialization.
> 

I have allocated the exact space required for the number of queues using xmalloc (please see v5). No need to initialize to 0.

> >
> > -    return rss;
> > +    return &rss_data->conf;
> 

> Pointer to the field of the structure returned. 'free' will be invoked for it but
> not for the pointer to the structure. This should not be like this even if this
> field is the first one.
> 

Fixed using container_of (please see v5)

> Maybe it's better to not have the additional structure and allocate arrays
> explicitly? The caller will free rss->queue, rss->key and the rss itself at the
> end. This will also remove the restriction for the number of queues.
> 

The required number of queues is allocated by xmalloc, later they are free (please see v5)

Regards,
Ophir