| Message ID | 1430461431-5936-4-git-send-email-akinobu.mita@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Herbert Xu |
| Headers | show |
On Fri, 2015-05-01 at 15:23 +0900, Akinobu Mita wrote: > This introduces crc_t10dif_update() which enables to calculate CRC > for a block which straddles multiple SG elements by calling multiple > times. This also converts crc_t10dif() to use crc_t10dif_update() as > they are almost same. > > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com> > Acked-by: Martin K. Petersen <martin.petersen@oracle.com> > Cc: Tim Chen <tim.c.chen@linux.intel.com> > Cc: Herbert Xu <herbert@gondor.apana.org.au> > Cc: "David S. Miller" <davem@davemloft.net> > Cc: linux-crypto@vger.kernel.org > Cc: Nicholas Bellinger <nab@linux-iscsi.org> > Cc: Sagi Grimberg <sagig@mellanox.com> > Cc: "Martin K. Petersen" <martin.petersen@oracle.com> > Cc: Christoph Hellwig <hch@lst.de> > Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com> > Cc: target-devel@vger.kernel.org > --- > * Changes from v3: > - Reduce duplicated code between crc_t10dif_update and crc_t10dif, > suggested by Tim and Herbert > > include/linux/crc-t10dif.h | 1 + > lib/crc-t10dif.c | 13 ++++++++++--- > 2 files changed, 11 insertions(+), 3 deletions(-) > > diff --git a/include/linux/crc-t10dif.h b/include/linux/crc-t10dif.h > index cf53d07..d81961e 100644 > --- a/include/linux/crc-t10dif.h > +++ b/include/linux/crc-t10dif.h > @@ -9,5 +9,6 @@ > extern __u16 crc_t10dif_generic(__u16 crc, const unsigned char *buffer, > size_t len); > extern __u16 crc_t10dif(unsigned char const *, size_t); > +extern __u16 crc_t10dif_update(__u16 crc, unsigned char const *, size_t); > > #endif > diff --git a/lib/crc-t10dif.c b/lib/crc-t10dif.c > index dfe6ec1..d775737 100644 > --- a/lib/crc-t10dif.c > +++ b/lib/crc-t10dif.c > @@ -19,7 +19,7 @@ > static struct crypto_shash *crct10dif_tfm; > static struct static_key crct10dif_fallback __read_mostly; > > -__u16 crc_t10dif(const unsigned char *buffer, size_t len) > +__u16 crc_t10dif_update(__u16 crc, const unsigned char *buffer, size_t len) > { > struct { > struct shash_desc shash; > @@ -28,17 +28,24 @@ __u16 crc_t10dif(const unsigned char *buffer, size_t len) > int err; > > if (static_key_false(&crct10dif_fallback)) > - return crc_t10dif_generic(0, buffer, len); > + return crc_t10dif_generic(crc, buffer, len); > > desc.shash.tfm = crct10dif_tfm; > desc.shash.flags = 0; > - *(__u16 *)desc.ctx = 0; > > + err = crypto_shash_import(&desc.shash, &crc); > + BUG_ON(err); > err = crypto_shash_update(&desc.shash, buffer, len); > BUG_ON(err); > > return *(__u16 *)desc.ctx; > } > +EXPORT_SYMBOL(crc_t10dif_update); > + > +__u16 crc_t10dif(const unsigned char *buffer, size_t len) > +{ > + return crc_t10dif_update(0, buffer, len); Nitpicking a bit: Will putting an extra function call to crc_t10dif_update will add extra overhead to crc_t10dif, which is what most driver uses? As we are calling crc_t10dif a lot (millions of times) as we go through a block device, this extra function call is undesirable. Using a __crc_t10dif_update inline function that both crc_t10dif_update and crc_t10dif invokes can will avoid this overhead. Tim > +} > EXPORT_SYMBOL(crc_t10dif); > > static int __init crc_t10dif_mod_init(void) -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2015-05-02 1:15 GMT+09:00 Tim Chen <tim.c.chen@linux.intel.com>: > On Fri, 2015-05-01 at 15:23 +0900, Akinobu Mita wrote: >> This introduces crc_t10dif_update() which enables to calculate CRC >> for a block which straddles multiple SG elements by calling multiple >> times. This also converts crc_t10dif() to use crc_t10dif_update() as >> they are almost same. >> >> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com> >> Acked-by: Martin K. Petersen <martin.petersen@oracle.com> >> Cc: Tim Chen <tim.c.chen@linux.intel.com> >> Cc: Herbert Xu <herbert@gondor.apana.org.au> >> Cc: "David S. Miller" <davem@davemloft.net> >> Cc: linux-crypto@vger.kernel.org >> Cc: Nicholas Bellinger <nab@linux-iscsi.org> >> Cc: Sagi Grimberg <sagig@mellanox.com> >> Cc: "Martin K. Petersen" <martin.petersen@oracle.com> >> Cc: Christoph Hellwig <hch@lst.de> >> Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com> >> Cc: target-devel@vger.kernel.org >> --- >> * Changes from v3: >> - Reduce duplicated code between crc_t10dif_update and crc_t10dif, >> suggested by Tim and Herbert >> >> include/linux/crc-t10dif.h | 1 + >> lib/crc-t10dif.c | 13 ++++++++++--- >> 2 files changed, 11 insertions(+), 3 deletions(-) >> >> diff --git a/include/linux/crc-t10dif.h b/include/linux/crc-t10dif.h >> index cf53d07..d81961e 100644 >> --- a/include/linux/crc-t10dif.h >> +++ b/include/linux/crc-t10dif.h >> @@ -9,5 +9,6 @@ >> extern __u16 crc_t10dif_generic(__u16 crc, const unsigned char *buffer, >> size_t len); >> extern __u16 crc_t10dif(unsigned char const *, size_t); >> +extern __u16 crc_t10dif_update(__u16 crc, unsigned char const *, size_t); >> >> #endif >> diff --git a/lib/crc-t10dif.c b/lib/crc-t10dif.c >> index dfe6ec1..d775737 100644 >> --- a/lib/crc-t10dif.c >> +++ b/lib/crc-t10dif.c >> @@ -19,7 +19,7 @@ >> static struct crypto_shash *crct10dif_tfm; >> static struct static_key crct10dif_fallback __read_mostly; >> >> -__u16 crc_t10dif(const unsigned char *buffer, size_t len) >> +__u16 crc_t10dif_update(__u16 crc, const unsigned char *buffer, size_t len) >> { >> struct { >> struct shash_desc shash; >> @@ -28,17 +28,24 @@ __u16 crc_t10dif(const unsigned char *buffer, size_t len) >> int err; >> >> if (static_key_false(&crct10dif_fallback)) >> - return crc_t10dif_generic(0, buffer, len); >> + return crc_t10dif_generic(crc, buffer, len); >> >> desc.shash.tfm = crct10dif_tfm; >> desc.shash.flags = 0; >> - *(__u16 *)desc.ctx = 0; >> >> + err = crypto_shash_import(&desc.shash, &crc); >> + BUG_ON(err); >> err = crypto_shash_update(&desc.shash, buffer, len); >> BUG_ON(err); >> >> return *(__u16 *)desc.ctx; >> } >> +EXPORT_SYMBOL(crc_t10dif_update); >> + >> +__u16 crc_t10dif(const unsigned char *buffer, size_t len) >> +{ >> + return crc_t10dif_update(0, buffer, len); > > Nitpicking a bit: > > Will putting an extra function call to crc_t10dif_update will add extra > overhead to crc_t10dif, which is what most driver uses? As > we are calling crc_t10dif a lot (millions of times) as we go > through a block device, this extra function call > is undesirable. Using a __crc_t10dif_update > inline function that both crc_t10dif_update and crc_t10dif > invokes can will avoid this overhead. I'll convert crc_t10dif to inline function. But do we also need to make crc_t10dif_update inline function? (i.e. is there any difference between __crc_t10dif_update and crc_t10dif_update?) -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, 2015-05-02 at 10:38 +0900, Akinobu Mita wrote: > 2015-05-02 1:15 GMT+09:00 Tim Chen <tim.c.chen@linux.intel.com>: > > On Fri, 2015-05-01 at 15:23 +0900, Akinobu Mita wrote: > >> This introduces crc_t10dif_update() which enables to calculate CRC > >> for a block which straddles multiple SG elements by calling multiple > >> times. This also converts crc_t10dif() to use crc_t10dif_update() as > >> they are almost same. > >> > >> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com> > >> Acked-by: Martin K. Petersen <martin.petersen@oracle.com> > >> Cc: Tim Chen <tim.c.chen@linux.intel.com> > >> Cc: Herbert Xu <herbert@gondor.apana.org.au> > >> Cc: "David S. Miller" <davem@davemloft.net> > >> Cc: linux-crypto@vger.kernel.org > >> Cc: Nicholas Bellinger <nab@linux-iscsi.org> > >> Cc: Sagi Grimberg <sagig@mellanox.com> > >> Cc: "Martin K. Petersen" <martin.petersen@oracle.com> > >> Cc: Christoph Hellwig <hch@lst.de> > >> Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com> > >> Cc: target-devel@vger.kernel.org > >> --- > >> * Changes from v3: > >> - Reduce duplicated code between crc_t10dif_update and crc_t10dif, > >> suggested by Tim and Herbert > >> > >> include/linux/crc-t10dif.h | 1 + > >> lib/crc-t10dif.c | 13 ++++++++++--- > >> 2 files changed, 11 insertions(+), 3 deletions(-) > >> > >> diff --git a/include/linux/crc-t10dif.h b/include/linux/crc-t10dif.h > >> index cf53d07..d81961e 100644 > >> --- a/include/linux/crc-t10dif.h > >> +++ b/include/linux/crc-t10dif.h > >> @@ -9,5 +9,6 @@ > >> extern __u16 crc_t10dif_generic(__u16 crc, const unsigned char *buffer, > >> size_t len); > >> extern __u16 crc_t10dif(unsigned char const *, size_t); > >> +extern __u16 crc_t10dif_update(__u16 crc, unsigned char const *, size_t); > >> > >> #endif > >> diff --git a/lib/crc-t10dif.c b/lib/crc-t10dif.c > >> index dfe6ec1..d775737 100644 > >> --- a/lib/crc-t10dif.c > >> +++ b/lib/crc-t10dif.c > >> @@ -19,7 +19,7 @@ > >> static struct crypto_shash *crct10dif_tfm; > >> static struct static_key crct10dif_fallback __read_mostly; > >> > >> -__u16 crc_t10dif(const unsigned char *buffer, size_t len) > >> +__u16 crc_t10dif_update(__u16 crc, const unsigned char *buffer, size_t len) > >> { > >> struct { > >> struct shash_desc shash; > >> @@ -28,17 +28,24 @@ __u16 crc_t10dif(const unsigned char *buffer, size_t len) > >> int err; > >> > >> if (static_key_false(&crct10dif_fallback)) > >> - return crc_t10dif_generic(0, buffer, len); > >> + return crc_t10dif_generic(crc, buffer, len); > >> > >> desc.shash.tfm = crct10dif_tfm; > >> desc.shash.flags = 0; > >> - *(__u16 *)desc.ctx = 0; > >> > >> + err = crypto_shash_import(&desc.shash, &crc); > >> + BUG_ON(err); > >> err = crypto_shash_update(&desc.shash, buffer, len); > >> BUG_ON(err); > >> > >> return *(__u16 *)desc.ctx; > >> } > >> +EXPORT_SYMBOL(crc_t10dif_update); > >> + > >> +__u16 crc_t10dif(const unsigned char *buffer, size_t len) > >> +{ > >> + return crc_t10dif_update(0, buffer, len); > > > > Nitpicking a bit: > > > > Will putting an extra function call to crc_t10dif_update will add extra > > overhead to crc_t10dif, which is what most driver uses? As > > we are calling crc_t10dif a lot (millions of times) as we go > > through a block device, this extra function call > > is undesirable. Using a __crc_t10dif_update > > inline function that both crc_t10dif_update and crc_t10dif > > invokes can will avoid this overhead. > > I'll convert crc_t10dif to inline function. But do we also need to > make crc_t10dif_update inline function? (i.e. is there any difference > between __crc_t10dif_update and crc_t10dif_update?) I don't mean convert crc_t10dif to inline, but make a local inline function __crc_t10dif_update that both crc_t10dif_update and crc_t10dif can use so crc_t10dif don't have to do one more function call. Yes, crc_t10dif_update and __crc_t10dif_update are equivalent but since the latter is inlined, so there's no performance impact and we get rid of the overhead of the extra function call in crc_t10dif. Like + +__u16 crc_t10dif_update(__u16 crc, const unsigned char *buffer, size_t len) +{ + return __crc_t10dif_update(crc, buffer, len, true); +} +EXPORT_SYMBOL(crc_t10dif_update); + +__u16 crc_t10dif(const unsigned char *buffer, size_t len) +{ + return __crc_t10dif_update(0, buffer, len, false); +} EXPORT_SYMBOL(crc_t10dif); Thanks. Tim -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2015-05-05 1:09 GMT+09:00 Tim Chen <tim.c.chen@linux.intel.com>: > On Sat, 2015-05-02 at 10:38 +0900, Akinobu Mita wrote: >> 2015-05-02 1:15 GMT+09:00 Tim Chen <tim.c.chen@linux.intel.com>: >> > On Fri, 2015-05-01 at 15:23 +0900, Akinobu Mita wrote: >> >> This introduces crc_t10dif_update() which enables to calculate CRC >> >> for a block which straddles multiple SG elements by calling multiple >> >> times. This also converts crc_t10dif() to use crc_t10dif_update() as >> >> they are almost same. >> >> >> >> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com> >> >> Acked-by: Martin K. Petersen <martin.petersen@oracle.com> >> >> Cc: Tim Chen <tim.c.chen@linux.intel.com> >> >> Cc: Herbert Xu <herbert@gondor.apana.org.au> >> >> Cc: "David S. Miller" <davem@davemloft.net> >> >> Cc: linux-crypto@vger.kernel.org >> >> Cc: Nicholas Bellinger <nab@linux-iscsi.org> >> >> Cc: Sagi Grimberg <sagig@mellanox.com> >> >> Cc: "Martin K. Petersen" <martin.petersen@oracle.com> >> >> Cc: Christoph Hellwig <hch@lst.de> >> >> Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com> >> >> Cc: target-devel@vger.kernel.org >> >> --- >> >> * Changes from v3: >> >> - Reduce duplicated code between crc_t10dif_update and crc_t10dif, >> >> suggested by Tim and Herbert >> >> >> >> include/linux/crc-t10dif.h | 1 + >> >> lib/crc-t10dif.c | 13 ++++++++++--- >> >> 2 files changed, 11 insertions(+), 3 deletions(-) >> >> >> >> diff --git a/include/linux/crc-t10dif.h b/include/linux/crc-t10dif.h >> >> index cf53d07..d81961e 100644 >> >> --- a/include/linux/crc-t10dif.h >> >> +++ b/include/linux/crc-t10dif.h >> >> @@ -9,5 +9,6 @@ >> >> extern __u16 crc_t10dif_generic(__u16 crc, const unsigned char *buffer, >> >> size_t len); >> >> extern __u16 crc_t10dif(unsigned char const *, size_t); >> >> +extern __u16 crc_t10dif_update(__u16 crc, unsigned char const *, size_t); >> >> >> >> #endif >> >> diff --git a/lib/crc-t10dif.c b/lib/crc-t10dif.c >> >> index dfe6ec1..d775737 100644 >> >> --- a/lib/crc-t10dif.c >> >> +++ b/lib/crc-t10dif.c >> >> @@ -19,7 +19,7 @@ >> >> static struct crypto_shash *crct10dif_tfm; >> >> static struct static_key crct10dif_fallback __read_mostly; >> >> >> >> -__u16 crc_t10dif(const unsigned char *buffer, size_t len) >> >> +__u16 crc_t10dif_update(__u16 crc, const unsigned char *buffer, size_t len) >> >> { >> >> struct { >> >> struct shash_desc shash; >> >> @@ -28,17 +28,24 @@ __u16 crc_t10dif(const unsigned char *buffer, size_t len) >> >> int err; >> >> >> >> if (static_key_false(&crct10dif_fallback)) >> >> - return crc_t10dif_generic(0, buffer, len); >> >> + return crc_t10dif_generic(crc, buffer, len); >> >> >> >> desc.shash.tfm = crct10dif_tfm; >> >> desc.shash.flags = 0; >> >> - *(__u16 *)desc.ctx = 0; >> >> >> >> + err = crypto_shash_import(&desc.shash, &crc); >> >> + BUG_ON(err); >> >> err = crypto_shash_update(&desc.shash, buffer, len); >> >> BUG_ON(err); >> >> >> >> return *(__u16 *)desc.ctx; >> >> } >> >> +EXPORT_SYMBOL(crc_t10dif_update); >> >> + >> >> +__u16 crc_t10dif(const unsigned char *buffer, size_t len) >> >> +{ >> >> + return crc_t10dif_update(0, buffer, len); >> > >> > Nitpicking a bit: >> > >> > Will putting an extra function call to crc_t10dif_update will add extra >> > overhead to crc_t10dif, which is what most driver uses? As >> > we are calling crc_t10dif a lot (millions of times) as we go >> > through a block device, this extra function call >> > is undesirable. Using a __crc_t10dif_update >> > inline function that both crc_t10dif_update and crc_t10dif >> > invokes can will avoid this overhead. >> >> I'll convert crc_t10dif to inline function. But do we also need to >> make crc_t10dif_update inline function? (i.e. is there any difference >> between __crc_t10dif_update and crc_t10dif_update?) > > I don't mean convert crc_t10dif to inline, but make a local inline function > __crc_t10dif_update that both crc_t10dif_update and crc_t10dif can use > so crc_t10dif don't have to do one more function call. Yes, crc_t10dif_update > and __crc_t10dif_update are equivalent but since the latter is inlined, so there's no > performance impact and we get rid of the overhead of the extra function > call in crc_t10dif. > > Like > > + > +__u16 crc_t10dif_update(__u16 crc, const unsigned char *buffer, size_t len) > +{ > + return __crc_t10dif_update(crc, buffer, len, true); > +} > +EXPORT_SYMBOL(crc_t10dif_update); > + > +__u16 crc_t10dif(const unsigned char *buffer, size_t len) > +{ > + return __crc_t10dif_update(0, buffer, len, false); > +} > EXPORT_SYMBOL(crc_t10dif); You mean that we should avoid crypto_shash_import() function call in crc_t10dif(), right? The reason for crypto_shash_import() was I wanted to make crc_t10dif_update() more generic. But as far as I can see existing crct10dif modules (crct10dif-generic and crct10dif-pclmul), we don't need it (i.e. '*(__u16 *)desc.ctx = crc' instead of crc_t10dif_import) Do you think it's better to remove crc_t10dif_import call from crc_t10dif() and crc_t10dif_update() altogether? -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 05, 2015 at 01:22:03PM +0900, Akinobu Mita wrote: > > The reason for crypto_shash_import() was I wanted to make > crc_t10dif_update() more generic. But as far as I can see existing > crct10dif modules (crct10dif-generic and crct10dif-pclmul), we don't > need it (i.e. '*(__u16 *)desc.ctx = crc' instead of crc_t10dif_import) Yes we do the same thing with crc32c so I don't see why you couldn't do that here. Cheers,
On Tue, 2015-05-05 at 13:22 +0900, Akinobu Mita wrote: > > > > + > > +__u16 crc_t10dif_update(__u16 crc, const unsigned char *buffer, size_t len) > > +{ > > + return __crc_t10dif_update(crc, buffer, len, true); > > +} > > +EXPORT_SYMBOL(crc_t10dif_update); > > + > > +__u16 crc_t10dif(const unsigned char *buffer, size_t len) > > +{ > > + return __crc_t10dif_update(0, buffer, len, false); > > +} > > EXPORT_SYMBOL(crc_t10dif); > > You mean that we should avoid crypto_shash_import() function call > in crc_t10dif(), right? > I think it is okay for crypto_shash_import(). Just that we can eliminate an extra call from crc_t10dif to crc_t10_dif as in your proposed patch, > +EXPORT_SYMBOL(crc_t10dif_update); > + > +__u16 crc_t10dif(const unsigned char *buffer, size_t len) > + { > + return crc_t10dif_update(0, buffer, len); > but use a local inlined __crc_t10dif_update. Tim -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2015-05-05 at 13:22 +0900, Akinobu Mita wrote: > The reason for crypto_shash_import() was I wanted to make > crc_t10dif_update() more generic. But as far as I can see existing > crct10dif modules (crct10dif-generic and crct10dif-pclmul), we don't > need it (i.e. '*(__u16 *)desc.ctx = crc' instead of crc_t10dif_import) > > Do you think it's better to remove crc_t10dif_import call from > crc_t10dif() and crc_t10dif_update() altogether? Yes, it will be better if we can eliminate crc_t10dif_import call if we can do a direct assignment. Tim -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/crc-t10dif.h b/include/linux/crc-t10dif.h index cf53d07..d81961e 100644 --- a/include/linux/crc-t10dif.h +++ b/include/linux/crc-t10dif.h @@ -9,5 +9,6 @@ extern __u16 crc_t10dif_generic(__u16 crc, const unsigned char *buffer, size_t len); extern __u16 crc_t10dif(unsigned char const *, size_t); +extern __u16 crc_t10dif_update(__u16 crc, unsigned char const *, size_t); #endif diff --git a/lib/crc-t10dif.c b/lib/crc-t10dif.c index dfe6ec1..d775737 100644 --- a/lib/crc-t10dif.c +++ b/lib/crc-t10dif.c @@ -19,7 +19,7 @@ static struct crypto_shash *crct10dif_tfm; static struct static_key crct10dif_fallback __read_mostly; -__u16 crc_t10dif(const unsigned char *buffer, size_t len) +__u16 crc_t10dif_update(__u16 crc, const unsigned char *buffer, size_t len) { struct { struct shash_desc shash; @@ -28,17 +28,24 @@ __u16 crc_t10dif(const unsigned char *buffer, size_t len) int err; if (static_key_false(&crct10dif_fallback)) - return crc_t10dif_generic(0, buffer, len); + return crc_t10dif_generic(crc, buffer, len); desc.shash.tfm = crct10dif_tfm; desc.shash.flags = 0; - *(__u16 *)desc.ctx = 0; + err = crypto_shash_import(&desc.shash, &crc); + BUG_ON(err); err = crypto_shash_update(&desc.shash, buffer, len); BUG_ON(err); return *(__u16 *)desc.ctx; } +EXPORT_SYMBOL(crc_t10dif_update); + +__u16 crc_t10dif(const unsigned char *buffer, size_t len) +{ + return crc_t10dif_update(0, buffer, len); +} EXPORT_SYMBOL(crc_t10dif); static int __init crc_t10dif_mod_init(void)