Message-ID: <559DA301.20203@redhat.com>
Date: Wed, 08 Jul 2015 18:24:01 -0400
From: Doug Ledford <dledford@redhat.com>
Organization: Red Hat, Inc.
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0
MIME-Version: 1.0
To: maninder1.s@samsung.com
CC: Sean Hefty <sean.hefty@intel.com>, Or Gerlitz <ogerlitz@mellanox.com>,
        David Miller <davem@davemloft.net>,
        "roland@purestorage.com" <roland@purestorage.com>,
        Matan Barak <matanb@mellanox.com>, Moni Shoua <monis@mellanox.com>,
        "jackm@dev.mellanox.co.il" <jackm@dev.mellanox.co.il>,
        Yishai Hadas <yishaih@mellanox.com>,
        "eranbe@mellanox.com" <eranbe@mellanox.com>,
        Ira Weiny <ira.weiny@intel.com>,
        linux-rdma <linux-rdma@vger.kernel.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        PANKAJ MISHRA <pankaj.m@samsung.com>
Subject: Re: [PATCH 1/1] infiniband: Remove redundant NULL check before kfree
References: <1037392254.437811436329408244.JavaMail.weblogic@ep2mlwas06a>
In-Reply-To: <1037392254.437811436329408244.JavaMail.weblogic@ep2mlwas06a>
OpenPGP: id=AE6B1BDA122B23B4265B1274B826A3330E572FDD;
	url=pgp.mit.edu
Content-Type: multipart/signed; micalg=pgp-sha256;
 protocol="application/pgp-signature";
 boundary="PBsSRKACBsJ7uFAK4Pu3hrJc7Qh1SPR0g"
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3444
Lines: 98

This is an OpenPGP/MIME signed message (RFC 4880 and 3156)
--PBsSRKACBsJ7uFAK4Pu3hrJc7Qh1SPR0g
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable

On 07/08/2015 12:23 AM, Maninder Singh wrote:
> Hello,
>=20
>>> +			for (i =3D 0; i < dev->caps.num_ports; i++)
>>> +				kfree(dm[i]);
>>> 			goto out;
>>> 		}
>>> 	}
>>> --
>>> 1.7.9.5
>>>
>>
>> If you are going to change this, you might as well make it 100% correc=
t:
>>
>> i=E2=80=94-;
>> while (i >=3D 0)
>> 	kfree(dm[i]);
>>
>> Then you don=E2=80=99t have to worry about whether kfree works on NULL=
, every item you free will be guaranteed to be non-NULL.
> Thanks for suggestion :)
> Sent new patch with described changes, I was thinking one more thing.
>=20
> In below code :-
>         if (!ibdev->sriov.is_going_down)
>             queue_work(ibdev->sriov.demux[i].ud_wq, &dm[i]->work);
>         spin_unlock_irqrestore(&ibdev->sriov.going_down_lock, flags);
>     }
> out:
>     kfree(dm);
>     return;
>=20
> dm is freed after queue_work, is it correct to free dm when other dm[i]=
 are allocated ? i did not get it.

The dm is just there to give an easy way to refer to a variable number
of work structs.  The flow is supposed to be something like this:

alloc(dm)
for(i=3D0;i<num_qps;i++)
    dm[i] =3D=3D alloc(work item);
for(i=3D0;i<num_qps;i++)
    init dm[i] work item
    queue dm[i] work item
free(dm)

In this scenario, all of the dm[i] items should be queued to delayed
work.  When that work completes, it should then free these structs.  So,
yes, the dm variable itself is just a temporary means of keeping all
those work struct pointers together.  However, your question caused me
to look closely here, and I see that there is a bug.  In particular, if
we check the sriov.is_going_down and as a result *don't* queue a work
item, then we end up leaking that work struct.  In addition, I think
there is room to optimize this routine considerably.  I'll post a patch
for that in a minute.

--=20
Doug Ledford <dledford@redhat.com>
              GPG KeyID: 0E572FDD


--PBsSRKACBsJ7uFAK4Pu3hrJc7Qh1SPR0g
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: OpenPGP digital signature
Content-Disposition: attachment; filename="signature.asc"

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBCAAGBQJVnaMBAAoJELgmozMOVy/dbMcQAIG2/opz+xWAPdt3OcqePsI2
TCXkMULl2MG32GV4te/C5SXMJbo01psM/GLjuhLVdwLsgMOYKCVdHG1hTka2sWEz
JJTVWOZu5pa6qW1FYR1V3rAVNMzTDtY9Zjozn6joTvW2ymt6RKrR1COK47LYy1Q7
DSw6RzWdPYoHP6OXMafd2yqhwA6bDGqmmTDtuSh+Dk2y1/kVnVqv3FPWHE6KoDj4
38eyk5a6a9hwxzm633ObpOLuHKsOK29dFezlxrbyi91jBXAwivit3kTqSHulM0Xq
M3NdAuRIjwNGUxr9jHhVqN3pQ4Ci4Psl0kRFtdOPunwnARshxA1fCRTpn19EiZ7r
y4hkm6ICI0Ui4OpB99Xlrxt0zKVbEEwVlV/97GF0Chk6QB4qLJoqdqFfKDv/3z3M
RA2Vsgd5spHZkFyT3nYueyub4mhrl6JrhdPXqs70yyXFI6NPYfsRVjCr1BkHIAqX
2DyZZr3bDeBjC5TwND4O3LYGjJoCh2TXyz/xA573Gv1rtlCAKjXeoGnkpDpmClBu
TUydW+GFU1yyEw+1GkFLwjLNxU+zNHHYT9pwXd5u12EvkbGLRqg+b8tzo9Xbh89K
/aYlBBOWs46ABq6ynd8ubdZvZpleu4+uJ6yGUoWPEGSWsbPTCbMlbiLQHPAhV4qH
GQx7OgUkKvgtPI4sxxgl
=hrXL
-----END PGP SIGNATURE-----

--PBsSRKACBsJ7uFAK4Pu3hrJc7Qh1SPR0g--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/