[lwip-devel] Re: [task #7040] Work on tcp_enqueue

[Jakob Stoklund Olesen]

"address@hidden" <address@hidden> writes:
> [...] zero copy for TCP
> can only be done if a) the application has a callback that memory
> passed to tcp_enqueue is not used any more or b) the application
> passes pbufs to TCP, and is not allowed to re-use them -> the pbufs
> are for free use by TCP and are freed when they are ACKed.

Of course, thank you. TCP always has to hold on to the data somehow, and
sending pbufs is one way of doing that.

In my first attempt at zero-copy TCP, I used the non-copying version of
tcp_write. The adaptation layer code would hold on to the data until the
tcp_sent callback indicated that it was safe to release the memory.

It didn't work. I think I misunderstood the meaning of the len parameter
to the tcp_sent callback. I did something like this:

my_tcp_sent(arg, pcb, len)
{
  accu += len;
  if (accu >= first_buffer.len) {
    accu -= first_buffer.len
    free (first_buffer)
    first_buffer = next_buffer
  }
}

It worked most of the time, but TCP retransmissions caused bad stuff to
happen. It was either a bug in my code, or the sum of callback lengths
did not match what had been actually sent.

Has anybody used non-copy tcp_write with memory that must be reclaimed?
Can it be done?

I get the idea with passing pbufs to tcp_write. It would work. I would
prefer not to expose my application code to pbuf_alloc and friends, but
I am OK with paying the price of a single copy to acheive that.

> - the 'old' form of tcp_enqueue (takes a memory pointer) copies data
> into newly allocated pbufs, creates a checksum while copying (see task
> #6849/patch #6253) and is able to use one pbuf only per segment (the
> last can be disabled for SO_NODELAY to prevent waisting space)

Sounds good. The first step would be to split tcp_enqueue in two - data
and options. I will have a go at that. tcp_enqueue is not considered part
of the API, right? It is only in tcp.h because it is called from tcp.c,
tcp_in.c, and tcp_out.c.

As for checksumming: You wouldn't need to extend the pbuf with a
checksum field. For this case, the checksum field in the TCP header
could be kept up to date when a segment is created and extended. A
checksum field of 0xffff would tell tcp_output to calculate it. (That
value is never returned by inet_chksum_*)

Simon, do you have a checksumming memcpy? Is it part of a patch, I
missed? I am looking for the equivalent of:

u16_t
inet_chksum_memcpy(void *dst, const void *src, size_t len)
{
  memcpy(dst, src, len);
  return inet_chksum(src, len);
}

(Actually, the simple implementation would work pretty well for me. I
have data caches.)

> - a 'new' function can take pbufs and directly enqueue them in old (if
> not full) or new segments (to be called by a raw-only application: the
> application is responsible of using a pbuf that is optimal for the
> MAC; we could provide functions to allocate such pbufs)

This is reasonable. I would suggest creating a third tcp_enqueue_pbuf
variant.

The allocation stuff is tricky. Should the allocation depend on the
amount of space in the last unsent segment? Should a chain be allocated
if more than an MSS is requested?

Presumably, the application is going to generate data directly into an
allocated chain. That probably means some requirements on alignment of
both payload pointers and lengths.

> I don't think these changes would make the API too complicated: we
> would just have 2 forms of tcp_write: data pointer and pbuf. Of course
> there's still the question of how to handle options, but that's a
> minor one, I guess.

I agree. An extra tcp_write_pbuf is not too bad.

/stoklund