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