Regards _Sugesh
> -----Original Message----- > From: Kevin Traynor [mailto:ktray...@redhat.com] > Sent: Tuesday, April 25, 2017 6:40 PM > To: Chandran, Sugesh <sugesh.chand...@intel.com>; d...@openvswitch.org > Subject: Re: [PATCH 1/2] netdev-dpdk: Fix Rx checksum reconfigure. > > 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:ktray...@redhat.com] > >> Sent: Tuesday, April 4, 2017 6:35 AM > >> To: d...@openvswitch.org > >> Cc: Kevin Traynor <ktray...@redhat.com>; Chandran, Sugesh > >> <sugesh.chand...@intel.com> > >> 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 <sugesh.chand...@intel.com> > >> Signed-off-by: Kevin Traynor <ktray...@redhat.com> > >> --- > >> 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 [Sugesh] yes, it looks clean that way. > before this bug fix? or a different fix for this specific issue? Maybe you > could [Sugesh] I would say to go ahead with this commit for fixing rx-checksum configuration issue. Refactoring of reconfigure can be done as a different fix. > add comment at the LOC you think should change so I understand. > > thanks, > Kevin. > > > > > > > > >> > >> -- > >> 1.8.3.1 > > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
