RE: [PATCH rdma-next v1 3/4] RDMA/nldev: Provide MR statistics

From: Parav Pandit <parav@mellanox.com>
To: Leon Romanovsky <leon@kernel.org>
Cc: Doug Ledford <dledford@redhat.com>,
	Jason Gunthorpe <jgg@mellanox.com>,
	RDMA mailing list <linux-rdma@vger.kernel.org>,
	Erez Alfasi <ereza@mellanox.com>
Subject: RE: [PATCH rdma-next v1 3/4] RDMA/nldev: Provide MR statistics
Date: Fri, 30 Aug 2019 12:06:15 +0000	[thread overview]
Message-ID: <AM0PR05MB486610B252DCDD895D82EF0DD1BD0@AM0PR05MB4866.eurprd05.prod.outlook.com> (raw)
In-Reply-To: <20190830111245.GF12611@unreal>


> -----Original Message-----
> From: Leon Romanovsky <leon@kernel.org>
> Sent: Friday, August 30, 2019 4:43 PM
> To: Parav Pandit <parav@mellanox.com>
> Cc: Doug Ledford <dledford@redhat.com>; Jason Gunthorpe
> <jgg@mellanox.com>; RDMA mailing list <linux-rdma@vger.kernel.org>; Erez
> Alfasi <ereza@mellanox.com>
> Subject: Re: [PATCH rdma-next v1 3/4] RDMA/nldev: Provide MR statistics
> 
> On Fri, Aug 30, 2019 at 10:18:35AM +0000, Parav Pandit wrote:
> >
> >
> > > -----Original Message-----
> > > From: linux-rdma-owner@vger.kernel.org <linux-rdma-
> > > owner@vger.kernel.org> On Behalf Of Leon Romanovsky
> > > Sent: Friday, August 30, 2019 1:46 PM
> > > To: Doug Ledford <dledford@redhat.com>; Jason Gunthorpe
> > > <jgg@mellanox.com>
> > > Cc: Leon Romanovsky <leonro@mellanox.com>; RDMA mailing list <linux-
> > > rdma@vger.kernel.org>; Erez Alfasi <ereza@mellanox.com>
> > > Subject: [PATCH rdma-next v1 3/4] RDMA/nldev: Provide MR statistics
> > >
> > > From: Erez Alfasi <ereza@mellanox.com>
> > >
> > > Add RDMA nldev netlink interface for dumping MR statistics information.
> > >
> > > Output example:
> > > ereza@dev~$: ./ibv_rc_pingpong -o -P -s 500000000
> > >   local address:  LID 0x0001, QPN 0x00008a, PSN 0xf81096, GID ::
> > >
> > > ereza@dev~$: rdma stat show mr
> > > dev mlx5_0 mrn 2 page_faults 122071 page_invalidations 0
> > > prefetched_pages 122071
> > >
> > > Signed-off-by: Erez Alfasi <ereza@mellanox.com>
> > > Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> > > ---
> 
> <...>
> 
> > > struct ib_device_ops {
> > >  	 */
> > >  	int (*counter_update_stats)(struct rdma_counter *counter);
> > >
> > > +	/**
> > > +	 * fill_odp_stats - Fill MR ODP stats into a given
> > > +	 * ib_odp_counters struct.
> > > +	 * Return value - true in case counters has been filled,
> > > +	 * false otherwise (if its non-ODP registered MR for example).
> > > +	 */
> > > +	bool (*fill_odp_stats)(struct ib_mr *mr, struct ib_odp_counters
> > > *cnt);
> > > +
> > Requesting ODP stats on non-ODP MR is an error.
> > Instead of returning bool, please return int = -EINVAL as an error for non ODP
> MRs.
> 
> We don't want to add extra checks in the drivers for something that is supposed
> to be checked by upper layer. If caller asks for ODP statistics, he will check that
> MR is ODP backed. Especially if the need to have ODP MR is embedded into the
> function name.
> 
That make sense. In that case return type should be void.
Comment says - "false otherwise (if its non-ODP registered MR for example)."

And with that logic,
Below code should be in upper layer instead of mlx5/main.c

+	if (!is_odp_mr(mr))
+		return false;