Hi,
On Thu, Jan 12, 2006 at 04:19:07PM +0200, Denis Vlasenko wrote:
> On Thursday 12 January 2006 12:37, Andreas Mohr wrote:
> > [copying netdev for centralized development]
> >
> > Hi all,
> >
> > some updates to acx-20060111:
>
> I'm afraid I will take only part of it.
But still you're already taking part *in* it ;)
(thanks!)
> > - add some cache prefetching at critical places, but still unsure whether it
> > helps (some rdtscl() testing hasn't shown much yet),
> > thus make it configurable
>
> Prefetching should be used when one needs to traverse a *lot* of memory
> (example: fs code might use it in dentry/inode cache search algorithms),
> but it is way below noise level in driver for a device with less than
> 30Mbit/s max throughput.
>
> This usage is possibly bogus:
>
> /* now write the parameters of the command if needed */
> + ACX_PREFETCHW(priv->cmd_area);
> if (buffer && buflen) {
> /* if it's an INTERROGATE command, just pass the length
> * of parameters to read, as data */
>
> because priv->cmd_area points to PCI device's memory, not RAM.
> It is not cacheable. I think that writes won't be sped up at all
> by such prefetchw.
I think I've heard that somewhere... so you're most likely right.
> > - use "counter % 8" instead of "counter % 5" for easier ASM calculation
>
> That is a wait loop, you should not cycle optimize those - you are
> waiting anyway, typically for a few ms at least!
Not execution time, code size! ;)
> If you really want to optimize it once and for all, do something like this:
>
> priv member:
> wait_queue_head_t cmd_wait;
>
> in init code:
> init_waitqueue_head(&priv->cmd_wait);
>
> in issue_cmd():
> CLEAR_BIT(priv->irq_status, HOST_INT_CMD_COMPLETE);
> ...cmd setup...
> wait_event_interruptible_timeout(&priv->wait,
> priv->irq_status & HOST_INT_CMD_COMPLETE,
> cmd_ms_timeout*HZ/1000);
> if (priv->irq_status & HOST_INT_CMD_COMPLETE)
> /* success */
>
> in IRQ handler:
> SET_BIT(priv->irq_status, HOST_INT_CMD_COMPLETE);
> wake_up(&priv->cmd_wait);
>
> This will save ~2.5 ms on average on each cmd.
Nice stuff, we should definitely go for it if possible (module init time
will benefit from that, it's currently 1.2 seconds on my P3/700, with
command requests being a sizeable part, despite the large firmware upload
which takes most of the remaining time!).
Are you going to add it already, or should this wait?
> > - add ACX_IE_HDR__TYPE_LEN define for IE struct header variables used
> > everywhere
>
> Why is this useful?
In order to prevent typos/problems in this area:
I already had one case last week in the power save mode structs header
(u32 instead of u16) which caused this thing to not work properly
(already months ago when I first tried it!).
I discovered this mismatch by accident only!
If this define somehow happens to get altered in the future, then you *will*
notice it immediately since *all* firmware structs will be affected then, not
only such a very unimportant one as 802.11 powersave mode.
And of course also to keep header size down (...and compile time up).
> @@ -171,7 +179,7 @@
> static inline int
> mac_is_bcast(const u8 *mac)
> {
> - /* AND together 4 first bytes with sign-entended 2 last bytes
> + /* AND together 4 first bytes with sign-extended 2 last bytes
> ** Only bcast address gives 0xffffffff. +1 gives 0 */
> return ( *(s32*)mac & ((s16*)mac)[2] ) + 1 == 0;
> }
>
> Took me 2 minutes to find the difference! :)
Sorry! ;)
Andreas Mohr
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
