On Mar 15, 2008 16:24 +0300, Andrew Perepechko wrote:
> On Saturday 15 March 2008 07:23:16 Andreas Dilger wrote:
> > In a few places you add new on-stack variables like "sb", but they aren't
> > used more than 1 or 2 times. While it makes the code a tiny bit clearer
> > (though not largely so for "sb" because it is only dquot->dq_sb) it does
> > increase the stack usage, and that is never a good idea.
>
> I agree. Probably, it makes sense to rollback sb change. Though this may lead
> to additional line breaks.
Line breaks don't consume stack space ;-).
> > Hmm, allocating "fakedquot" on the stack just to compare it to zero
> > doesn't seem like a good use of space. What about doing the memcmp()
> > against page_address(ZERO_PAGE(0))? It might be nice to have a permanent
> > mapping of ZERO_PAGE(0) like void *zero_buffer that can be used for this.
>
> What do you think about something like this?
>
> +static union v2_disk_dqblk emptydquot;
> +static union v2_disk_dqblk fakedquot[2] = {
> + {.disk_dqblk_r0 = {.dqb_itime = __constant_cpu_to_le64(1LLU)}},
> + {.disk_dqblk_r1 = {.dqb_itime = __constant_cpu_to_le64(1LLU)}}
> +};
Yes, since these structures are constant there are no multi-thread issues.
Not only does this use less stack space, but it is better performing as
well due to less memset() overhead.
NB - it seems you have whitespace at the end of some lines, please remove it.
>@@ -724,9 +724,7 @@ static int v2_read_dquot(struct dquot *d
> else {
> ret = 0;
> /* We need to escape back all-zero structure */
>- memset(&empty, 0, dqblksz);
>- DQF_PUT(&empty, rev, dqb_itime, 1);
>- if (!memcmp(&empty, &ddquot, dqblksz))
>+ if (!memcmp(&fakedquot[rev], &ddquot, dqblksz))
> DQF_PUT(&ddquot, rev, dqb_itime, 0);
> }
I wonder if there is a CPU instruction "compare memory with zero" that
would be available for something like this, that could be used like:
if (!memzcmp(&ddquot, dqblksz))
DQF_PUT(&ddquot, rev, dqb_itime, 0);
I know I've had to use a few hacks like this to check if some buffer is
zero filled, so having it efficiently done by the CPU would be a win.
It would be possible to use something like generic_find_first_bit():
#define memzcmp(ptr, bytes) (generic_find_first_bit(ptr, bytes*8) >= bytes*8)
which breaks down to an assembly instruction "bsfq" on x86_64 if the
parameter is constant. That might make it more efficient to have 2
separate codepaths here with a constant "bytes" parameter so that the
larger memzcmp() can be optimized.
Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
