[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [patch 1/5][v2] Extract code from get_cluster_offset()
From: |
Laurent Vivier |
Subject: |
Re: [Qemu-devel] [patch 1/5][v2] Extract code from get_cluster_offset() |
Date: |
Tue, 05 Aug 2008 16:28:27 +0200 |
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 <address@hidden>
>
> Acked-by: Kevin Wolf <address@hidden>
>
>
> 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
--
----------------- address@hidden ------------------
"La perfection est atteinte non quand il ne reste rien à
ajouter mais quand il ne reste rien à enlever." Saint Exupéry