Re: [lwip-devel] pbuf_chain followed by pbuf_free

[Jani Monoses]

Hi

> Hmm, I would prefer not changing the API and be suboptimal. Although,
> did you look into which of the pbuf_chain()s are most often used,
> those with or without _free()?

well this happens on most pbuf_chain uses as I said, I noticed the one
in tcp_out.c first.

> I think we can only optimize lwIP after a decent amount of profiling.
> 
> Anyway, it looks like your proposed change can be implemented
> non-intrusively in a step-by-step approach:
> 
> 1 - implement pbuf_cat() as a copy of pbuf_chain(), without increment
> tail's reference count.
> 2 - make pbuf_chain() do  pbuf_ref(tail); pbuf_cat(head,tail);
> 3 - change the pbuf_chain()/free() combinations (which now have three
> locks!) into pbuf_cat()
> in the lwIP stack as well as the contributions.
> 
> I am strongly against having both a pbuf_chain() and pbuf_cat() full
> implementation, for
> code size and maintenance reasons.

:)

this is exactly what I proposed in my mail: 'pbuf_chain would then be
pbuf_cat followed by pbuf_ref' . Of course I don't want duplicate
implementation, nor changing the API. pbuf_chain would keep the current
behaviour and the more optimal pbuf_cat would be used when we don't need
to unnecessarily inc/dec the refcount.


> Also, I do not think the names are more clear in your proposal. They
> do not indicate that
> the caller of pbuf_cat() may no longer reference the tail pbuf.

For what it's worth the current pbuf_XX names are not self-descriptive
either, but I can name pbuf_cat whatever else anybody suggests. It just
concatenates two pbuf chains without refcount touching.

> This is much clearer in the current situation, where the caller
> explicitly calls pbuf_free(tail)
> and where the programmer is enforced not to reference tail afterwards.

Could you explain this? How is he forced not to reference tail? tail is
not set to NULL, it is still a valid pbuf with a refcount of at least 1.
 
> Both ways are fine with me. We do not use the pbuf locks anyway, we
> use a coarser

Even if not for the locks doing a pbuf_cat seems clearer to me: it says
we append this pbuf. Whereas now : we chain them then decrease the
refcount because chain has the effect of increasing it. If we had a
pbuf_unref instead of pbuf_free that would be self_descriptive, but
still not as optimal as not doing the ref thing at all.

So if you agree (and I suppose you do since you proposed what I
intended to express in my first mail I'll do the change)
OK?

Jani