isn't queue size connected to number elements in the pool?
Maxim.
On 29 March 2017 at 04:55, Brian Brooks <brian.bro...@arm.com> 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 <brian.bro...@arm.com>
> wrote:<br>
> > > Signed-off-by: Brian Brooks <brian.bro...@arm.com><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>
>
