[ofw] [mthca/mlx4] ChkSum bitfields mismatch?
Alex Naslednikov
xalex at mellanox.co.il
Wed Sep 10 01:19:58 PDT 2008
See inline
-----Original Message-----
From: Fab Tillier [mailto:ftillier at windows.microsoft.com]
Sent: Tuesday, September 09, 2008 7:52 PM
To: Alex Naslednikov; Alex Estrin; ofw at lists.openfabrics.org
Subject: RE: [ofw] [mthca/mlx4] ChkSum bitfields mismatch?
---Hi Xalex,
Hello Fab !
>
> You are right. This caused to IP checksum to be recalculated again in
> SW stack, in spite of the HW offload.
> Bellow is the patch that fixes this problem and persuade IPoIB to
> recalculate csum even if HW returns good value in a case of
> CSUM_DISABLED.
> Tzachi, please apply it
>
> Index: core/al/al_av.c
> ===================================================================
> --- core/al/al_av.c (revision 3107)
> +++ core/al/al_av.c (working copy)
> @@ -153,12 +153,12 @@
> return IB_INVALID_PD_HANDLE;
> }
>
> - status = __check_av_port(h_pd->obj.p_ci_ca, p_av_attr);
> + /*status = __check_av_port(h_pd->obj.p_ci_ca, p_av_attr);
> if( status != IB_SUCCESS )
> {
> AL_PRINT_EXIT( TRACE_LEVEL_ERROR, AL_DBG_ERROR,
> ("IB_INVALID_PORT\n") );
> return status;
> - }
> + }*/
---I'm confused by this change - maybe it doesn't belong in this patch?
Yes, this change is a temporarily fix of another bug in mthca. It's not
related to the patch
>
> /* Get an AV tracking structure. */
> h_av = alloc_av();
> Index: hw/mlx4/kernel/bus/inc/cq.h
> ===================================================================
> --- hw/mlx4/kernel/bus/inc/cq.h (revision 3107)
> +++ hw/mlx4/kernel/bus/inc/cq.h (working copy)
> @@ -115,12 +115,12 @@
> };
>
> enum {
> - MLX4_NdisPacketTcpChecksumFailed = 1 << 1,
> - MLX4_NdisPacketUdpChecksumFailed = 1 << 2,
> - MLX4_NdisPacketIpChecksumFailed = 1 << 3,
> - MLX4_NdisPacketTcpChecksumSucceeded = 1 << 4,
> - MLX4_NdisPacketUdpChecksumSucceeded = 1 << 5,
> - MLX4_NdisPacketIpChecksumSucceeded = 1 << 6
> + MLX4_NdisPacketTcpChecksumFailed = 1 << 0,
> + MLX4_NdisPacketUdpChecksumFailed = 1 << 1,
> + MLX4_NdisPacketIpChecksumFailed = 1 << 2,
> + MLX4_NdisPacketTcpChecksumSucceeded = 1 << 3,
> + MLX4_NdisPacketUdpChecksumSucceeded = 1 << 4,
> + MLX4_NdisPacketIpChecksumSucceeded = 1 << 5
> };
>
> static inline void mlx4_cq_arm(struct mlx4_cq *cq, u32 cmd,
> Index: hw/mthca/kernel/mthca_cq.c
> ===================================================================
> --- hw/mthca/kernel/mthca_cq.c (revision 3108)
> +++ hw/mthca/kernel/mthca_cq.c (working copy)
> @@ -118,12 +118,12 @@
> };
>
> enum {
> - MTHCA_NdisPacketTcpChecksumFailed = 1 << 1,
> - MTHCA_NdisPacketUdpChecksumFailed = 1 << 2,
> - MTHCA_NdisPacketIpChecksumFailed = 1 << 3,
> - MTHCA_NdisPacketTcpChecksumSucceeded = 1 << 4,
> - MTHCA_NdisPacketUdpChecksumSucceeded = 1 << 5,
> - MTHCA_NdisPacketIpChecksumSucceeded = 1 << 6
> + MTHCA_NdisPacketTcpChecksumFailed = 1 << 0,
> + MTHCA_NdisPacketUdpChecksumFailed = 1 << 1,
> + MTHCA_NdisPacketIpChecksumFailed = 1 << 2,
> + MTHCA_NdisPacketTcpChecksumSucceeded = 1 << 3,
> + MTHCA_NdisPacketUdpChecksumSucceeded = 1 << 4,
> + MTHCA_NdisPacketIpChecksumSucceeded = 1 << 5
> };
---For the HCA driver fields, why not just use the
NDIS_TCP_IP_CHECKSUM_PACKET_INFO rather than making your own enum? Is
there an issue including ndis.h in the HCA driver?
It makes sense if we had dedicated field for the checsum.
But we are going to use receive_opt in order to pass the value of csum
up to IPoIB driver (see the patch of Alex Estrin). Using this
implementation, it will be very unreadable to define additional value of
NDIS_TCP_IP_CHECKSUM_PACKET_INFO, than embedding into TCP options, than
receive it again in IPoIB driver and transforming it back to
NDIS_TCP_IP_CHECKSUM_PACKET_INFO
> struct mthca_cqe {
> Index: inc/iba/ib_types.h
> ===================================================================
> --- inc/iba/ib_types.h (revision 3107)
> +++ inc/iba/ib_types.h (working copy)
> @@ -11395,3 +11395,4 @@
>
>
>
> +
---This change probably doesn't belong in this patch.
Correct
> Index: ulp/ipoib/kernel/ipoib_port.c
> ===================================================================
> --- ulp/ipoib/kernel/ipoib_port.c (revision 3107)
> +++ ulp/ipoib/kernel/ipoib_port.c (working copy)
> @@ -2542,6 +2542,8 @@
> NDIS_STATUS
> status;
> uint32_t
> pkt_filter;
> ip_stat_sel_t type;
> + NDIS_TCP_IP_CHECKSUM_PACKET_INFO chksum;
> +
> PERF_DECLARE( GetNdisPkt );
>
> IPOIB_ENTER( IPOIB_DBG_RECV ); @@ -2621,21 +2623,30 @@
> ("__buf_mgr_get_ndis_pkt failed\n") );
> return IB_INSUFFICIENT_RESOURCES;
> }
> - if (p_port->p_adapter->params.recv_chksum_offload !=
> CSUM_BYPASS) {
> - /* Get the checksums directly from packet information. */
> - /* In this case, no one of cheksum's cat get false value */
> - /* If hardware checksum failed or wasn't calculated, NDIS will
> recalculate it again */
> +
> + chksum.Value = 0;
> + switch (p_port->p_adapter->params.recv_chksum_offload) {
> + case CSUM_DISABLED:
> + NDIS_PER_PACKET_INFO_FROM_PACKET( *pp_packet,
> TcpIpChecksumPacketInfo ) =
> + (void*)(uintn_t)chksum.Value;
> + break;
> + case CSUM_ENABLED:
> + /* Get the checksums directly from packet information.
> */
> + /* In this case, no one of cheksum's cat get false
> value
> */
> + /* If hardware checksum failed or wasn't calculated,
> NDIS will recalculate it again */
> NDIS_PER_PACKET_INFO_FROM_PACKET( *pp_packet,
> TcpIpChecksumPacketInfo ) =
> - (PVOID) (uintn_t) (p_desc->ndis_csum.Value);
> - } else {
> - NDIS_TCP_IP_CHECKSUM_PACKET_INFO chksum;
> + (void*)(uintn_t)(p_desc->ndis_csum.Value);
> + break;
> + case CSUM_BYPASS:
> /* Flag the checksums as having been calculated. */
> - chksum.Value = 0;
> chksum.Receive.NdisPacketTcpChecksumSucceeded = TRUE;
> chksum.Receive.NdisPacketUdpChecksumSucceeded = TRUE;
> chksum.Receive.NdisPacketIpChecksumSucceeded = TRUE;
> NDIS_PER_PACKET_INFO_FROM_PACKET( *pp_packet,
> TcpIpChecksumPacketInfo ) =
> (void*)(uintn_t)chksum.Value;
> + break;
> + default:
> + ASSERT(FALSE);
---Why not do this check when you store p_desc->ndis_csum?
Our driver will report correct IPOK bit within CQE. Thus, in order to
avoid implicit receive checksum offloading, this check forces NDIS to
recalculate the checksum
---Also, the default case should probably fall through to the
CSUM_DISABLED case after asserting so that a release build at least gets
the correct behavior.
Agree, here it's possible to fall through CSUM_DISABLED in a case of
free version.
I will resend the patch again with this change
> }
> ipoib_inc_recv_stat( p_port->p_adapter, type, p_desc->len );
More information about the ofw
mailing list