[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