Re: [Qemu-devel] [patch 1/5][v2] Extract code from get_cluster_offset()

[Laurent Vivier <Laurent.Vivier@bull.net>]

Le mardi 05 août 2008 à 16:15 +0200, Kevin Wolf a écrit :
> Laurent Vivier schrieb:
> > Extract code from get_cluster_offset() into new functions:
> > 
> > - seek_l2_table()
> > 
> > Search an l2 offset in the l2_cache table.
> > 
> > - l2_load()
> > 
> > Read the l2 entry from disk
> > 
> > - l2_allocate()
> > 
> > Allocate a new l2 entry.
> > 
> > Signed-off-by: Laurent Vivier <Laurent.Vivier@bull.net>
> 
> Acked-by: Kevin Wolf <kwolf@suse.de>
> 
> 
> However, unrelated to your patch, I noticed that you touch code which
> could use more comments in some places. (Oh btw, why do you explain the
> new functions in the patch header but not in the code itself? But okay,
> these are more or less obvious.)

OK, you're right, I'll add some comments.

> > +static uint64_t *seek_l2_table(BDRVQcowState *s, uint64_t l2_offset)
> > +{
> > +    int i,j;
> > +
> > +    for(i = 0; i < L2_CACHE_SIZE; i++) {
> > +        if (l2_offset == s->l2_cache_offsets[i]) {
> > +            /* increment the hit count */
> > +            if (++s->l2_cache_counts[i] == 0xffffffff) {
> > +                for(j = 0; j < L2_CACHE_SIZE; j++) {
> > +                    s->l2_cache_counts[j] >>= 1;
> > +                }
> > +            }
> 
> This if block is one of them. While it's quite obvious that some counter
> is incremented, the action to avoid an overflow isn't. Why don't we do
> something like if (count < 0xffffffff) count++?
> 
> And l2_cache_counts is a bad name anyway, IMHO. I first thought of
> something like a reference counter (and then it would be definitely
> worth a comment; now it is debatable). l2_cache_hits could be a better name.

It is a cut&paste... so I don't know what it is really doing and I don't
want to modify something that has no direct link to the goal I reach...
even a variable name.

I'm currently testing these patches with encrypted disk and compressed
disk, and I've found a bug with encrypted one. I'll repost patches soon.

Thank you for your comments,
Laurent
-- 
----------------- Laurent.Vivier@bull.net  ------------------
  "La perfection est atteinte non quand il ne reste rien à
ajouter mais quand il ne reste rien à enlever." Saint Exupéry