Re: [Qemu-devel] [PATCH for-2.1 v2 0/2] Generalise FIFO to more integer types

[Peter Crosthwaite]

On Wed, Apr 9, 2014 at 7:22 AM, Don Slutz <address@hidden> wrote:
>
> On 04/08/14 05:10, Markus Armbruster wrote:
>>
>> Peter Crosthwaite <address@hidden> writes:
>>
>>> On Tue, Apr 8, 2014 at 5:14 PM, Markus Armbruster <address@hidden>
>>> wrote:
>>>>
>>>> Peter Crosthwaite <address@hidden> writes:
>>>>
>>>>> There is a utility helper for dealing with 8 bit fifos. This should be
>>>>> applicable to other integer widths as well. These two patches
>>>>> generalise this FIFO to work for 16, 32 and 64 bit ints.
>>>>
>>>> Can you show us a user for the wider FIFOs?
>>>>
>>> Hi Markus,
>>>
>>> I have a couple out of tree that are incomplete and a bit out of scope
>>> of this series. Rather not wait and send a complicated series spanning
>>> multiple sub-systrems. I guess I could shop around for an easy lead
>>> example to fix, but does the series stand in its own right?
>>
>> I don't like infrastructure without an in-tree user.  Even if it's
>> "only" a sensible, straightforward generalization of existing
>> infrastructure.
>>
>> Infrastructure without a user tends to rot.  Probably not a serious
>> issue in this particular case.
>>
>> The cost of applying a change and maintaining the additional complexity
>> needs to be justified by some gain.  Even when the costs are small, like
>> they probably are in this particular case.  An in-tree user could
>> probably justify easily.
>
>
> I am not sure that the run time cost is the best.  Looks more like c++
> templates should be done via macros.  Just like
> include/exec/softmmu_header.h:
>
>
> void glue(glue(fifo, 8), _create)(Fifo *fifo, uint32_t capacity);
> void glue(glue(fifo, 8), _push)(Fifo *fifo, glue(glue(uint, 8), _t) data);
> ...
>

So I did actually start out with a glue based implementation of this
but thought the better of it. It's reasonably ugly in general and the
VMSD stuff was trickiest. There are however some possible compromises
when we consider only using glue for the hottest paths. E.g. I think
push and pop are both red-hot as they tend to happen once per byte.
The buffer pushes/pops not so much - they are more a once-per-packet.
So if we glue just the single push/pop we might be in a good place.
reset/create etc is all pretty cold. occupancy checkers (is_full) etc
are unaffected perf wise by the common implementation.

> Also that would mean not doing the patch #1 at all.
>

If we went with my hot-path-only compromise I think we would alias the
create function as such to keep the namespaces consistent:

static inline void fifo_create8(Fifo *fifo, uint32_t cap) {
    fifo_create(fifo, cap, 8);
}

But I think I want to s/fifo8_pop/fifo_pop8 etc though as the fifo8
type wouldnt really exist anymore and this would be more consistent
with other api that have 8/16/32/64 bit variants of their functions.
So the P1 change pattern has a place. Reset/is_full/is_empty would all
remain as in this series.

Regards,
Peter

> That way all the cost is at compile time, and only the versions currently
> used are generated.
>
>    -Don Slutz
>