Re: [RFC] Suggestion for the improvement of the buffering layer

[Jonathan Bastien-Filiatrault]

Nikos Mavrogiannopoulos wrote:
> Jonathan Bastien-Filiatrault wrote:
>
> > > Indeed but don't underestimate memcpy. Both for low latency and high
> > > speed links (especially with crypto accelerators) that could be the
> > > bottleneck. Keep that in mind in your design. In any case it doesn't
> > > look easy task but that's always in the eye of the beholder.
> > Agreed. You can check http://x2a.org/git/gnutls/?h=buffers-redux for
> > preliminary work. I have squashed _gnutls_io_write_buffered and down to
> > less than a screenful each. The mbuffers code is now handling partial
> > socket writes without problem (and regressions). The code is still
> > missing some comments, but you should find it clear after a little
> > while. The main benefit at this point is that the buffer logic is
> > abstracted away in the mbuffers code and is kept out of the code that
> > uses mbuffers.
> >
> > Your input is very important to me at this point.
>
> Hello Jonathan,
>  I like the overall change due to simplicity it introduces. Some comments:
Thanks !
> - You could use const gnutls_datum* to pass datums that will not be
> changed, just to save few bytes from stack.
No problem, I will do this.
> - I don't like the malloc for each mbuffer_st being added. I'd suggest
> to be avoided even if it results to some fixed number of mbuffers per
> session (which seems quite realistic requirement).
So do we just allocate a series of maximum-sized buffers (2^14+2048 for 
regular TLS and ~2^16 for DTLS) and keep a free list ? Do we keep a free 
list and realloc if we need more memory ? I do not want to reimplement 
malloc... Is the added complexity worth it ? In the general case, 
mbuffer chains should not be very long...
> Further I'd suggest to avoid the _gnutls_mbuffer_enqueue_copy() in
> io_write_buffered() by passing a datum to the latter as well as flag on
> whether this datum can be assigned without copy to an mbuffer. Thus in
> gnutls_record.c you could convert
>       ret = _gnutls_io_write_buffered (session, cipher, cipher_size);
>       gnutls_free (cipher);
> to something like:
>         gnutls_datum tmp = {cipher, cipher_size};
>         ret = _gnutls_io_write_buffered (session, &tmp, 1);
>         if (ret < 0) {
>                 gnutls_free(cipher);
>         }
Yes, I agree. I avoided this modification in order to keep the internal 
API constant for now. I will apply this modification.
> and will allow the reuse of the already allocated data.
>
> regards,
> Nikos

Thanks for your input,
Jonathan