[ofw] [mthca/mlx4] ChkSum bitfields mismatch?
Tzachi Dar
tzachid at mellanox.co.il
Wed Sep 10 09:04:10 PDT 2008
Applied to trunk and branch.
Thanks
Tzachi
> -----Original Message-----
> From: Alex Naslednikov
> Sent: Wednesday, September 10, 2008 12:20 PM
> To: Alex Naslednikov; Fab Tillier; Alex Estrin;
> ofw at lists.openfabrics.org
> Cc: Tzachi Dar
> Subject: RE: [ofw] [mthca/mlx4] ChkSum bitfields mismatch?
>
> Attached the patch that includes Fab comments.
> Index: ulp/ipoib/kernel/ipoib_port.c
> ===================================================================
> --- ulp/ipoib/kernel/ipoib_port.c (revision 3142)
> +++ ulp/ipoib/kernel/ipoib_port.c (revision 3143)
> @@ -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,32 @@
> ("__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);
> + NDIS_PER_PACKET_INFO_FROM_PACKET( *pp_packet,
> TcpIpChecksumPacketInfo ) =
> + (void*)(uintn_t)chksum.Value;
> }
> ipoib_inc_recv_stat( p_port->p_adapter, type, p_desc->len );
>
> Index: hw/mlx4/kernel/bus/inc/cq.h
> ===================================================================
> --- hw/mlx4/kernel/bus/inc/cq.h (revision 3142)
> +++ hw/mlx4/kernel/bus/inc/cq.h (revision 3143)
> @@ -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 3142)
> +++ hw/mthca/kernel/mthca_cq.c (revision 3143)
> @@ -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
> };
>
> struct mthca_cqe {
>
> -----Original Message-----
> From: Alex Naslednikov
> Sent: Wednesday, September 10, 2008 10:20 AM
> To: 'Fab Tillier'; Alex Estrin; ofw at lists.openfabrics.org
> Cc: Tzachi Dar
> Subject: RE: [ofw] [mthca/mlx4] ChkSum bitfields mismatch?
>
> 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