On Friday 06 October 2006 7:22 pm, Christopher "Monty" Montgomery wrote:
> > ... you're not including whitespace which you _should_ include,
> > and (elsewhere) are adding whitespace you should not include.
>
> I accept this in the context of the kernel coding style. Given that
> this is all entirely mechanical and mandatory, I'm surprised
> linux-c-mode does not handle it for me.
You should not expect that linux-c-mode actually implements everything
per the coding conventions. I'm pretty sure that the CodingStyle doc
even points that out.
> > > - if (stream->refcount == 1) {
> > > + /* don't free on last descriptor; free when endpoint disable
> > > + finally releases last refcount. Although it is technically
> > > + broken for an endpoint driver to submit its streaming
> > > + descriptors such that a new one appears after the old one
> > > + ends, it is only punishing the users to insist on breaking
> > > + these drivers when it's not necessary to do so. This saves
> > > + substantial overhead in that case.*/
> >
> > The comment should be corrected, both for content and style. In this
> > case I'd suggest
> >
> > /* if refcount == 1 this is just from udev->ep_{in,out}[];
> > * we can at least avoid reallocating this stream's memory, and
> > * the previously budgeted bandwidth may still be available.
> > */
>
> Except that your comment is incorrect in both content and intent. The
> whole point is that the bandwidth *is* still there and *will* be
> available, by design.
Nope. That's not the policy Linux uses. Alan has pointed that out,
and I've pointed that out. You should listen to us on that issue.
- Bandwidth allocations are maintained by keeping the periodic
endpoint active. When its queue drains, it no longer needs
any bandwidth.
- The drivers above usbcore all understand that's how they work.
- Even userspace knows that applications get "no bandwidth allowed"
faults only when starting I/O on a stream... not when calling
SET_INTERFACE or SET_CONFIGURATION or whatever.
You are suggesting a model where as soon as an endpoint gets enabled
and used, it ties down bandwidth virtually forever. This creates
nastily mysterious fault modes, like: because several days ago one
application may have sent URBs to ten different ISO endpoints, today
no application can use any more ISO endpoints ... because those
bandwidth reservations never got canceled.
> There is no 'may'. There is no 'should'. It is invariant.
No; see above. You're suggesting we change _one_ HCD to use a
different bandwidth allocation policy, in ways that are visible to
other drivers and userspace ... yet when I look in drivers/usb/host
I see several more HCDs than that. I don't think you're proposing
to change all of those, and all drivers that issue ISO urbs, and
all userspace code, etc.
Note that in your target application context (call it ten
concurrent ISO streams, in a dedicated config), the "may" is
a non-issue. That's because once the bandwidth gets allocated
to one endpoint, and a schedule "slot" gets allocated to it,
there's nothing that will ever need to clobber that allocation.
And even if there were (something assigns altsetting 0, etc),
then the fact that you were able to budget once means that
you can safely budget that again...
> More than that, the ehci_iso_stream struct does not
> exist for its own sake; it is the handle being used by the budget to
> ID the stream.
Fine. But identifying a stream is not the same thing as tying
down some bandwidth for it. And again, $SUBJECT says you're
just making the _instances_ more persistent ... it doesn't say
anything about changing the model for ISO transfers. And in
fact, this patch by itself _doesn't_ change that.
> I don't care about its memory, the important part is
> that it is a persistent handle.
But I do care about avoiding heap allocations in critical paths,
they impact icache, dcache, and instruction count ... :)
- Dave
-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys -- and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
