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
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
