On 09/05/2018 07:44 AM, Chandran, Sugesh wrote: > Hi Ophir, > > Thank you for working on this. > I tried to run some tests and getting some errors while adding physical ports > as below. I use Niantic NIC for this test > >>>>>>>>>>>>>>>> > 2018-09-05T06:34:58Z|00069|dpdk|INFO|Device with port_id=0 already stopped > 2018-09-05T06:34:58Z|00070|dpdk|ERR|Ethdev port_id=0 invalid rss_hf: 0x3afbc, > valid value: 0x38d34 > 2018-09-05T06:34:58Z|00071|netdev_dpdk|WARN|Interface dpdk0 eth_dev setup > error Invalid argument > 2018-09-05T06:34:58Z|00072|netdev_dpdk|ERR|Interface dpdk0(rxq:2 txq:2 lsc > interrupt mode:false) configure error: Invalid argument > 2018-09-05T06:34:58Z|00073|dpif_netdev|ERR|Failed to set interface dpdk0 new > configuration > 2018-09-05T06:34:58Z|00074|bridge|WARN|could not add network device dpdk0 to > ofproto (No such device) > ovs-vsctl: Error detected while setting up 'dpdk0': could not add network > device dpdk0 to ofproto (No such device). See ovs-vswitchd log for details. > ovs-vsctl: The default log directory is "/usr/local/var/log/openvswitch". >
I didn't have time to compare the below patch with the ones that I did, but I saw that this patch was needed for niantic https://github.com/kevintraynor/ovs-dpdk-master/commit/88f46cc5ab338eb4f3ca5db1eacd0effefe4fa0c >>>>>>>>>>>>>>>>> > Please find below for my comments. > > > Regards > _Sugesh > >> -----Original Message----- >> From: Ophir Munk [mailto:ophi...@mellanox.com] >> Sent: Monday, September 3, 2018 3:19 PM >> To: ovs-dev@openvswitch.org >> Cc: Asaf Penso <as...@mellanox.com>; Chandran, Sugesh >> <sugesh.chand...@intel.com>; Stokes, Ian <ian.sto...@intel.com>; Ben Pfaff >> <b...@ovn.org>; Shahaf Shuler <shah...@mellanox.com>; Thomas Monjalon >> <tho...@monjalon.net>; Olga Shern <ol...@mellanox.com>; Ophir Munk >> <ophi...@mellanox.com>; Kevin Traynor <ktray...@redhat.com> >> Subject: [dpdk-hwol PATCH v1] netdev-dpdk: Upgrade to dpdk v18.08.0 >> >> 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 to >> accommodate to the dpdk changes. >> - ce17eddefc20 ("ethdev: introduce Rx queue offloads API") >> - ab3ce1e0c193 ("ethdev: remove old offload API") >> - c06ddf9698e0 ("meter: add configuration profile") >> - e58638c32411 ("ethdev: fix TPID handling in flow API") >> - cd8c7c7ce241 ("ethdev: replace bus specific struct with generic dev") >> - ac8d22de2394 ("ethdev: flatten RSS configuration in flow API") >> >> 2. Update references to DPDK version in Documentation >> >> 3. Update DPDK version in travis' linux-build script >> >> Signed-off-by: Ophir Munk <ophi...@mellanox.com> >> --- >> .travis/linux-build.sh | 2 +- >> Documentation/faq/releases.rst | 2 +- >> lib/netdev-dpdk.c | 135 >> +++++++++++++++++++++++++++++------------ >> 3 files changed, 98 insertions(+), 41 deletions(-) >> >> diff --git a/.travis/linux-build.sh b/.travis/linux-build.sh index >> 4b9fc4a..c60ac71 >> 100755 >> --- a/.travis/linux-build.sh >> +++ b/.travis/linux-build.sh >> @@ -83,7 +83,7 @@ fi >> >> if [ "$DPDK" ]; then >> if [ -z "$DPDK_VER" ]; then >> - DPDK_VER="17.11.3" >> + DPDK_VER="18.08.0" >> fi >> install_dpdk $DPDK_VER >> if [ "$CC" = "clang" ]; then >> diff --git a/Documentation/faq/releases.rst b/Documentation/faq/releases.rst >> index 41d41e3..646ae09 100644 >> --- a/Documentation/faq/releases.rst >> +++ b/Documentation/faq/releases.rst >> @@ -168,7 +168,7 @@ Q: What DPDK version does each Open vSwitch release >> work with? >> 2.7.x 16.11.7 >> 2.8.x 17.05.2 >> 2.9.x 17.11.3 >> - 2.10.x 17.11.3 >> + 2.10.x 18.08.0 >> ============ ======= >> >> Q: Are all the DPDK releases that OVS versions work with maintained? >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index f91aa27..80d2af9 >> 100644 >> --- a/lib/netdev-dpdk.c >> +++ b/lib/netdev-dpdk.c >> @@ -168,11 +168,7 @@ static const struct rte_eth_conf port_conf = { >> .rxmode = { >> .mq_mode = ETH_MQ_RX_RSS, >> .split_hdr_size = 0, >> - .header_split = 0, /* Header Split disabled */ >> - .hw_ip_checksum = 0, /* IP checksum offload disabled */ >> - .hw_vlan_filter = 0, /* VLAN filtering disabled */ >> - .jumbo_frame = 0, /* Jumbo Frame Support disabled */ >> - .hw_strip_crc = 0, >> + .offloads = 0, >> }, >> .rx_adv_conf = { >> .rss_conf = { >> @@ -364,6 +360,7 @@ struct dpdk_ring { >> struct ingress_policer { >> struct rte_meter_srtcm_params app_srtcm_params; >> struct rte_meter_srtcm in_policer; >> + struct rte_meter_srtcm_profile in_prof; >> rte_spinlock_t policer_lock; >> }; >> >> @@ -903,16 +900,17 @@ dpdk_eth_dev_port_config(struct netdev_dpdk *dev, >> int n_rxq, int n_txq) >> if (dev->mtu > ETHER_MTU) { >> rte_eth_dev_info_get(dev->port_id, &info); >> if (strncmp(info.driver_name, "net_nfp", 7)) { >> - conf.rxmode.enable_scatter = 1; >> + conf.rxmode.offloads |= DEV_RX_OFFLOAD_SCATTER; >> } >> } >> >> conf.intr_conf.lsc = dev->lsc_interrupt_mode; >> - conf.rxmode.hw_ip_checksum = (dev->hw_ol_features & >> - NETDEV_RX_CHECKSUM_OFFLOAD) != 0; >> + conf.rxmode.offloads |= ((dev->hw_ol_features & >> + NETDEV_RX_CHECKSUM_OFFLOAD) != 0) ? >> + DEV_RX_OFFLOAD_CHECKSUM : 0; >> >> if (dev->hw_ol_features & NETDEV_RX_HW_CRC_STRIP) { >> - conf.rxmode.hw_strip_crc = 1; >> + conf.rxmode.offloads |= DEV_RX_OFFLOAD_CRC_STRIP; >> } >> >> /* A device may report more queues than it makes available (this has @@ >> - >> 1932,16 +1930,18 @@ netdev_dpdk_eth_tx_burst(struct netdev_dpdk *dev, int >> qid, >> >> static inline bool >> netdev_dpdk_policer_pkt_handle(struct rte_meter_srtcm *meter, >> + struct rte_meter_srtcm_profile *profile, >> struct rte_mbuf *pkt, uint64_t time) { >> uint32_t pkt_len = rte_pktmbuf_pkt_len(pkt) - sizeof(struct ether_hdr); >> >> - return rte_meter_srtcm_color_blind_check(meter, time, pkt_len) == >> + return rte_meter_srtcm_color_blind_check(meter, profile, time, >> + pkt_len) == >> e_RTE_METER_GREEN; } >> >> static int >> netdev_dpdk_policer_run(struct rte_meter_srtcm *meter, >> + struct rte_meter_srtcm_profile *profile, >> struct rte_mbuf **pkts, int pkt_cnt, >> bool should_steal) { @@ -1953,7 +1953,8 @@ >> netdev_dpdk_policer_run(struct rte_meter_srtcm *meter, >> for (i = 0; i < pkt_cnt; i++) { >> pkt = pkts[i]; >> /* Handle current packet */ >> - if (netdev_dpdk_policer_pkt_handle(meter, pkt, current_time)) { >> + if (netdev_dpdk_policer_pkt_handle(meter, profile, pkt, >> + current_time)) { >> if (cnt != i) { >> pkts[cnt] = pkt; >> } >> @@ -1975,8 +1976,8 @@ ingress_policer_run(struct ingress_policer *policer, >> struct rte_mbuf **pkts, >> int cnt = 0; >> >> rte_spinlock_lock(&policer->policer_lock); >> - cnt = netdev_dpdk_policer_run(&policer->in_policer, pkts, >> - pkt_cnt, should_steal); >> + cnt = netdev_dpdk_policer_run(&policer->in_policer, &policer->in_prof, >> + pkts, pkt_cnt, should_steal); >> rte_spinlock_unlock(&policer->policer_lock); >> >> return cnt; >> @@ -2767,8 +2768,15 @@ netdev_dpdk_policer_construct(uint32_t rate, >> uint32_t burst) >> policer->app_srtcm_params.cir = rate_bytes; >> policer->app_srtcm_params.cbs = burst_bytes; >> policer->app_srtcm_params.ebs = 0; >> + err = rte_meter_srtcm_profile_config(&policer->in_prof, >> + &policer->app_srtcm_params); >> + if (err) { >> + VLOG_ERR("Could not create rte meter profile for ingress policer"); >> + free(policer); >> + return NULL; >> + } >> err = rte_meter_srtcm_config(&policer->in_policer, >> - &policer->app_srtcm_params); >> + &policer->in_prof); >> if (err) { >> VLOG_ERR("Could not create rte meter for ingress policer"); >> free(policer); >> @@ -3043,13 +3051,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; >> + 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%u", >> + pci_dev->id.vendor_id); >> + smap_add_format(args, "pci-device_id", "0x%x", >> + pci_dev->id.device_id); >> + } >> } >> - >> return 0; >> } >> >> @@ -3727,6 +3740,7 @@ struct egress_policer { >> struct qos_conf qos_conf; >> struct rte_meter_srtcm_params app_srtcm_params; >> struct rte_meter_srtcm egress_meter; >> + struct rte_meter_srtcm_profile egress_prof; >> }; >> >> static void >> @@ -3749,8 +3763,16 @@ egress_policer_qos_construct(const struct smap >> *details, >> policer = xmalloc(sizeof *policer); >> qos_conf_init(&policer->qos_conf, &egress_policer_ops); >> egress_policer_details_to_param(details, &policer->app_srtcm_params); >> + err = rte_meter_srtcm_profile_config(&policer->egress_prof, >> + &policer->app_srtcm_params); >> + if (err) { > [Sugesh] Should we error out the message on failing the profile config?? >> + free(policer); >> + *conf = NULL; >> + return -err; >> + } >> err = rte_meter_srtcm_config(&policer->egress_meter, >> - &policer->app_srtcm_params); >> + &policer->egress_prof); >> + >> if (!err) { >> *conf = &policer->qos_conf; >> } else { >> @@ -3803,7 +3825,8 @@ egress_policer_run(struct qos_conf *conf, struct >> rte_mbuf **pkts, int pkt_cnt, >> struct egress_policer *policer = >> CONTAINER_OF(conf, struct egress_policer, qos_conf); >> >> - cnt = netdev_dpdk_policer_run(&policer->egress_meter, pkts, >> + cnt = netdev_dpdk_policer_run(&policer->egress_meter, >> + &policer->egress_prof, pkts, >> pkt_cnt, should_steal); >> >> return cnt; >> @@ -4103,15 +4126,15 @@ 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); >> } else { >> VLOG_DBG(" Mask = null\n"); >> } >> @@ -4281,27 +4304,56 @@ add_flow_action(struct flow_actions *actions, >> enum rte_flow_action_type type, >> actions->cnt++; >> } >> >> +/* >> + * Storage for struct rte_flow_action_rss >> + * including storage for key and queue array */ #define >> +MAX_RSS_HASH_KEY_LENGTH 128 #define MAX_ACTION_RSS_QUEUE_NUM >> 128 >> +struct action_rss_data { >> + struct rte_flow_action_rss conf; >> + uint16_t queue[MAX_ACTION_RSS_QUEUE_NUM]; >> + uint8_t key[MAX_RSS_HASH_KEY_LENGTH]; }; >> + >> static struct rte_flow_action_rss * >> add_flow_rss_action(struct flow_actions *actions, >> struct netdev *netdev) { >> int i; >> - struct rte_flow_action_rss *rss; >> + struct action_rss_data *rss_data; >> >> - 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; >> + if (netdev->n_rxq > MAX_ACTION_RSS_QUEUE_NUM) { >> + VLOG_ERR("Num of rxq (%u) must not be greater " \ >> + "than max rss num of queues (%u)", >> + netdev->n_rxq, MAX_ACTION_RSS_QUEUE_NUM); >> + return NULL; >> + } >> >> - 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){ >> + .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 = rss_data->key, >> + }, >> + .key = { 0 }, >> + .queue = { 0 }, >> + }; >> + >> + /* TODO: Override key with default */ > [Sugesh] Do we really need this TODO?? >> + >> + /* Override queue array with default */ >> + for (i = 0; i < rss_data->conf.queue_num; i++) { >> + rss_data->queue[i] = i; >> } >> >> - add_flow_action(actions, RTE_FLOW_ACTION_TYPE_RSS, rss); >> + add_flow_action(actions, RTE_FLOW_ACTION_TYPE_RSS, >> + &rss_data->conf); >> >> - return rss; >> + return &rss_data->conf; >> } >> >> static int >> @@ -4365,7 +4417,7 @@ netdev_dpdk_add_rte_flow_offload(struct netdev >> *netdev, >> vlan_mask.tci = match->wc.masks.vlans[0].tci & ~htons(VLAN_CFI); >> >> /* match any protocols */ >> - vlan_mask.tpid = 0; >> + vlan_mask.inner_type = 0; >> >> add_flow_pattern(&patterns, RTE_FLOW_ITEM_TYPE_VLAN, >> &vlan_spec, &vlan_mask); @@ -4516,6 +4568,11 @@ >> end_proto_check: >> >> struct rte_flow_action_rss *rss; >> rss = add_flow_rss_action(&actions, netdev); >> + if (!rss) { >> + VLOG_ERR("add_flow_rss_action error.\n"); > I feel this error can be bit more verbose something like Failed to add > rss_flow+actions or something?? What do you think? >> + ret = -1; >> + goto out; >> + } >> add_flow_action(&actions, RTE_FLOW_ACTION_TYPE_END, NULL); >> >> flow = rte_flow_create(dev->port_id, &flow_attr, patterns.items, >> -- >> 1.8.3.1 > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
