Of course the current approach has the downside that MEM_USE_POOLS diverts the whole heap to pools, not only PBUF_RAM. Maybe we should review this... ?

I think the mem_malloc in dhcp can be easily removed by creating a netif_dhcp which has a netif and definition of struct dhcp added.  The allocation looks to be a one time event but I have not dug that deep into the code.  In dhcp_inform, it's allocated and freed - maybe that struct dhcp can be stack based?

> That's not true! The first changes I made to tcp_in.c was in
> version 1.65 of the file. But the line 'pcb->snd_queuelen -=
> pbuf_clen(next->p);' was already present in tcp_receive() before
> that version!

Bizarre - I had used cvs annotate before. I must have just misread it, sorry. Doesn't make a difference to what we do going forward fortunatley.

> As for the macro, I agree. Perhaps we can make the function use
> that macro so that we don't have the same code twice.

Indeed.

> Ah, OK. But there are two problems with counting the segments:
> - you have no control of how many pbufs you need to configure

That is true.

> - snd_queuelen is decremented using pbuf_clen (because when
> sending with copy=0, a segment will consists of one PBUF_REF for
> the data and one PBUF_RAM for the header) so it must be
> incremented using pbuf_clen also, I think.

Before your changes it wasn't decremented using pbuf_clen either.

But I guess if pbuf_clen becomes a macro, it's not worth worrying about - the only problem being now that I'm not sure it can be made a macro. Perhaps we need a different name for the macro version:

#define pbuf_clen_p( p, c ) \
{                           \
  struct pbuf *_p = (p);    \
  u8_t _len = 0;            \
  while (_p != NULL) {      \
    ++_len;                 \
    _p = _p->next;          \
  }                         \
  (*c) = _len;              \
}

>> So perhaps is it ok for it to just represent the length of the
>> queue, rather than being an accurate count of the number of
>> pbufs?
>
> I don't really get what you mean here,

what I was meaning is that the specific number of pbufs limited by TCP_SND_QUEUELEN may not be that important. I might be wrong, but it seems to me that TCP_SND_QUEUELEN is just there to act as a limit to the queue getting too long. At present it looks like instead of counting pbufs, it is counting segments. But is this actually a problem?

Maybe we should just change the comments in tcp_out.c and opt.h!

I'm prepared to be corrected on this though.

Re comment #17: Only raw API applications would be affected. anything using the socket API or netbuf_* functions of the sequential API would be fine. But even then, I think it's always been the case that we should have been able to handle chained PBUF_RAMs, and the issue in tcp_enqueue falls into the category of a bug, not a feature ;).

Re comment #16 pbuf_clen() versus new pbuf_count.
Firstly be aware that adding one-byte to struct pbuf would probably in practice add 4 for most people, due to alignment constraints (assuming 32-bit ints).

I think most chains will be short enough that pbuf_clen() isn't much of an issue - the biggest thing would be the function call overhead itself, and to be honest, pbuf_clen() is so short it should probably be made a function-like macro anyway.

OTOH, I'm not sure there is anything magic about the queue len. Correct me if I'm wrong, but it doesn't appear to represent anything other than a contrived view of "how long the queue is". The value that really represents something is the number of bytes in the queue. So perhaps is it ok for it to just respresent the length of the queue, rather than being an accurate count of the number of pbufs?

> I see no reason why we shouldn't allow chained PBUF_RAMs, so the assertions are probably a good idea.

I still have doubts on this since I think this could affect existing applications that are built on the assumption that PBUF_RAM pbufs are not chained. Maybe we have to implement a new type for this? PBUF_RAM_NOCOPY could be the right type for this.

I see no reason why we shouldn't allow chained PBUF_RAMs, so the assertions are probably a good idea.  However, i don't understand why pbuf_clen() is insufficient to count the length of such a chain.

Some changes are necessary to make tcp work with chained pbufs:

- introcude a 'pbuf_count' counter in struct tcp_seg and increment it when chaining 2 segments together for output using pbuf_cat (pbuf_cat(useg->p, queue->p); in tcp_enqueue; around line 342).

- this counter is then used for 'pcb->snd_queuelen -= next->pbuf_count;' instead of 'pcb->snd_queuelen -= pbuf_clen(next->p);' (pbuf_clen(next->p) had the assumption of one pbuf per tcp_enqueue-call)

This is both compatible with chained PBUF_RAM pbufs and faster than the current version since pbuf_clen() doesn't have to be called. Of course at the cost of one u8_t per segment.

Should I check this in or am I the only one interested in this 'bug' (I'd prefer to call it a 'task' though)?

What I meant was maybe there are some corner cases when a PBUF_RAM is allocated and it not fails that the code allocating it expects it to be in one piece. The parts of the stack that can deal with PBUF_REF or PBUF_POOL can cope with chains, of course!

[I'm working on a mem to memp implementation (that is written in ANSI C; and I'm short of time at the moment so it could take a while).]

>If an application sends and receives a lot of small packets (text-based communication, for example) with an occasional large packet, the pools can be configured to be small and pbuf chaining takes care of the large packets.

That would imply that PBUF_RAM packets are not guaranteed any more to be one piece of memory. That would be a good option for low memory targets, though. But I think the stack is not yet ready to handle pbuf chains everywhere...

Also, we might need some kind of algorithm when to use a bigger buffer and waste memory and when to use 2 smaller ones. And we would need some kind of configuration options to use the 'one-piece' or chained method.

Oh, I forgot to comment about the buddy allocator. It was me you mentioned that one to, some half a year ago... But I must say I'm not really satisfied with that. While it gives less fragmentation than our current heap implementation, it's still worse than our pool-implementation. And since we are not that low on RAM, we've chosen to stay with our pools ;-)

Yes, I was slightly more concerned about Atte's suggestion of making everything come from pools.

I half recall mentioning this somewhere before, but I'm not sure. But one fast and compact way to implement power of two buffers is a buddy allocator (see http://en.wikipedia.org/wiki/Buddy_memory_allocation if you don't know what I'm referring to, or know it by a different name).

So rather than having, say, three pools for three sizes of pbufs, you have a single pool, but can efficiently split up and coalesce pbufs. The fact we can chain pbufs makes this even better. So we can satisfy a 64K allocation with a 64K pbuf, or two 32K pbufs chained together. Of course ethernet frames are not (usually) a power of two, and although a 1024+512 chain would be close, it's probably better to keep using a separate pool for that special case.

It's just one possibility anyway.

Since you've already done the work.... Does your rewritten mem.c have the precise same API as the present mem.c? Is it fairly self-contained? Would it be possible to have it in the repository named perhaps as mem-alt.c (for alternative) and users can choose which they build?

By having multiple separate pools, you increase overall RAM memory use - which is far more of an issue than code footprint for most embedded systems. It's inevitable you will sometimes run out of memory in one pool while still having memory in another, which is wasteful. And pools of fixed chunks which don't fit the allocations will intrinsically have waste. Statistical analysis may reduce some but not all of the overhead, and only really helps "average" operation.

If you want to be memory efficient, you should only use pools of fixed size memory chunks for fixed size allocations. With fixed size allocations that is when an implementation like the current memp one becomes efficient, rather than wasteful as Atte states: users can fine-tune precisely the resources to match the number of TCP connections (pcbs), in-flight TCP segments, etc. they intend to support. They are tuning lwIP so that it will meet their requirements, rather than statistically assume that sometimes it will, sometimes it may not, because you gave it a fairly arbitrarily chosen lump of memory to play with. Otherwise it can mean that "low priority" allocations can use up memory for high priority ones - dropping a packet can be recovered from far more easily than no longer being able to create new outbound connections. The point is, the user chooses how to divide the resources according to the application requirements.

No, it may not be as fast, but lwIP's primary goal is low memory use. Speed is secondary.

If people want this type of solution then I think it must be a configurable option. With what Leon describes, achieving that wouldn't be so hard - he's only referring to pbuf allocations, so that's all fairly self-contained. Trying to adopt fixed size pools for all pbufs permanently, or for variable sized quantities more generally (although it is true that there aren't many non-pbuf variable sized allocations around), or using wrongly sized pools for fixed size quantities, would be bad. 

I'm using lwIP on systems with 32K RAM and I believe should get into 16K for some people with simpler applications. I strongly suspect Atte and Simon have significantly more memory. I think it would be a step in the wrong direction to increase memory overheads for everyone.

This one would be of great interest for me, as I think embedded systems running for a long time should not have a heap if memory is allocated ADN deallocated often with different sizes. (And our devices have to run a looong time, ~30 years is our goal)

Implementing fully pooled pbufs would be one step into that direction.
BUT: I think this would only make sense if mem_malloc() could be eliminated for the rest of the core code. Since this is mainly dhcp and snmp, I hope introducing new memp pools for those would be enough.

This could also solve the issue where no pbufs were free to acknowledge tcp segments (which again would free some tcp enqueued pbufs). This was an error report somewhere, though I don't remeber it.

I'm not planning to take on this right now, just wanted to know your opinions.