From: "Horia Geantă" <horia.geanta@freescale.com>
To: Kim Phillips <kim.phillips@freescale.com>,
Martin Hicks <mort@bork.org>,
Herbert Xu <herbert@gondor.apana.org.au>
Cc: Scott Wood <scottwood@freescale.com>,
Kumar Gala <galak@kernel.crashing.org>,
<linuxppc-dev@lists.ozlabs.org>, <linux-crypto@vger.kernel.org>
Subject: Re: [PATCH v2 5/5] crypto: talitos: Add software backlog queue handling
Date: Thu, 5 Mar 2015 11:35:23 +0200 [thread overview]
Message-ID: <54F8235B.5080301@freescale.com> (raw)
In-Reply-To: <20150303182332.546523088b5891a776880c0f@freescale.com>
On 3/4/2015 2:23 AM, Kim Phillips wrote:
> On Tue, 3 Mar 2015 08:21:37 -0500
> Martin Hicks <mort@bork.org> wrote:
>
>> @@ -1170,6 +1237,8 @@ static struct talitos_edesc *talitos_edesc_alloc(struct device *dev,
>> edesc->dma_len,
>> DMA_BIDIRECTIONAL);
>> edesc->req.desc = &edesc->desc;
>> + /* A copy of the crypto_async_request to use the crypto_queue backlog */
>> + memcpy(&edesc->req.base, areq, sizeof(struct crypto_async_request));
>
> this seems backward, or, at least can be done more efficiently IMO:
> talitos_cra_init should set the tfm's reqsize so the rest of
> the driver can wholly embed its talitos_edesc (which should also
> wholly encapsulate its talitos_request (i.e., not via a pointer))
> into the crypto API's request handle allocation. This
> would absorb and eliminate the talitos_edesc kmalloc and frees, the
> above memcpy, and replace the container_of after the
> crypto_dequeue_request with an offset_of, right?
>
> When scatter-gather buffers are needed, we can assume a slower-path
> and make them do their own allocations, since their sizes vary
> depending on each request. Of course, a pointer to those
> allocations would need to be retained somewhere in the request
> handle.
Unfortunately talitos_edesc structure size is most of the times
variable. Its exact size can only be established at "request time", and
not at "tfm init time".
Fixed size would be sizeof(talitos_edesc).
Below are factors that influence the variable part, i.e. link_tbl in
talitos_edesc:
- whether any assoc / src / dst data is scattered
- icv_stashing (case when ICV checking is done in SW)
Still we'd be better with:
-crypto API allocates request + request context (i.e.
sizeof(talitos_edesc) + any alignment required)
-talitos driver allocates variable part of talitos_edesc (if needed)
instead of:
-crypto API allocates request
-talitos driver allocates talitos_edesc (fixed + variable)
-memcopy of the req.base (crypto_async_request) into talitos_edesc
both in terms of performance and readability.
At first look, the driver wouldn't change that much:
-talitos_cra_init() callback would have to set tfm.reqsize to
sizeof(talitos_edesc) + padding and also add the CRYPTO_TFM_REQ_DMA
indication in tfm.crt_flags
-talitos_edesc_alloc() logic would be pretty much the same, but would
allocate memory only for the link_tbl
I'm willing to do these changes if needed.
>
> Only potential problem is getting the crypto API to set the GFP_DMA
> flag in the allocation request, but presumably a
> CRYPTO_TFM_REQ_DMA crt_flag can be made to handle that.
Right. And this flag would apply only to request __ctx[].
Herbert, would this be an acceptable addition to crypto API?
Thanks,
Horia
next prev parent reply other threads:[~2015-03-05 9:35 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-03-03 13:21 [PATCH v2 0/5] crypto: talitos: Add crypto async queue handling Martin Hicks
2015-03-03 13:21 ` [PATCH v2 1/5] crypto: talitos: Simplify per-channel initialization Martin Hicks
2015-03-06 0:06 ` Kim Phillips
2015-03-03 13:21 ` [PATCH v2 2/5] crypto: talitos: Remove MD5_BLOCK_SIZE Martin Hicks
2015-03-06 0:07 ` Kim Phillips
2015-03-06 12:02 ` Herbert Xu
2015-03-03 13:21 ` [PATCH v2 3/5] crypto: talitos: Fix off-by-one and use all hardware slots Martin Hicks
2015-03-04 0:35 ` Kim Phillips
2015-03-04 14:46 ` Martin Hicks
2015-03-03 13:21 ` [PATCH v2 4/5] crypto: talitos: Reorganize request submission data structures Martin Hicks
2015-03-03 13:21 ` [PATCH v2 5/5] crypto: talitos: Add software backlog queue handling Martin Hicks
2015-03-04 0:23 ` Kim Phillips
2015-03-05 9:35 ` Horia Geantă [this message]
2015-03-06 0:34 ` Kim Phillips
2015-03-06 4:48 ` Herbert Xu
2015-03-09 12:08 ` Horia Geantă
2015-03-16 10:02 ` Horia Geantă
2015-03-17 0:19 ` Kim Phillips
2015-03-17 17:58 ` Horia Geantă
2015-03-17 22:03 ` Kim Phillips
2015-03-19 15:56 ` Horia Geantă
2015-03-19 18:38 ` Kim Phillips
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=54F8235B.5080301@freescale.com \
--to=horia.geanta@freescale.com \
--cc=galak@kernel.crashing.org \
--cc=herbert@gondor.apana.org.au \
--cc=kim.phillips@freescale.com \
--cc=linux-crypto@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=mort@bork.org \
--cc=scottwood@freescale.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).