Re: [PATCH 1/4] sha1sum: use AF_ALG when available

[Tim Rühsen]

On 23.04.2018 18:52, Matteo Croce wrote:
> On Mon, Apr 23, 2018 at 5:07 PM, Tim Rühsen <address@hidden> wrote:
>> On 04/23/2018 01:17 PM, Matteo Croce wrote:
>>> +#include <config.h>
>>> +
>>> +#include "af_alg.h"
>>> +
>>> +/* from linux/include/linux/fs.h: (INT_MAX & PAGE_MASK) */
>>> +#define MAX_RW_COUNT 0x7FFFF000
>>> +#define BLOCKSIZE 32768
>>> +
>>> +int
>>> +afalg_stream (FILE * stream, void *resblock, const char *alg, int hashlen)
>>> +{
>>> +  struct sockaddr_alg salg = {
>>> +    .salg_family = AF_ALG,
>>> +    .salg_type = "hash",
>>> +  };
>>> +  int ret, cfd, ofd;
>>> +  static char buf[BLOCKSIZE];
>>> +  ssize_t size;
>>> +  struct stat st;
>>> +
>>> +  strcpy((char *)salg.salg_name, alg);
>> Better consider for size of salg.salg_name here.
>>
> Right. Will check it in v2.
>
>>> +  cfd = socket (AF_ALG, SOCK_SEQPACKET, 0);
>>> +  if (cfd < 0)
>>> +      return -EAFNOSUPPORT;
>> What about moving salg initialization here ?
>>
> I think that contextual initialization it's a good trick to fill it
> with zeroes avoiding a memset later.
> What are the advantage of initializing the structure later? The effort
> should be minimal in case of no AF_ALG support.
You are right with the minimal overhead for a single call, but you can
never be sure about corner use cases.
The contextual init is good (though I remember gcc clears all the memory
anyways - so no benefit by no using memset),
and you can also use it after the first code line (C99, which th
econstruct already is).

>>> +  ret = bind (cfd, (struct sockaddr *) &salg, sizeof (salg));
>>> +  if (ret < 0)
>>> +    {
>>> +      ret = -EAFNOSUPPORT;
>>> +      goto out_cfd;
>>> +    }
>>> +
>>> +  ofd = accept (cfd, NULL, 0);
>>> +  if (ofd < 0)
>>> +    {
>>> +      ret = -EAFNOSUPPORT;
>>> +      goto out_ofd;
>>> +    }
>>> +
>>> +  /* if file is a regular file, attempt sendfile() to pipe the data */
>>> +  if (!fstat(fileno(stream), &st) && S_ISREG(st.st_mode) &&
>>> +      st.st_size <= MAX_RW_COUNT)
>> Is it possible to skip that MAX_RW_COUNT check on recent glibc ?
>> From 'man sendfile':
>> "
>> The original Linux sendfile() system call was not designed to handle
>> large file offsets.  Consequently, Linux 2.4 added
>> sendfile64(), with a wider type for the offset argument.  The glibc
>> sendfile()  wrapper  function  transparently  deals
>> with the kernel differences.
>> "
>>
> The advantage of sendfile64() over sendfile() is only that the offset
> in the input file can be bigger.
> I've tried it and it seems that MAX_RW_COUNT is an hard limit on every system:
>
> $ strace -e read dd if=/dev/zero of=/dev/null bs=3G count=1
> read(0, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"...,
> 3221225472) = 2147479552
> 0+1 records in
> 0+1 records out
> 2147479552 bytes (2.1 GB, 2.0 GiB) copied, 0.221906 s, 9.7 GB/s

Thanks for testing - I didn't read the man page thoroughly (or
misunderstood it).
> As reported in the manpage too:
>
> "sendfile() will transfer at most 0x7ffff000 (2,147,479,552) bytes,
> returning the number of bytes actually transferred.  (This is true on
> both 32-bit and 64-bit systems.)"
>
>>> +    {
>>> +      if (sendfile(ofd, fileno(stream), NULL, st.st_size) == -1)
>> From 'man sendfile':
>> "
>> Note  that  a  successful  call  to
>> sendfile()  may  write fewer bytes than requested; the caller should be
>> prepared to retry the call if there were unsent
>> bytes.  See also NOTES.
>> "
>>
>>> +        ret = -EIO;
>>> +      else
>>> +        ret = 0;
>>> +    } else {
>>> +      /* sendfile() not possible, do a classic read-write loop */
>>> +      while ((size = fread (buf, 1, sizeof (buf), stream)))
>>> +        {
>>> +          if (send (ofd, buf, size, size == sizeof (buf) ? MSG_MORE : 0) 
>>> == -1)
>>> +            {
>>> +              ret = -EIO;
>>> +              goto out_ofd;
>>> +            }
>>> +        }
>>> +  }
> I might be wrong here, but looking at the kernel code of the
> sendfile64(), which actually calls do_splice_direct() that calls
> splice_direct_to_actor(),
> it can happen only with nonblocking sockets. I didn't managed to
> trigger such behaviour myself, but more testing is welcome.
>
>> Regards, Tim
>>
> Regards,
Regards, Tim