Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755471AbcKOHdQ (ORCPT ); Tue, 15 Nov 2016 02:33:16 -0500 Received: from mail.linuxfoundation.org ([140.211.169.12]:49250 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752777AbcKOHdN (ORCPT ); Tue, 15 Nov 2016 02:33:13 -0500 Date: Tue, 15 Nov 2016 08:33:22 +0100 From: Greg KH To: Peter Zijlstra Cc: keescook@chromium.org, will.deacon@arm.com, elena.reshetova@intel.com, arnd@arndb.de, tglx@linutronix.de, mingo@kernel.org, hpa@zytor.com, dave@progbits.org, linux-kernel@vger.kernel.org Subject: Re: [RFC][PATCH 2/7] kref: Add kref_read() Message-ID: <20161115073322.GC28248@kroah.com> References: <20161114173946.501528675@infradead.org> <20161114174446.486581399@infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20161114174446.486581399@infradead.org> User-Agent: Mutt/1.7.1 (2016-10-04) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4978 Lines: 106 On Mon, Nov 14, 2016 at 06:39:48PM +0100, Peter Zijlstra wrote: > Since we need to change the implementation, stop exposing internals. > > Provide kref_read() to read the current reference count; typically > used for debug messages. > > Kills two anti-patterns: > > atomic_read(&kref->refcount) > kref->refcount.counter > > Signed-off-by: Peter Zijlstra (Intel) > --- > drivers/block/drbd/drbd_req.c | 2 - > drivers/block/rbd.c | 8 ++--- > drivers/block/virtio_blk.c | 2 - > drivers/gpu/drm/drm_gem_cma_helper.c | 2 - > drivers/gpu/drm/drm_info.c | 2 - > drivers/gpu/drm/drm_mode_object.c | 4 +- > drivers/gpu/drm/etnaviv/etnaviv_gem.c | 2 - > drivers/gpu/drm/msm/msm_gem.c | 2 - > drivers/gpu/drm/nouveau/nouveau_fence.c | 2 - > drivers/gpu/drm/omapdrm/omap_gem.c | 2 - > drivers/gpu/drm/ttm/ttm_bo.c | 4 +- > drivers/gpu/drm/ttm/ttm_object.c | 2 - > drivers/infiniband/hw/cxgb3/iwch_cm.h | 6 ++-- > drivers/infiniband/hw/cxgb3/iwch_qp.c | 2 - > drivers/infiniband/hw/cxgb4/iw_cxgb4.h | 6 ++-- > drivers/infiniband/hw/cxgb4/qp.c | 2 - > drivers/infiniband/hw/usnic/usnic_ib_sysfs.c | 6 ++-- > drivers/infiniband/hw/usnic/usnic_ib_verbs.c | 4 +- > drivers/misc/genwqe/card_dev.c | 2 - > drivers/misc/mei/debugfs.c | 2 - > drivers/pci/hotplug/pnv_php.c | 2 - > drivers/pci/slot.c | 2 - > drivers/scsi/bnx2fc/bnx2fc_io.c | 8 ++--- > drivers/scsi/cxgbi/libcxgbi.h | 4 +- > drivers/scsi/lpfc/lpfc_debugfs.c | 2 - > drivers/scsi/lpfc/lpfc_els.c | 2 - > drivers/scsi/lpfc/lpfc_hbadisc.c | 40 +++++++++++++-------------- > drivers/scsi/lpfc/lpfc_init.c | 3 -- > drivers/scsi/qla2xxx/tcm_qla2xxx.c | 4 +- > drivers/staging/android/ion/ion.c | 2 - > drivers/staging/comedi/comedi_buf.c | 2 - > drivers/target/target_core_pr.c | 10 +++--- > drivers/target/tcm_fc/tfc_sess.c | 2 - > drivers/usb/gadget/function/f_fs.c | 2 - > fs/exofs/sys.c | 2 - > fs/ocfs2/cluster/netdebug.c | 2 - > fs/ocfs2/cluster/tcp.c | 2 - > fs/ocfs2/dlm/dlmdebug.c | 12 ++++---- > fs/ocfs2/dlm/dlmdomain.c | 2 - > fs/ocfs2/dlm/dlmmaster.c | 8 ++--- > fs/ocfs2/dlm/dlmunlock.c | 2 - > include/drm/drm_framebuffer.h | 2 - > include/drm/ttm/ttm_bo_driver.h | 4 +- > include/linux/kref.h | 5 +++ > include/linux/sunrpc/cache.h | 2 - > include/net/bluetooth/hci_core.h | 4 +- > net/bluetooth/6lowpan.c | 2 - > net/bluetooth/a2mp.c | 4 +- > net/bluetooth/amp.c | 4 +- > net/bluetooth/l2cap_core.c | 4 +- > net/ceph/messenger.c | 4 +- > net/ceph/osd_client.c | 10 +++--- > net/sunrpc/cache.c | 2 - > net/sunrpc/svc_xprt.c | 6 ++-- > net/sunrpc/xprtrdma/svc_rdma_transport.c | 4 +- > 55 files changed, 120 insertions(+), 116 deletions(-) > > --- a/drivers/block/drbd/drbd_req.c > +++ b/drivers/block/drbd/drbd_req.c > @@ -520,7 +520,7 @@ static void mod_rq_state(struct drbd_req > /* Completion does it's own kref_put. If we are going to > * kref_sub below, we need req to be still around then. */ > int at_least = k_put + !!c_put; > - int refcount = atomic_read(&req->kref.refcount); > + int refcount = kref_read(&req->kref); > if (refcount < at_least) > drbd_err(device, > "mod_rq_state: Logic BUG: %x -> %x: refcount = %d, should be >= %d\n", As proof of "things you should never do", here is one such example. ugh. > --- a/drivers/block/virtio_blk.c > +++ b/drivers/block/virtio_blk.c > @@ -767,7 +767,7 @@ static void virtblk_remove(struct virtio > /* Stop all the virtqueues. */ > vdev->config->reset(vdev); > > - refc = atomic_read(&disk_to_dev(vblk->disk)->kobj.kref.refcount); > + refc = kref_read(&disk_to_dev(vblk->disk)->kobj.kref); > put_disk(vblk->disk); > vdev->config->del_vqs(vdev); > kfree(vblk->vqs); And this too, ugh, that's a huge abuse and is probably totally wrong... thanks again for digging through this crap. I wonder if we need to name the kref reference variable "do_not_touch_this_ever" or some such thing to catch all of the people who try to be "too smart". greg k-h