On 29 March 2017 at 03:55, Brian Brooks <[email protected]> wrote: > On 03/28 19:18:37, Bill Fischofer wrote: >> <html><head><meta http-equiv="Content-Type" content="text/html; >> charset=utf-8"> > > It is infinitely better to do patch review in plain text rather > than HTML. I thought this was a plain text mailing list? > >> <meta name="Generator" content="Microsoft Exchange Server"> >> <!-- converted from text --> >> <style><!-- .EmailQuote { margin-left: 1pt; padding-left: 4pt; border-left: >> #800000 2px solid; } --></style></head> >> <body> >> <font size="2"><span style="font-size:10pt;"><div class="PlainText">On Tue, >> Mar 28, 2017 at 2:23 PM, Brian Brooks <[email protected]> wrote:<br> >> > Signed-off-by: Brian Brooks <[email protected]><br> >> > ---<br> >> > include/odp/api/spec/queue.h | 5 +++++<br> >> > 1 file changed, 5 insertions(+)<br> >> ><br> >> > diff --git a/include/odp/api/spec/queue.h >> b/include/odp/api/spec/queue.h<br> >> > index 7972feac..1cec4773 100644<br> >> > --- a/include/odp/api/spec/queue.h<br> >> > +++ b/include/odp/api/spec/queue.h<br> >> > @@ -124,6 +124,11 @@ typedef struct odp_queue_param_t {<br> >> > * the queue >> type. */<br> >> > odp_queue_type_t >> type;<br> >> ><br> >> > + /** Queue size<br> >> > + *<br> >> > + * Indicates the >> max ring size of the ring buffer. */<br> >> > + uint32_t ring_size;<br> >> <br> >> ODP queues have historically been of unspecified size. If we're going<br> >> to introduce the notion of explicitly limited sized queues this has<br> >> additional implications.<br> Implementations have likely applied some internal limitation, unknown to the application. linux-generic's use of linked lists can't be seen as *the* way to do it.
>> >> First, ring_size is an inappropriate choice of name here since a ring<br> >> is an implementation model, not a specification. The documentation<br> >> says "Queue size", so<br> >> <br> >> uint32_t size;<br> >> <br> >> is sufficient here. > > Agree, will change 'ring_size' to a better name. My suggestion is 'min_capacity' (or 'min_size'). The application specifies the minimum capacity requires/desires. > >> We should document that size = 0 requests a queue<br> >> of default size (which may be unbounded).<br> > > Unbounded size is not practical or possible. Can we agree that 0 means > that the default aka ODP_CONFIG_QUEUE_SIZE is used? Should we allow for > greater than ODP_CONFIG_QUEUE_SIZE? E.g. 10,000 queue depth is also not > practical or possible. Perhaps we need a max which also acts as the default. IMO, a 'min_capacity' of 0 should mean use default size (per the ODP configuration file). Which is likely what implementations already do when the application cannot specify the size. > >> <br> >> Second, if we're going to allow a queue size to be specified then this<br> >> needs to be added as an output to odp_queue_capability() > > OK, will look into that. > >> so the<br> >> application knows the max_size supported (again 0 = unbounded).<br> >> <br> >> A larger question, however, is why is this being introduced at all<br> >> since this field is only used in the modified<br> >> odph_cuckoo_table_create() helper routine and this, in turn, is only<br> >> used within the cuckootable test module? This seems an extraneous and<br> >> unnecessary change and has no relationship to the rest of this patch<br> >> series.<br> > > AFAIK, the cuckoo unit test enqueues too many events (millions) to a queue. > That sounds like it makes no sense, but is an example of how anything is > possible. The cuckoo hash table uses a queue to store unused "slots". The hash table could be very large so have a very large number of unused slots that need to be saved on a queue. I think it is unreasonable of the cuckoo hash table to expect infinite queue sizes. Better to specify the required capacity and then fail in odp_queue_create(). > >> <br> >> So Parts 1 and 3 of this series don't seem to have anything to do with<br> >> the scalable scheduler. As a minor point, the order of these needs to<br> >> be reversed to preserve bisectability since Part 1 can't reference the<br> >> new field before Part 3 defines it.<br> > > Agree that there are 2 independent sets of patches, but the order is needed > in order to get a sane `make check' on ARM-based chips. Without these fixes > going in first, we have no way of knowing whether the scalable scheduler > patches > caused the issue or not. I can break these into 2 separate sets of patches. > >> <br> >> > +<br> >> > /** Enqueue mode<br> >> > *<br> >> > * Default >> value for both queue types is ODP_QUEUE_OP_MT. Application<br> >> > --<br> >> > 2.12.1<br> >> ><br> >> </div></span></font> >> </body> >> </html>
