[PATCH 0/2] lio-target: Add support for libcrypto crc32c and crc32-intel offload

[PATCH 0/2] lio-target: Add support for libcrypto crc32c and crc32-intel offload

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

Greetings all,

This patch series converts the LIO-Target fabric module from using a legacy
internal slicing by 1x CRC32C algorithm to the slicing by 1x CRC32C available in
crc32c.ko, as well as initial support for the Nehalem series crc32c-intel.ko
instruction offload available since v2.6.27 in late 2008.

So far this series has been lightly tested with a handful of Open-iSCSI client VMs
with the optimized crc32c-intel.ko offload case, and there appears to be some
HeaderDigest failures for one case with RHEL6 B2 x86_64, while Ubuntu i686 on
v2.6.27 and OpenSuse 11.2 x86_64 on v2.6.31 work as expected with HeaderDigest=CRC32C
and DataDigest=CRC32C using the offload on the LIO-Target side.

Currently this patch disables the new iSCSI TPG attribute crc32c_x86_offload
to force the crc32c.ko slicing by x1 CRC32C algorithm until these compatibility
issues with existing libcrc32c clients can be properly resolved with the Nehalem
CRC32C offload instructions running on the LIO-Target side.

Comments are welcome!

Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>

Nicholas Bellinger (2):
  lio-target: Convert to use libcrypto crc32c
  lio-target: Add support for Intel Nehalem crc32-intel libcrypto
    offload

 drivers/target/lio-target/Kbuild                  |    1 -
 drivers/target/lio-target/iscsi_crc.c             |  171 ------------
 drivers/target/lio-target/iscsi_crc.h             |    9 -
 drivers/target/lio-target/iscsi_target.c          |  302 ++++++++++++++++-----
 drivers/target/lio-target/iscsi_target_configfs.c |    6 +
 drivers/target/lio-target/iscsi_target_core.h     |    7 +
 drivers/target/lio-target/iscsi_target_erl1.c     |    1 -
 drivers/target/lio-target/iscsi_target_erl2.c     |    1 -
 drivers/target/lio-target/iscsi_target_login.c    |   76 +++++-
 drivers/target/lio-target/iscsi_target_login.h    |    1 +
 drivers/target/lio-target/iscsi_target_nego.c     |   18 +-
 drivers/target/lio-target/iscsi_target_tmr.c      |    1 -
 drivers/target/lio-target/iscsi_target_tpg.c      |   19 ++
 drivers/target/lio-target/iscsi_target_tpg.h      |    1 +
 14 files changed, 356 insertions(+), 258 deletions(-)
 delete mode 100644 drivers/target/lio-target/iscsi_crc.c
 delete mode 100644 drivers/target/lio-target/iscsi_crc.h

Re: [PATCH 0/2] lio-target: Add support for libcrypto crc32c and crc32-intel offload

[Hannes Reinecke]

Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
> 
> Greetings all,
> 
> This patch series converts the LIO-Target fabric module from using a legacy
> internal slicing by 1x CRC32C algorithm to the slicing by 1x CRC32C available in
> crc32c.ko, as well as initial support for the Nehalem series crc32c-intel.ko
> instruction offload available since v2.6.27 in late 2008.
> 
> So far this series has been lightly tested with a handful of Open-iSCSI client VMs
> with the optimized crc32c-intel.ko offload case, and there appears to be some
> HeaderDigest failures for one case with RHEL6 B2 x86_64, while Ubuntu i686 on
> v2.6.27 and OpenSuse 11.2 x86_64 on v2.6.31 work as expected with HeaderDigest=CRC32C
> and DataDigest=CRC32C using the offload on the LIO-Target side.
> 
> Currently this patch disables the new iSCSI TPG attribute crc32c_x86_offload
> to force the crc32c.ko slicing by x1 CRC32C algorithm until these compatibility
> issues with existing libcrc32c clients can be properly resolved with the Nehalem
> CRC32C offload instructions running on the LIO-Target side.
> 
Due to a customer issue I had to revisit the CRC32 issue recently.
And according to you the main reason for using your own CRC routines
had been endianness issues.
IE the in-kernel crc32c routines apparently weren't able to
calculate the checksum in an endianness-independent manner.
So a CRC calculated on a BE machine would fail to be validated by a
LE machine and vice versa.

Has this been fixed / verified?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)

Re: [PATCH 0/2] lio-target: Add support for libcrypto crc32c and crc32-intel offload

[Hannes Reinecke]

Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
> 
> Greetings all,
> 
> This patch series converts the LIO-Target fabric module from using a legacy
> internal slicing by 1x CRC32C algorithm to the slicing by 1x CRC32C available in
> crc32c.ko, as well as initial support for the Nehalem series crc32c-intel.ko
> instruction offload available since v2.6.27 in late 2008.
> 
> So far this series has been lightly tested with a handful of Open-iSCSI client VMs
> with the optimized crc32c-intel.ko offload case, and there appears to be some
> HeaderDigest failures for one case with RHEL6 B2 x86_64, while Ubuntu i686 on
> v2.6.27 and OpenSuse 11.2 x86_64 on v2.6.31 work as expected with HeaderDigest=CRC32C
> and DataDigest=CRC32C using the offload on the LIO-Target side.
> 
> Currently this patch disables the new iSCSI TPG attribute crc32c_x86_offload
> to force the crc32c.ko slicing by x1 CRC32C algorithm until these compatibility
> issues with existing libcrc32c clients can be properly resolved with the Nehalem
> CRC32C offload instructions running on the LIO-Target side.
> 
Due to a customer issue I had to revisit the CRC32 issue recently.
And according to you the main reason for using your own CRC routines
had been endianness issues.
IE the in-kernel crc32c routines apparently weren't able to
calculate the checksum in an endianness-independent manner.
So a CRC calculated on a BE machine would fail to be validated by a
LE machine and vice versa.

Has this been fixed / verified?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] lio-target: Add support for libcrypto crc32c and crc32-intel offload

[Nicholas A. Bellinger]

On Mon, 2010-09-13 at 11:21 +0200, Hannes Reinecke wrote:
> Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <nab@linux-iscsi.org>
> > 
> > Greetings all,
> > 
> > This patch series converts the LIO-Target fabric module from using a legacy
> > internal slicing by 1x CRC32C algorithm to the slicing by 1x CRC32C available in
> > crc32c.ko, as well as initial support for the Nehalem series crc32c-intel.ko
> > instruction offload available since v2.6.27 in late 2008.
> > 
> > So far this series has been lightly tested with a handful of Open-iSCSI client VMs
> > with the optimized crc32c-intel.ko offload case, and there appears to be some
> > HeaderDigest failures for one case with RHEL6 B2 x86_64, while Ubuntu i686 on
> > v2.6.27 and OpenSuse 11.2 x86_64 on v2.6.31 work as expected with HeaderDigest=CRC32C
> > and DataDigest=CRC32C using the offload on the LIO-Target side.
> > 
> > Currently this patch disables the new iSCSI TPG attribute crc32c_x86_offload
> > to force the crc32c.ko slicing by x1 CRC32C algorithm until these compatibility
> > issues with existing libcrc32c clients can be properly resolved with the Nehalem
> > CRC32C offload instructions running on the LIO-Target side.
> > 
> Due to a customer issue I had to revisit the CRC32 issue recently.
> And according to you the main reason for using your own CRC routines
> had been endianness issues.

Indeed, this has historically been the case for LIO-Target and
Core-iSCSI.  I noticed the issue with libcrypto while porting the latter
on a old G4 TI-book waaay back during v2.6.10-rc3 days.

> IE the in-kernel crc32c routines apparently weren't able to
> calculate the checksum in an endianness-independent manner.
> So a CRC calculated on a BE machine would fail to be validated by a
> LE machine and vice versa.
> 
> Has this been fixed / verified?
> 

>From taking a look at crypto/crc32c.c it still appears AFAICT to not be
big endian safe.  I was planning to test this patch on some powerpc/ppc
hardware with v2.6.36-rc4 in the next days, but it looks like
lio-core-2.6.git will need a seperate crypto/crc32c.c patch to function
properly on big endian arches.

Thanks for mentioning this point Hannes!

--nab

Re: [PATCH 0/2] lio-target: Add support for libcrypto crc32c and crc32-intel offload

[Mike Christie]

On 09/13/2010 02:50 PM, Nicholas A. Bellinger wrote:
>> IE the in-kernel crc32c routines apparently weren't able to
>> calculate the checksum in an endianness-independent manner.
>> So a CRC calculated on a BE machine would fail to be validated by a
>> LE machine and vice versa.
>>
>> Has this been fixed / verified?
>>
>
>> From taking a look at crypto/crc32c.c it still appears AFAICT to not be
> big endian safe.  I was planning to test this patch on some powerpc/ppc
> hardware with v2.6.36-rc4 in the next days, but it looks like
> lio-core-2.6.git will need a seperate crypto/crc32c.c patch to function
> properly on big endian arches.
>

There was this bug that was fixed a couple years ago:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=ef19454bd437b2ba14c9cda1de85debd9f383484
since then I think we have not had problems.

Re: [PATCH 0/2] lio-target: Add support for libcrypto crc32c and crc32-intel offload

[Nicholas A. Bellinger]

On Mon, 2010-09-13 at 15:35 -0500, Mike Christie wrote:
> On 09/13/2010 02:50 PM, Nicholas A. Bellinger wrote:
> >> IE the in-kernel crc32c routines apparently weren't able to
> >> calculate the checksum in an endianness-independent manner.
> >> So a CRC calculated on a BE machine would fail to be validated by a
> >> LE machine and vice versa.
> >>
> >> Has this been fixed / verified?
> >>
> >
> >> From taking a look at crypto/crc32c.c it still appears AFAICT to not be
> > big endian safe.  I was planning to test this patch on some powerpc/ppc
> > hardware with v2.6.36-rc4 in the next days, but it looks like
> > lio-core-2.6.git will need a seperate crypto/crc32c.c patch to function
> > properly on big endian arches.
> >
> 
> There was this bug that was fixed a couple years ago:
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=ef19454bd437b2ba14c9cda1de85debd9f383484
> since then I think we have not had problems.

Hi Mike,

Thanks for the pointer on this, I will give it a shot on powerpc and see
what happens.

FYI, I was under the assumption that for BIG_ENDIAN that crc32c() still
needed an bitshift for the returned value.  This is what we had been
doing with our old internal do_crc() code here:

http://git.kernel.org/?p=linux/kernel/git/nab/lio-core-2.6.git;a=blob;f=drivers/target/lio-target/iscsi_crc.c;hb=83559072b27eb8cd92abb9a3fc9be85018c5db06#l138

AFAICT there is still nothing that makes crypto/crc32c.c big endian
safe, unless I am overlooking something obvious..?

Best,

--nab

Re: [PATCH 0/2] lio-target: Add support for libcrypto crc32c and crc32-intel offload

[Nicholas A. Bellinger]

On Mon, 2010-09-13 at 13:40 -0700, Nicholas A. Bellinger wrote:
> On Mon, 2010-09-13 at 15:35 -0500, Mike Christie wrote:
> > On 09/13/2010 02:50 PM, Nicholas A. Bellinger wrote:
> > >> IE the in-kernel crc32c routines apparently weren't able to
> > >> calculate the checksum in an endianness-independent manner.
> > >> So a CRC calculated on a BE machine would fail to be validated by a
> > >> LE machine and vice versa.
> > >>
> > >> Has this been fixed / verified?
> > >>
> > >
> > >> From taking a look at crypto/crc32c.c it still appears AFAICT to not be
> > > big endian safe.  I was planning to test this patch on some powerpc/ppc
> > > hardware with v2.6.36-rc4 in the next days, but it looks like
> > > lio-core-2.6.git will need a seperate crypto/crc32c.c patch to function
> > > properly on big endian arches.
> > >
> > 
> > There was this bug that was fixed a couple years ago:
> > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=ef19454bd437b2ba14c9cda1de85debd9f383484
> > since then I think we have not had problems.
> 
> Hi Mike,
> 
> Thanks for the pointer on this, I will give it a shot on powerpc and see
> what happens.
> 
> FYI, I was under the assumption that for BIG_ENDIAN that crc32c() still
> needed an bitshift for the returned value.  This is what we had been
> doing with our old internal do_crc() code here:
> 
> http://git.kernel.org/?p=linux/kernel/git/nab/lio-core-2.6.git;a=blob;f=drivers/target/lio-target/iscsi_crc.c;hb=83559072b27eb8cd92abb9a3fc9be85018c5db06#l138
> 
> AFAICT there is still nothing that makes crypto/crc32c.c big endian
> safe, unless I am overlooking something obvious..?
> 

Hi Mike and Hannes,

So after a quick test using Open-iSCSI 32-bit x86 VM w/ v2.6.27 and
LIO-Target 64-bit powerpc w/v2.6.36-rc3 with this libcrypto patch I do
not see any HeaderDigest or DataDigest failures using the exiting
slicing by 1x crypto/crc32c.c code on 64-bit BIG_ENDIAN.  While I am at
it I will also try OpenSuse 11.2 and RHEL6 B2 x86_64 VMs for good
measure.

So the appropiate endian conversion does occur in chksum_final() and
__chksum_finup() code.  My apologies for the earier confusion.

Thanks again,

--nab