Re: [RFC][PATCH 2/7] kref: Add kref_read()

[Greg KH]

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) <peterz@xxxxxxxxxxxxx>
> ---
>  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