Re: [PATCH] PPP: Optimize write buffer management

[Patrick Porlan <patrick.porlan@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 1786 bytes --]

Hi Denis,

On Wed, 2011-03-02 at 09:28 -0600, Denis Kenzior wrote:

> >> Is there a particular reason why you chose to use a ring buffer of ring
> >> buffers? A simple GQueue might be much easier to understand.  If you are
> >> worried about 'infinite queuing' then a simple counter might help to
> >> alleviate that.

I just sent a new version of my patch that does it (forgot to tag it as
v2, sorry).


> >> You change BUFFER_SIZE from 2048 to 4096 and remove the multiplication
> >> here.

Having a constant named BUFFER_SIZE whose value is half a buffer size
did not seem right though. I reinstantated the multiplication, but at
the macro definition level.


> >> So I think we have to be a bit careful here.  HDLC framing can in theory
> >> (if you're maximally unlucky) result in doubling of the data size once
> >> it is framed.  This means that we might have enough space in the current
> >> buffer according to this estimate, but still exceed it once the actual
> >> framing is performed.  If so, then we have to drop the frame.

I checked with random data, and get some frames dropped with a 128
margin, but none with 256. Using a much larger margin might be
detrimental to memory usage, so I chose to leave it like this. We can
address this in a further patch if you wish.


> Yes, we definitely want to have some sort of buffer pool management
> strategy to share a pool of buffers between different entities (e.g.
> GAtMux, GAtServer, GAtHDLC).  Have you looked at g_slice* inside glib
> yet?  I think we can take advantage of the fact that most of our ring
> buffers are sized at 4k.

It looks like this allocator should be used in ringbuffer.c. Let's
discuss that and follow up with another patch.

Regards,

-- Patrick