Hi Wenxu, Thank you for the patch. I had reviewed the patch and I have same comment as Zhenyu.
As you said there are so many other configs. Its not a good practice to offer configuration choices for some of them. I don’t think user see any difference in performance/functionality either its enabled/disabled. My understanding is, this will let user to reconfigure the port again with hw_strip_crc when they see a failure. Instead of having an option for user. How about check the support first and enable/disable accordingly. I think DPDK doesn’t provide this information in its rte_eth_dev_info now. However its worth to check the possibility in DPDK to report hw_crc_strip feature support with other flags. It looks similar to how rx checksum offload is being handled in OVS-DPDK. Also if all the DPDK NICs can support the hw_crc_strip, it make sense to keep it enable as a default choice. What do you think? Regards _Sugesh > -----Original Message----- > From: ovs-dev-boun...@openvswitch.org [mailto:ovs-dev- > boun...@openvswitch.org] On Behalf Of wenxu > Sent: Tuesday, October 10, 2017 3:33 AM > To: Gao Zhenyu <sysugaozhe...@gmail.com> > Cc: ovs dev <d...@openvswitch.org> > Subject: Re: [ovs-dev] [PATCH] netdev-dpdk: New option 'hw_strip_crc' fields > for DPDK interfaces > > Hi Zhenyu, > > > I think add a option maybe better. > > > 1. It keep unchange the action with default value is false 2. It can be try > one > more time but if there some other different config, there should be more times > to try 3. For some user who want to enable this feature, it can be > configurable > > > > > 在 2017-10-09 21:13:12,"Gao Zhenyu" <sysugaozhe...@gmail.com> 写道: > > Thanks for working on it. > > > I want to know if we can have a patch to try one more time if we hit the > hw_strip_crc error since we had introduce many options in ovs-dpdk now. (Just > like function dpdk_eth_dev_queue_setup, it figures out queue number by > retrying) > > > Thanks > Zhenyu Gao > > > 2017-10-09 17:09 GMT+08:00 <we...@ucloud.cn>: > From: wenxu <we...@ucloud.cn> > > Some vf driver like i40evf can't disable the hw_strip_crc, with err > 'dpdk|ERR|i40evf_dev_configure(): VF can't disable HW CRC Strip' > so hw_strip_crc feature should can be configurable > > Signed-off-by: wenxu <we...@ucloud.cn> > --- > NEWS | 2 ++ > lib/netdev-dpdk.c | 30 ++++++++++++++++++++++++++++++ > vswitchd/vswitch.xml | 9 +++++++++ > 3 files changed, 41 insertions(+) > > diff --git a/NEWS b/NEWS > index a3c1a02..d913847 100644 > --- a/NEWS > +++ b/NEWS > @@ -5,6 +5,8 @@ Post-v2.8.0 > chassis "hostname" in addition to a chassis "name". > - Linux kernel 4.13 > * Add support for compiling OVS with the latest Linux 4.13 kernel > + - DPDK: > + * New option 'hw_strip_crc' fields for DPDK interfaces > > v2.8.0 - xx xxx xxxx > --------------------- > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index c60f46f..23c2a72 > 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -402,11 +402,15 @@ struct netdev_dpdk { > int requested_n_rxq; > int requested_rxq_size; > int requested_txq_size; > + bool requested_hw_strip_crc; > > /* Number of rx/tx descriptors for physical devices */ > int rxq_size; > int txq_size; > > + /*crc strip feature config*/ > + bool hw_strip_crc; > + > /* Socket ID detected when vHost device is brought up */ > int requested_socket_id; > > @@ -700,6 +704,7 @@ dpdk_eth_dev_queue_setup(struct netdev_dpdk *dev, > int n_rxq, int n_txq) > > conf.rxmode.hw_ip_checksum = (dev->hw_ol_features & > NETDEV_RX_CHECKSUM_OFFLOAD) != 0; > + conf.rxmode.hw_strip_crc = dev->hw_strip_crc; > /* A device may report more queues than it makes available (this has > * been observed for Intel xl710, which reserves some of them for > * SRIOV): rte_eth_*_queue_setup will fail if a queue is not @@ -914,6 > +919,8 @@ common_construct(struct netdev *netdev, dpdk_port_t port_no, > dev->requested_n_txq = NR_QUEUE; > dev->requested_rxq_size = NIC_PORT_DEFAULT_RXQ_SIZE; > dev->requested_txq_size = NIC_PORT_DEFAULT_TXQ_SIZE; > + dev->requested_hw_strip_crc = false; > + dev->hw_strip_crc = false; > > /* Initialize the flow control to NULL */ > memset(&dev->fc_conf, 0, sizeof dev->fc_conf); @@ -1192,6 +1199,11 @@ > netdev_dpdk_get_config(const struct netdev *netdev, struct smap *args) > } else { > smap_add(args, "rx_csum_offload", "false"); > } > + if (dev->hw_strip_crc) { > + smap_add(args, "hw_strip_crc", "true"); > + } else { > + smap_add(args, "hw_strip_crc", "false"); > + } > } > ovs_mutex_unlock(&dev->mutex); > > @@ -1255,6 +1267,19 @@ dpdk_set_rxq_config(struct netdev_dpdk *dev, > const struct smap *args) } > > static void > +dpdk_set_hw_strip_crc_config(struct netdev_dpdk *dev, const struct smap > *args) > + OVS_REQUIRES(dev->mutex) > +{ > + int new_hw_strip_crc; > + > + new_hw_strip_crc = smap_get_bool(args, "hw_strip_crc", false); > + if (new_hw_strip_crc != dev->requested_hw_strip_crc) { > + dev->requested_hw_strip_crc = new_hw_strip_crc; > + netdev_request_reconfigure(&dev->up); > + } > +} > + > +static void > dpdk_process_queue_size(struct netdev *netdev, const struct smap *args, > const char *flag, int default_size, int *new_size) > { @@ -1290,6 > +1315,8 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap > *args, > > dpdk_set_rxq_config(dev, args); > > + dpdk_set_hw_strip_crc_config(dev, args); > + > dpdk_process_queue_size(netdev, args, "n_rxq_desc", > NIC_PORT_DEFAULT_RXQ_SIZE, > &dev->requested_rxq_size); @@ -3196,6 +3223,7 @@ > netdev_dpdk_reconfigure(struct netdev *netdev) > if (netdev->n_txq == dev->requested_n_txq > && netdev->n_rxq == dev->requested_n_rxq > && dev->mtu == dev->requested_mtu > + && dev->hw_strip_crc == dev->requested_hw_strip_crc > && dev->rxq_size == dev->requested_rxq_size > && dev->txq_size == dev->requested_txq_size > && dev->socket_id == dev->requested_socket_id) { @@ -3217,6 +3245,8 > @@ netdev_dpdk_reconfigure(struct netdev *netdev) > dev->rxq_size = dev->requested_rxq_size; > dev->txq_size = dev->requested_txq_size; > > + dev->hw_strip_crc = dev->requested_hw_strip_crc; > + > rte_free(dev->tx_q); > err = dpdk_eth_dev_init(dev); > dev->tx_q = netdev_dpdk_alloc_txq(netdev->n_txq); > diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml index > 074535b..d1e7a6b 100644 > --- a/vswitchd/vswitch.xml > +++ b/vswitchd/vswitch.xml > @@ -2589,6 +2589,15 @@ > </p> > </column> > > + <column name="options" key="hw_strip_crc" > + type='{"type": "bool"}'> > + <p> > + Specifies DPDK interfaces whether enable hw_strip_crc feature or > not. > + If not specified, the default is false. > + Not supported by DPDK vHost interfaces. > + </p> > + </column> > + > <column name="other_config" key="pmd-rxq-affinity"> > <p>Specifies mapping of RX queues of this interface to CPU cores.</p> > <p>Value should be set in the following form:</p> > -- > 1.8.3.1 > > > _______________________________________________ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev