On 04/10/2017 04:31 PM, Chandran, Sugesh wrote: > Thank you for the patch, > Please see the comment below. > > Regards > _Sugesh > >> -----Original Message----- >> From: Kevin Traynor [mailto:[email protected]] >> Sent: Tuesday, April 4, 2017 6:35 AM >> To: [email protected] >> Cc: Kevin Traynor <[email protected]>; Chandran, Sugesh >> <[email protected]> >> Subject: [PATCH 1/2] netdev-dpdk: Fix Rx checksum reconfigure. >> >> Rx checksum offload is enabled by default where available, with >> reconfiguration through OVSDB options:rx-checksum-offload. >> However, setting rx-checksum-offload does not result in a reconfiguration of >> the NIC. >> >> Fix that by checking if the requested port config features (e.g. rx checksum >> offload) are currently applied on the NIC and if not, perform a >> reconfiguration. >> >> Fixes: 1a2bb11817a4 ("netdev-dpdk: Enable Rx checksum offloading feature >> on DPDK physical ports.") >> Cc: Sugesh Chandran <[email protected]> >> Signed-off-by: Kevin Traynor <[email protected]> >> --- >> lib/netdev-dpdk.c | 14 +++++++++----- >> 1 file changed, 9 insertions(+), 5 deletions(-) >> >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index ddc651b..d5a9800 >> 100644 >> --- a/lib/netdev-dpdk.c >> +++ b/lib/netdev-dpdk.c >> @@ -374,4 +374,5 @@ struct netdev_dpdk { >> int requested_rxq_size; >> int requested_txq_size; >> + uint32_t requested_hwol; >> >> /* Number of rx/tx descriptors for physical devices */ @@ -647,5 +648,5 >> @@ dpdk_eth_dev_queue_setup(struct netdev_dpdk *dev, int n_rxq, int >> n_txq) >> conf.rxmode.max_rx_pkt_len = 0; >> } >> - conf.rxmode.hw_ip_checksum = (dev->hw_ol_features & >> + conf.rxmode.hw_ip_checksum = (dev->requested_hwol & >> NETDEV_RX_CHECKSUM_OFFLOAD) != 0; >> /* A device may report more queues than it makes available (this has @@ >> -701,4 +702,5 @@ dpdk_eth_dev_queue_setup(struct netdev_dpdk *dev, >> int n_rxq, int n_txq) >> dev->up.n_rxq = n_rxq; >> dev->up.n_txq = n_txq; >> + dev->hw_ol_features = dev->requested_hwol; >> >> return 0; >> @@ -718,5 +720,5 @@ dpdk_eth_checksum_offload_configure(struct >> netdev_dpdk *dev) >> DEV_RX_OFFLOAD_IPV4_CKSUM; >> rte_eth_dev_info_get(dev->port_id, &info); >> - rx_csum_ol_flag = (dev->hw_ol_features & >> NETDEV_RX_CHECKSUM_OFFLOAD) != 0; >> + rx_csum_ol_flag = (dev->requested_hwol & >> + NETDEV_RX_CHECKSUM_OFFLOAD) != 0; >> >> if (rx_csum_ol_flag && >> @@ -725,5 +727,5 @@ dpdk_eth_checksum_offload_configure(struct >> netdev_dpdk *dev) >> VLOG_WARN_ONCE("Rx checksum offload is not supported on device >> %d", >> dev->port_id); >> - dev->hw_ol_features &= ~NETDEV_RX_CHECKSUM_OFFLOAD; >> + dev->requested_hwol &= ~NETDEV_RX_CHECKSUM_OFFLOAD; >> return; >> } >> @@ -871,4 +873,5 @@ common_construct(struct netdev *netdev, unsigned >> int port_no, >> /* Initilize the hardware offload flags to 0 */ >> dev->hw_ol_features = 0; >> + dev->requested_hwol = 0; >> >> dev->flags = NETDEV_UP | NETDEV_PROMISC; @@ -1259,5 +1262,5 @@ >> netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args, >> != 0; >> if (temp_flag != rx_chksm_ofld) { >> - dev->hw_ol_features ^= NETDEV_RX_CHECKSUM_OFFLOAD; >> + dev->requested_hwol ^= NETDEV_RX_CHECKSUM_OFFLOAD; >> dpdk_eth_checksum_offload_configure(dev); >> } >> @@ -3124,5 +3127,6 @@ netdev_dpdk_reconfigure(struct netdev *netdev) >> && dev->rxq_size == dev->requested_rxq_size >> && dev->txq_size == dev->requested_txq_size >> - && dev->socket_id == dev->requested_socket_id) { >> + && dev->socket_id == dev->requested_socket_id >> + && dev->hw_ol_features == dev->requested_hwol) { >> /* Reconfiguration is unnecessary */ > [Sugesh] The patch fixes the issue however I believe these checks has to be > moved to the corresponding functions than here. May be that looks cleaner. > Any configuration change that happen to a netdev validated at the > corresponding functions than in a common reconfigure. That can avoid the need > of 2 struct element for every param. > I verified the patch and now it reloads the NIC configuration when there is a > change.
Hi Sugesh, Not sure I totally follow you, I think this fixes the issue close to the original patch and the current reconfigure approach. Are you suggesting that the reconfigure code should be re-written to remove the requested_*'s before this bug fix? or a different fix for this specific issue? Maybe you could add comment at the LOC you think should change so I understand. thanks, Kevin. > > > >> >> -- >> 1.8.3.1 > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
