Re: [ovs-dev] [RFC PATCHv2] netdev-dpdk: Enable Rx checksum offloading feature on DPDK physical ports.
On Mon, Aug 22, 2016 at 6:38 AM, Chandran, Sugesh wrote: >> -Original Message- >> From: Jesse Gross [mailto:[email protected]] >> Sent: Saturday, August 20, 2016 2:07 AM >> To: Chandran, Sugesh >> Cc: ovs dev >> Subject: Re: [RFC PATCHv2] netdev-dpdk: Enable Rx checksum offloading >> feature on DPDK physical ports. >> In the future, if DPDK offloads become tunnel aware some of these issues >> can become quite complicated due to the presence of multiple checksums >> being offloaded and how that is represented. On the Linux side there was a >> fair amount of work put into this over the past several years, so it might be >> worth taking a look at that for some inspiration before the DPDK interfaces >> get locked down. > [Sugesh] May be in the future when DPDK starts supporting checksum offload for > inner packets, Its worth to define it as 'csum_level' with > CHECKSUM_UNNECESSARY than defining separate flags > for every packet type, which is similar to what kernel is doing for tunnel > packet checksum offloading. The checksum reset & > validation logic must take care of this by changing the csum_level value and > CHECKSUM_* flag accordingly. > > > However the current DPDK driver only supports IP and L4 checksum offloading > using independent flags. > Will provide the inputs when other checksum offloading features are > implementing in DPDK in the future. Yes, I don't think that there is anything that can be or needs to be done on the OVS side at this point. The main reason why I mentioned it is to see if there is anything that we should do on the DPDK API side now to reduce possible API incompatibilities in the future if changes are necessary. ___ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [RFC PATCHv2] netdev-dpdk: Enable Rx checksum offloading feature on DPDK physical ports.
Regards _Sugesh > -Original Message- > From: Jesse Gross [mailto:[email protected]] > Sent: Saturday, August 20, 2016 2:07 AM > To: Chandran, Sugesh > Cc: ovs dev > Subject: Re: [RFC PATCHv2] netdev-dpdk: Enable Rx checksum offloading > feature on DPDK physical ports. > > On Fri, Aug 19, 2016 at 3:40 AM, Sugesh Chandran > wrote: > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index > > e5f2cdd..113e6d8 100644 > > --- a/lib/netdev-dpdk.c > > +++ b/lib/netdev-dpdk.c > > @@ -1090,6 +1127,15 @@ netdev_dpdk_set_config(struct netdev > *netdev, > > const struct smap *args) > > > > dpdk_eth_flow_ctrl_setup(dev); > > > > +/* Rx checksum offload configuration */ > > +/* By default the Rx checksum offload is ON */ > > +rx_chksm_ofld = smap_get_bool(args, "rx-checksum-offload", true); > > +temp_flag = (dev->hw_ol_features & > NETDEV_RX_CHECKSUM_OFFLOAD) > > +!= 0; > > +if (temp_flag != rx_chksm_ofld) { > > +dev->hw_ol_features ^= NETDEV_RX_CHECKSUM_OFFLOAD; > > +} > > +dpdk_eth_checksum_offload_configure(dev); > > ovs_mutex_unlock(&dev->mutex); > > I think that while we are going through the trouble of checking whether the > old flag is different from the new one, we might as well only call > dpdk_eth_checksum_offload_configure() if there is a difference. [Sugesh] Sure, Will move in the ' dpdk_eth_checksum_offload_configure(dev)' inside the if statement. > > > diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c index > > ce2582f..23e987c 100644 > > --- a/lib/netdev-native-tnl.c > > +++ b/lib/netdev-native-tnl.c > > @@ -85,9 +85,11 @@ netdev_tnl_ip_extract_tnl_md(struct dp_packet > > *packet, struct flow_tnl *tnl, > > > > ovs_be32 ip_src, ip_dst; > > > > -if (csum(ip, IP_IHL(ip->ip_ihl_ver) * 4)) { > > -VLOG_WARN_RL(&err_rl, "ip packet has invalid checksum"); > > -return NULL; > > +if(OVS_UNLIKELY(!dp_packet_ip_checksum_valid(packet))) { > > +if (csum(ip, IP_IHL(ip->ip_ihl_ver) * 4)) { > > +VLOG_WARN_RL(&err_rl, "ip packet has invalid checksum"); > > +return NULL; > > +} > > } > > > > if (ntohs(ip->ip_tot_len) > l3_size) { @@ -179,18 +181,20 @@ > > udp_extract_tnl_md(struct dp_packet *packet, struct flow_tnl *tnl, > > } > > > > if (udp->udp_csum) { > > -uint32_t csum; > > -if (netdev_tnl_is_header_ipv6(dp_packet_data(packet))) { > > -csum = packet_csum_pseudoheader6(dp_packet_l3(packet)); > > -} else { > > -csum = packet_csum_pseudoheader(dp_packet_l3(packet)); > > -} > > - > > -csum = csum_continue(csum, udp, dp_packet_size(packet) - > > - ((const unsigned char *)udp - > > - (const unsigned char > > *)dp_packet_l2(packet))); > > -if (csum_finish(csum)) { > > -return NULL; > > +if(OVS_UNLIKELY(!dp_packet_l4_checksum_valid(packet))) { > > +uint32_t csum; > > +if (netdev_tnl_is_header_ipv6(dp_packet_data(packet))) { > > +csum = packet_csum_pseudoheader6(dp_packet_l3(packet)); > > +} else { > > +csum = packet_csum_pseudoheader(dp_packet_l3(packet)); > > +} > > + > > +csum = csum_continue(csum, udp, dp_packet_size(packet) - > > + ((const unsigned char *)udp - > > + (const unsigned char > > *)dp_packet_l2(packet))); > > +if (csum_finish(csum)) { > > +return NULL; > > +} > > } > > tnl->flags |= FLOW_TNL_F_CSUM; > > } > > I think the previous version of the patch cleared the offload flags after > popping off the tunnel headers. I think that's probably a good idea here as > well. Since we're not terminating the payload it's probably relatively rare > that > we would have multiple checksums but it's theoretically possible that we > might have a tunnel-in-tunnel situation (and I suppose that it's that we might > want to use checksum offload for higher level services - connection > tracking/NAT/load balancing, etc.). > [Sugesh] Yes . it make sense , in fact the old patch was doing that. Will reset the flags after the use. > In the future, if DPDK offloads become tunnel aware some of these issues > can become quite complicated due to the presence of multiple checksums > being offloaded and how that is represented. On the Linux side there was a > fair amount of work put into this over the past several years, so it might be > worth taking a look at that for some inspiration before the DPDK interfaces > get locked down. [Sugesh] May be in the future when DPDK starts supporting checksum offload for inner packets, Its worth to define it as 'csum_level' with CHECKSUM_UNNECESSARY than defining separate flags for every packet type, whi
Re: [ovs-dev] [RFC PATCHv2] netdev-dpdk: Enable Rx checksum offloading feature on DPDK physical ports.
On Fri, Aug 19, 2016 at 3:40 AM, Sugesh Chandran
wrote:
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index e5f2cdd..113e6d8 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -1090,6 +1127,15 @@ netdev_dpdk_set_config(struct netdev *netdev, const
> struct smap *args)
>
> dpdk_eth_flow_ctrl_setup(dev);
>
> +/* Rx checksum offload configuration */
> +/* By default the Rx checksum offload is ON */
> +rx_chksm_ofld = smap_get_bool(args, "rx-checksum-offload", true);
> +temp_flag = (dev->hw_ol_features & NETDEV_RX_CHECKSUM_OFFLOAD)
> +!= 0;
> +if (temp_flag != rx_chksm_ofld) {
> +dev->hw_ol_features ^= NETDEV_RX_CHECKSUM_OFFLOAD;
> +}
> +dpdk_eth_checksum_offload_configure(dev);
> ovs_mutex_unlock(&dev->mutex);
I think that while we are going through the trouble of checking
whether the old flag is different from the new one, we might as well
only call dpdk_eth_checksum_offload_configure() if there is a
difference.
> diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c
> index ce2582f..23e987c 100644
> --- a/lib/netdev-native-tnl.c
> +++ b/lib/netdev-native-tnl.c
> @@ -85,9 +85,11 @@ netdev_tnl_ip_extract_tnl_md(struct dp_packet *packet,
> struct flow_tnl *tnl,
>
> ovs_be32 ip_src, ip_dst;
>
> -if (csum(ip, IP_IHL(ip->ip_ihl_ver) * 4)) {
> -VLOG_WARN_RL(&err_rl, "ip packet has invalid checksum");
> -return NULL;
> +if(OVS_UNLIKELY(!dp_packet_ip_checksum_valid(packet))) {
> +if (csum(ip, IP_IHL(ip->ip_ihl_ver) * 4)) {
> +VLOG_WARN_RL(&err_rl, "ip packet has invalid checksum");
> +return NULL;
> +}
> }
>
> if (ntohs(ip->ip_tot_len) > l3_size) {
> @@ -179,18 +181,20 @@ udp_extract_tnl_md(struct dp_packet *packet, struct
> flow_tnl *tnl,
> }
>
> if (udp->udp_csum) {
> -uint32_t csum;
> -if (netdev_tnl_is_header_ipv6(dp_packet_data(packet))) {
> -csum = packet_csum_pseudoheader6(dp_packet_l3(packet));
> -} else {
> -csum = packet_csum_pseudoheader(dp_packet_l3(packet));
> -}
> -
> -csum = csum_continue(csum, udp, dp_packet_size(packet) -
> - ((const unsigned char *)udp -
> - (const unsigned char *)dp_packet_l2(packet)));
> -if (csum_finish(csum)) {
> -return NULL;
> +if(OVS_UNLIKELY(!dp_packet_l4_checksum_valid(packet))) {
> +uint32_t csum;
> +if (netdev_tnl_is_header_ipv6(dp_packet_data(packet))) {
> +csum = packet_csum_pseudoheader6(dp_packet_l3(packet));
> +} else {
> +csum = packet_csum_pseudoheader(dp_packet_l3(packet));
> +}
> +
> +csum = csum_continue(csum, udp, dp_packet_size(packet) -
> + ((const unsigned char *)udp -
> + (const unsigned char
> *)dp_packet_l2(packet)));
> +if (csum_finish(csum)) {
> +return NULL;
> +}
> }
> tnl->flags |= FLOW_TNL_F_CSUM;
> }
I think the previous version of the patch cleared the offload flags
after popping off the tunnel headers. I think that's probably a good
idea here as well. Since we're not terminating the payload it's
probably relatively rare that we would have multiple checksums but
it's theoretically possible that we might have a tunnel-in-tunnel
situation (and I suppose that it's that we might want to use checksum
offload for higher level services - connection tracking/NAT/load
balancing, etc.).
In the future, if DPDK offloads become tunnel aware some of these
issues can become quite complicated due to the presence of multiple
checksums being offloaded and how that is represented. On the Linux
side there was a fair amount of work put into this over the past
several years, so it might be worth taking a look at that for some
inspiration before the DPDK interfaces get locked down.
___
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
