Re: [PATCH] PPP: Optimize write buffer management

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

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

Hi Denis,

On Tue, 2011-03-01 at 21:47 -0600, Denis Kenzior wrote:
> This is a great description, but right after reading it one should
> realize that this patch is better broken down into two.  The first patch
> addressing the ringbuffer performance improvements and the second one
> dealing with PPP/HDLC queuing.  In fact, if there were two patches, then
> ringbuffer changes could go in right away.

Fair enough ; I'll separate the changes.


> Please try to avoid going over 80 characters a line.  Sometimes it is
> unavoidable, but lets try very hard to avoid it.

Understood.


> 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.

Well, this implementation opens the door to further optimizations, such
as reusing cells (buffers) to avoid frequent allocation/deallocation.


> You change BUFFER_SIZE from 2048 to 4096 and remove the multiplication
> here.  It is a good idea to send these types of changes in a separate
> patch for two reasons:
> 	- Logically they should be separate
> 	- It is much easier to review for correctness outside the context of a
> large patch

Got it. By the way, why is it written this way? Is it to stress that we
want space for two frames?


> Please watch out for space / tab indentation mixups. 

OK. I'll fix that.


> 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.
> 
> There are two possibilities:
> 	- Retry again with a full buffer this time
> 	- Always pick a buffer if we have less than 2x size available
> 

Thanks for the insight. Picking a new buffer may be best. That will
cause more buffer "run away" though, strengthening the case for better
buffer recycling strategies.

Regards,

-- Patrick