> -Original Message-
> From: David Miller
> Sent: Thursday, September 20, 2018 11:49 PM
> To: Vakul Garg
> Cc: netdev@vger.kernel.org; bor...@mellanox.com;
> avia...@mellanox.com; davejwat...@fb.com; doro...@fb.com
> Subject: Re: [PATCH net-next] net/tls: Add support for async encryption of
> records for performance
>
> From: Vakul Garg
> Date: Wed, 19 Sep 2018 20:51:35 +0530
>
> > This patch enables encryption of multiple records in parallel when an
> > async capable crypto accelerator is present in system.
>
> This seems to be trading off zero copy with async support.
>
> Async crypto device support is not the common case at all, and synchronous
> crypto via cpu crypto acceleration instructions is so much more likely.
>
> Oh I see, the new logic is only triggered with ASYNC_CAPABLE is set?
>
> > +static inline bool is_tx_ready(struct tls_context *tls_ctx,
> > + struct tls_sw_context_tx *ctx)
> > +{
>
> Two space between "inline" and "bool", please make it one.
Fixed.
Seems checkpatch misses it.
>
> > static void tls_write_space(struct sock *sk) {
> > struct tls_context *ctx = tls_get_ctx(sk);
> > + struct tls_sw_context_tx *tx_ctx = tls_sw_ctx_tx(ctx);
>
> Longest to shortest line (reverse christmas tree) ordering for local variable
> declarations please.
Can't do this. The second variable assignment is dependent upon previous one.
> >
> > + list_for_each_prev(pos, >tx_ready_list) {
> > + struct tls_rec *rec = (struct tls_rec *)pos;
> > + u64 seq = be64_to_cpup((const __be64 *)>aad_space);
>
> Likewise.
>
I can split variable declaration 'seq' and its assignment into two separate
lines.
But I am not sure if increasing number of lines in order to comply reverse
Christmas tree
is a good thing for this case.
> > -static int tls_do_encryption(struct tls_context *tls_ctx,
> > +int tls_tx_records(struct sock *sk, int flags) {
> > + struct tls_rec *rec, *tmp;
> > + struct tls_context *tls_ctx = tls_get_ctx(sk);
> > + struct tls_sw_context_tx *ctx = tls_sw_ctx_tx(tls_ctx);
> > + int rc = 0;
> > + int tx_flags;
>
> Likewise.
Could partially address since ctx assignment depends upon tls_ctx assignment.
>
> > +static void tls_encrypt_done(struct crypto_async_request *req, int
> > +err) {
> > + struct aead_request *aead_req = (struct aead_request *)req;
> > + struct sock *sk = req->data;
> > + struct tls_context *tls_ctx = tls_get_ctx(sk);
> > + struct tls_sw_context_tx *ctx = tls_sw_ctx_tx(tls_ctx);
> > + struct tls_rec *rec;
> > + int pending;
> > + bool ready = false;
>
> Likewise.
Placed 'ready' above pending 'pending'. Rest unchanged because of dependencies.
>
> > +static int tls_do_encryption(struct sock *sk,
> > +struct tls_context *tls_ctx,
> > struct tls_sw_context_tx *ctx,
> > struct aead_request *aead_req,
> > size_t data_len)
> > {
> > int rc;
> > + struct tls_rec *rec = ctx->open_rec;
>
> Likewise.
>
> > @@ -473,11 +630,12 @@ static int memcopy_from_iter(struct sock *sk,
> > struct iov_iter *from, {
> > struct tls_context *tls_ctx = tls_get_ctx(sk);
> > struct tls_sw_context_tx *ctx = tls_sw_ctx_tx(tls_ctx);
> > - struct scatterlist *sg = ctx->sg_plaintext_data;
> > + struct tls_rec *rec = ctx->open_rec;
> > + struct scatterlist *sg = rec->sg_plaintext_data;
> > int copy, i, rc = 0;
>
> Likewise.
Can't change because of dependencies.
>
> > +struct tls_rec *get_rec(struct sock *sk) {
> > + int mem_size;
> > + struct tls_context *tls_ctx = tls_get_ctx(sk);
> > + struct tls_sw_context_tx *ctx = tls_sw_ctx_tx(tls_ctx);
> > + struct tls_rec *rec;
>
> Likewise.
>
Declared 'mem_size' below 'rec'.
> > @@ -510,21 +707,33 @@ int tls_sw_sendmsg(struct sock *sk, struct
> msghdr *msg, size_t size)
> > int record_room;
> > bool full_record;
> > int orig_size;
> > + struct tls_rec *rec;
> > bool is_kvec = msg->msg_iter.type & ITER_KVEC;
> > + struct crypto_tfm *tfm = crypto_aead_tfm(ctx->aead_send);
> > + bool async_capable = tfm->__crt_alg->cra_flags &
> CRYPTO_ALG_ASYNC;
> > + int num_async = 0;
> > + int num_zc = 0;
>
> Likewise.
Fixed
> > @@ -661,6 +904,8 @@ int tls_sw_sendpage(struct sock *sk, struct page
> *page,
> > struct scatterlist *sg;
> > bool full_record;
> > int record_room;
> > + struct tls_rec *rec;
> > + int num_async = 0;
>
> Likewise.
Fixed.
Sending v2.
