Re: [Qemu-devel] [PATCH v4 1/3] util/fifo8: implement push/pop of multiple bytes

[Peter Crosthwaite]

On Wed, Jan 29, 2014 at 4:48 AM, Beniamino Galvani <address@hidden> wrote:
> On Tue, Jan 28, 2014 at 10:43:28AM +0000, Peter Maydell wrote:
>> On 28 January 2014 00:04, Peter Crosthwaite
>> <address@hidden> wrote:
>> > On Tue, Jan 28, 2014 at 4:32 AM, Peter Maydell <address@hidden> wrote:
>> >>>  void fifo8_reset(Fifo8 *fifo)
>> >>>  {
>> >>>      fifo->num = 0;
>> >>> +    fifo->head = 0;
>> >>
>> >> This is a bug fix, right? It should go in its own patch.
>> >>
>> >
>> > No bug - where the ring buffer starts following a reset is undefined
>> > and need not be defined. But it improves the predicatability of the
>> > newly added pop_buf fn as you can now following a reset, guarantee
>> > that a single pop_buf will take all contents if its the first pop
>> > (which is how its being used in P2).
>>
>> True. I still think it should have its own patch (and
>> indeed it would be worth saying what you just did as
>> part of the commit message for that patch...)
>
> Ok, I will move the change to a new patch.
>
>> I think it's also nicer for any state that gets migrated
>> to be reset cleanly.
>
> Do you mean also the buffer content? In the emac the tx fifo gets
> reset after each transmission. Isn't this too costly?
>

I would say yes, it is too costly. Out-of-range buffer content is
undefined and there is no good reason that I can see to memset to 0 at
the cost of performance. If we are worried about stale information
leak via VMSD images or anything wierd like that then it should be
handled pre-save and post-load - not here in reset() which is
potentially a fast path.

However resetting the head pointer as you have done in this change is
a win-win, as it's both cleaner (as per Peter's argument) as well as
increasing performance for your use case.

Regards.
Peter

> Beniamino
>