On Tue, Apr 02, 2019 at 03:51:34PM +0300, Gal Pressman wrote:
> On 01-Apr-19 16:47, Leon Romanovsky wrote:
> > On Mon, Apr 01, 2019 at 03:11:26PM +0300, Gal Pressman wrote:
> >> On 01-Apr-19 11:47, Leon Romanovsky wrote:
> >>> From: Mark Zhang <markz@xxxxxxxxxxxx>
> >>>
> >>> In manual mode a QP is bound to a counter manually. If counter is not
> >>> specified then a new one will be allocated.
> >>> Manually mode is enabled when user binds a QP, and disabled when the
> >>> last manually bound QP is unbound.
> >>> When auto-mode is turned off and there are counters left, manual mode
> >>> is enabled so that the user is able to access these counters.
> >>
> >> IMO, an API to allocate a counter makes more sense than allocating it on the
> >> first bind when a counter is not specified.
> >
> > This suggestion was raised during internal code review and we decided to
> > make users life simple and don't require from them to manage counter
> > lifetime. Allocation by users will immediately means that they need
> > to deallocate explicitly too. Otherwise we will find ourselves with
> > asymmetrical API which is not good. It is unclear who and when will
> > call to deallocate.
> >
> > The decision to make bind/unbind internally allocate and deallocate
> > allowed us to ensure that counter lifetime is strict and managed by the
> > kernel.
> >
> >> The part about turning off auto mode with counters left seems like a mess, do we
> >> really want all these hidden "rules" instead of keeping it simple? Don't let the
> >> user change modes when a counter still has resources bind to it.
> >
> > There are no any hidden rules, once you disable auto mode, it will
> > applicable for new objects only. There are no changes in old and
> > already allocated objects.
>
> I think it's better to be explicit than deciding for the user when to
> allocate/deallocate his resources, but that's just my opinion.
There is no meaning in counter which is not connected to any resource,
such counter is subject to be forgotten and will cause to "leakage".
This is our attempt to manage lifetime by the entity (kernel) which
can distinguish between live and dead objects.
>
> >
> >>
> >>>
> >>> Signed-off-by: Mark Zhang <markz@xxxxxxxxxxxx>
> >>> Reviewed-by: Majd Dibbiny <majd@xxxxxxxxxxxx>
> >>> Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> >>> ---
> >>> +static int rdma_counter_bind_qp_manual(struct rdma_counter *counter,
> >>> + struct ib_qp *qp)
> >>> +{
> >>> + int ret = 0;
> >
> > We will fix this and everything below.
> >
> > Thanks
> >
Attachment:
signature.asc
Description: PGP signature
