On Tue, 03 Feb 2015 11:19:58 +0100 walter harms <wharms@xxxxxx> wrote:
>
>
> Am 03.02.2015 10:07, schrieb NeilBrown:
> > On Tue, 03 Feb 2015 09:48:01 +0100 walter harms <wharms@xxxxxx> wrote:
> >
> >>
> >>
> >> Am 02.02.2015 20:46, schrieb NeilBrown:
> >>> On Mon, 02 Feb 2015 16:20:42 +0100 SF Markus Elfring
> >>> <elfring@xxxxxxxxxxxxxxxxxxxxx> wrote:
> >>>
> >>>> From: Markus Elfring <elfring@xxxxxxxxxxxxxxxxxxxxx>
> >>>> Date: Mon, 2 Feb 2015 15:10:57 +0100
> >>>>
> >>>> The kfree() function tests whether its argument is NULL and then
> >>>> returns immediately. Thus the test around the call is not needed.
> >>>>
> >>>> * This issue was detected by using the Coccinelle software.
> >>>>
> >>>> * Let us also move an assignment for the variable "pages" to the place
> >>>> directly before it is really needed for a loop.
> >>>>
> >>>> * Let us also move another kfree() call into a block which should belong
> >>>> to a previous check for the variable "bp".
> >>>>
> >>>> Signed-off-by: Markus Elfring <elfring@xxxxxxxxxxxxxxxxxxxxx>
> >>>> ---
> >>>> drivers/md/bitmap.c | 10 +++++-----
> >>>> 1 file changed, 5 insertions(+), 5 deletions(-)
> >>>>
> >>>> diff --git a/drivers/md/bitmap.c b/drivers/md/bitmap.c
> >>>> index da3604e..47d72df 100644
> >>>> --- a/drivers/md/bitmap.c
> >>>> +++ b/drivers/md/bitmap.c
> >>>> @@ -1586,15 +1586,15 @@ static void bitmap_free(struct bitmap *bitmap)
> >>>> bitmap_file_unmap(&bitmap->storage);
> >>>>
> >>>> bp = bitmap->counts.bp;
> >>>> - pages = bitmap->counts.pages;
> >>>>
> >>>> /* free all allocated memory */
> >>>> -
> >>>> - if (bp) /* deallocate the page memory */
> >>>> + if (bp) { /* deallocate the page memory */
> >>>> + pages = bitmap->counts.pages;
> >>>> for (k = 0; k < pages; k++)
> >>>> - if (bp[k].map && !bp[k].hijacked)
> >>>> + if (!bp[k].hijacked)
> >>>> kfree(bp[k].map);
> >>>> - kfree(bp);
> >>>> + kfree(bp);
> >>>> + }
> >>>> kfree(bitmap);
> >>>> }
> >>>>
> >>>
> >>> Hi,
> >>> I'm somewhat amused that you removed a test for one kfree, but imposed a
> >>> test on another. I realised the second test was already there, but why not
> >>> just:
> >>>
> >>> diff --git a/drivers/md/bitmap.c b/drivers/md/bitmap.c
> >>> index da3604e73e8a..ad13b2e1bf1f 100644
> >>> --- a/drivers/md/bitmap.c
> >>> +++ b/drivers/md/bitmap.c
> >>> @@ -1592,7 +1592,7 @@ static void bitmap_free(struct bitmap *bitmap)
> >>>
> >>> if (bp) /* deallocate the page memory */
> >>> for (k = 0; k < pages; k++)
> >>> - if (bp[k].map && !bp[k].hijacked)
> >>> + if (!bp[k].hijacked)
> >>> kfree(bp[k].map);
> >>> kfree(bp);
> >>> kfree(bitmap);
> >>>
> >>>
> >>> It makes the intention of the patch much clearer.
> >>>
> >>> I'd probably prefer to leave the code as it is. I don't think either patch
> >>> is really an improvement in readability, and readability trumps performance
> >>> in places like this.
> >>>
> >>
> >>
> >> hello Neil,
> >> just for my curiosity.
> >> is it possible that a bp[k] exists but bp[k].hijacked is 0 ?
> >> if that case: kfree(bp) would free an object in use.
> >> otherwise hijacked seems useless here as it is possible to free
> >> everything.
> >>
> >> re,
> >> wh
> >
> > If .hijacked is 0, the .map is a pointer (that would need to be freed) or is
> > NULL.
> > If .hijacked is 1, then .map has been re-tasked as 2 16-bit counters. They
> > are probably bother zero at this point, but I think it is safer to completely
> > skip the 'free' if 'hijacked' is set as in that case 'map' isn't a pointer.
> >
>
> It seems i was not clear enough, sorry about that.
>
> we have a loop to kfree(bp[k].map), thats fine how it is done.
>
> I am wondering about the kfree(bp) after the loop.
> if bp[k].map is not freed for what ever reason how can it be freed later
> after a kfree(bp) ?
>
> what is what i am missing ? Is there a hidden copy ?
If bp[k].map is a pointer, it will be freed.
If bp[k].map is not a pointer (because it has been hijacked), then there is
nothing to free.
So after the loop, all bp[k].maps that were allocated will have been freed.
Does that help?
NeilBrown
>
> re,
> wh
>
>
>
Attachment:
pgpebwV0NbnBV.pgp
Description: OpenPGP digital signature
