Hi Evgenly,

See inline...

-----Original Message-----
From: Evgeniy Polyakov [mailto:[EMAIL PROTECTED] 
Sent: Friday, October 24, 2008 2:08 PM
To: Shasi Pulijala
Cc: Herbert Xu; linux-crypto@vger.kernel.org; Loc Ho
Subject: Re: [PATCH 1/1 v7] Add Crypto API User Interface Support

Hi.

On Fri, Oct 24, 2008 at 01:54:23PM -0700, Shasi Pulijala ([EMAIL PROTECTED]) 
wrote:
> This patch v7 includes a few coding style changes and benchmark comparison 
> between for tcrypt and cryptodev. These are just a rough estimate and not the 
> exact numbers for cbc-aes. CryptoDev interface will always be slight more as 
> it includes crypto setup and a few other housekeeping tasks. Tcrypt is just 
> the time for the encrypt or decrypt function calls only.
> 
>               Tcrypt  Cryptodev
> 16 byte       47us            132us
> 32 byte       51us            141us
> 64 byte       68us            150us

Can you also run cryptodev under profiler to determine, why it is, well,
noticebly, slower than tcrypt?

[Loc Ho]
We will look at profiling but in either case cryptodev will always takes longer 
as tcrypto only measure the encrypt/decrypt function. Cryptodev has to setup 
the operation as well as other house cleaning.

> With regard to forward declaration, they are needed. 

Why? Can't you reorder functions to eliminate that?
[Loc Ho]
We will look again.

> >> +          }
> >> +          sg_set_page(&sg[x], page, bufsize, offset);
> >> +          rop = PAGE_SIZE - sg[x].offset;
> >> +          if (bufsize > rop) {
> >> +                  sg[x].length = rop;
> >> +                  bufsize = bufsize - rop;
> >> +          }
> >> +          offset = 0;
> >>
> >>What if bufsize is smaller than 'rop', but there are several pages?
> >>This will initialize wrong buffers, but probably there is a check somewhere 
> >>above this layer.
> 
> Yes, there is a check above for the number of pages returned. It returns an 
> error if the number of pages returned is greater/less than requested.
> 
> With regard to "Still this is not an async crypto," it is asynchronous 
> interface if called via AIO. For vector write, it will turn into synchronous 
> AIO within the kernel AIO framework.

It does not mean it is async, since crypto processing itself is
synchronous. I.e. you can submit requests asynchronously, but they are
processed by the crypto code one-by-one with waiting after each request.
This actually can explain your numbers: instead of having flow of
requests, you have to do really lots of work per-request. I do not say
this is wrong (although imho it should be done differently), since it is
your code and likely it fits your needs.

[Loc Ho]
I don't understand what you are referring to here. Let's assume that operation 
is submitted synchronously to cryptodev, cryptodev in turn will submit to the 
underlying driver. If the underlying driver is software based, it will 
completed synchronous and return without calling wait_for_completion. If the 
underlying driver is hardware based or asynchronous, it will wait for 
completion via the callback function signaling the event wait object. Now, 
let's assume that operation is submitted asynchronously to cryptodev, crypto in 
turn will submit to the underlying driver. If the underlying driver is software 
based and completed, it just return. If the underlying driver is asynchronous 
such as hardware, it will return immediately without waiting. CryptoDev will 
call the AIO callback function when the crypto driver call cryptodev callback 
function. Therefore, what is the issue?

As regard to performance, asynchronous only help performance if there are a 
large number request (which implement hardware support).


Plus, you did not figure what happens when request completion is
interrupted with regard to freeing data, which may be accesed by the
crypto code in parallel.

[Loc Ho]
We looked at this. There is reference count that will prevent delete of the 
request context. Apparently, we missed this. We will fix this.


> +     aead_request_set_crypt(req, ssg, dsg, bufsize, cop->iv);
> +     aead_request_set_assoc(req, &adata, cop->assoc_len);
> +
> +     atomic_inc(&ses_ptr->refcnt);
> +
> +     if (cop->eop == COP_ENCRYPT)
> +             ret = crypto_aead_encrypt(req);
> +     else
> +             ret = crypto_aead_decrypt(req);
> +
> +     switch (ret) {
> +     case 0:
> +             if (!iocb)
> +                     atomic_dec(&result->opcnt);
> +             break;
> +     case -EINPROGRESS:
> +     case -EBUSY:
> +             if (iocb) {
> +                     CDPRINTK(2, KERN_INFO,
> +                             "Async Call AEAD:Returning Now\n");
> +                     return -EIOCBQUEUED;
> +             }
> +             ret = wait_for_completion_interruptible(
> +                                     &result->crypto_completion);
> +             if (!ret)
> +                     ret = result->err;
> +             if (!ret) {
> +                     INIT_COMPLETION(result->crypto_completion);
> +                     break;
> +             }

I.e. let's suppose it was interrupted here, while crypto driver performs
a processing on given pages.

> +             /* fall through */
> +     default:
> +             printk(KERN_ERR PFX "sid %p enc/dec failed error %d\n",
> +                     ses_ptr, -ret);
> +             if (!iocb)
> +                     atomic_dec(&result->opcnt);
> +             break;
> +     }
> +
> +     if (nopin && !ret) {
> +             if (copy_to_user(dst, data, enc ? bufsize + authsize :
> +                                             bufsize - authsize))
> +                     printk(KERN_ERR PFX
> +                             "failed to copy encrypted data "
> +                             "to user space\n");
> +             CD_HEXDUMP(data, enc ? bufsize + authsize :
> +                                     bufsize - authsize);
> +     }
> +
> +     /* Check if last reference */
> +     if (atomic_dec_and_test(&ses_ptr->refcnt))
> +             cryptodev_destroy_session(ses_ptr);
> +     if (dst_flag)
> +             cryptodev_release_pages(result->dpages, nr_dpages);
> +out_spages:
> +     cryptodev_release_pages(result->spages, nr_spages);

Now you have freed pages, which are still accessible by the request
crypto processing code somewhere in the underlying driver.

-- 
        Evgeniy Polyakov
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to