Hi Eelco,
Please find comments inline
> -----Original Message-----
> From: Eelco Chaudron [mailto:[email protected]]
> Sent: Wednesday, October 24, 2018 1:41 PM
> To: Ophir Munk <[email protected]>
> Cc: Thomas Monjalon <[email protected]>; [email protected];
> Asaf Penso <[email protected]>; Shahaf Shuler
> <[email protected]>
> Subject: Re: [ovs-dev] [dpdk-howl PATCH v5 1/2] netdev-dpdk: Upgrade to
> dpdk v18.08
>
> Hi Ophir,
>
> Did not see any response on my comments below, is this another mailing list
> issue you explained?
>
V6 is expected soon. There is no mailing list issue.
> //Eelco
>
> On 12 Oct 2018, at 10:56, Eelco Chaudron wrote:
>
> >> @@ -3043,13 +3054,18 @@ netdev_dpdk_get_status(const struct netdev
> >> *netdev, struct smap *args)
> >> smap_add_format(args, "if_descr", "%s %s", rte_version(),
> >>
> >> dev_info.driver_name);
> >>
> >> - if (dev_info.pci_dev) {
> >> - smap_add_format(args, "pci-vendor_id", "0x%x",
> >> - dev_info.pci_dev->id.vendor_id);
> >> - smap_add_format(args, "pci-device_id", "0x%x",
> >> - dev_info.pci_dev->id.device_id);
> >> + const struct rte_bus *bus;
> >> + const struct rte_pci_device *pci_dev;
> >
> > Don’t we need to take the ovs_mutex_lock(&dev->mutex) lock here, we
> > are calling DPDK code?
There is no dev access in the added code. Therefore should use dpdk_mutex rather
than dev->mutex.
Will update in v6
> >
> >> + bus = rte_bus_find_by_device(dev_info.device);
> >> + if (bus && !strcmp(bus->name, "pci")) {
> >> + pci_dev = RTE_DEV_TO_PCI(dev_info.device);
> >> + if (pci_dev) {
> >> + smap_add_format(args, "pci-vendor_id", "0x%x",
> >> + pci_dev->id.vendor_id);
> >> + smap_add_format(args, "pci-device_id", "0x%x",
> >> + pci_dev->id.device_id);
> >> + }
> >> }
> >> -
> >> return 0;
> >> }
> >>
> >> dump_flow_pattern(struct rte_flow_item *item)
> >>
> >> VLOG_DBG("rte flow vlan pattern:\n");
> >> if (vlan_spec) {
> >> - VLOG_DBG(" Spec: tpid=0x%"PRIx16", tci=0x%"PRIx16"\n",
> >> - ntohs(vlan_spec->tpid), ntohs(vlan_spec->tci));
> >> + VLOG_DBG(" Spec: inner_type=0x%"PRIx16",
> >> tci=0x%"PRIx16"\n",
> >> + ntohs(vlan_spec->inner_type),
> >> ntohs(vlan_spec->tci));
> >> } else {
> >> VLOG_DBG(" Spec = null\n");
> >> }
> >>
> >> if (vlan_mask) {
> >> - VLOG_DBG(" Mask: tpid=0x%"PRIx16", tci=0x%"PRIx16"\n",
> >> - vlan_mask->tpid, vlan_mask->tci);
> >> + VLOG_DBG(" Mask: inner_type=0x%"PRIx16",
> >> tci=0x%"PRIx16"\n",
> >> + vlan_mask->inner_type, vlan_mask->tci);
> >
> > Should the vlan_mask also use htons()?
> >
It seems so as both vlan_spec and vlan_mask are of the same type and have Big
Endian fields .
This patch only renamed the field tpid ==> inner_type so not using htons() was
already present in 17.11.
Will update in v6.
> >> } else {
> >> VLOG_DBG(" Mask = null\n");
> >> }
> >> +
> >> + 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,
> >> + .key_len = 0,
> >> + .queue_num = netdev->n_rxq,
> >> + .queue = rss_data->queue,
> >> + .key = NULL
> >
> > If you have them in a different order than the structure, you might as
> > well group key_len and key together.
> >> + },
> >> + };
> >>
Agreed. Will group key_len and key in v6
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
