[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 1/3] util/fifo8: implement push/pop of multip
From: |
Peter Crosthwaite |
Subject: |
Re: [Qemu-devel] [PATCH v4 1/3] util/fifo8: implement push/pop of multiple bytes |
Date: |
Wed, 29 Jan 2014 22:02:14 +1000 |
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
>
[Qemu-devel] [PATCH v4 3/3] hw/arm/allwinner-a10: initialize EMAC, Beniamino Galvani, 2014/01/26