Re: [PATCH] IB/mad: Use ID allocator routines to allocate agent number

[Jason Gunthorpe]

On Wed, May 30, 2018 at 09:32:02AM +0200, HÃkon Bugge wrote:
> 
> 
> > On 29 May 2018, at 18:40, Jason Gunthorpe <jgg@xxxxxxxx> wrote:
> > 
> > On Tue, May 29, 2018 at 06:16:14PM +0200, HÃkon Bugge wrote:
> >> 
> >>> On 29 May 2018, at 17:49, Jason Gunthorpe <jgg@xxxxxxxx> wrote:
> >>> 
> >>> On Tue, May 29, 2018 at 09:38:08AM +0200, Hans Westgaard Ry wrote:
> >>>> The agent TID is a 64 bit value split in two dwords.  The least
> >>>> significant dword is the TID running counter. The most significant
> >>>> dword is the agent number. In the CX-3 shared port model, the mlx4
> >>>> driver uses the most significant byte of the agent number to store the
> >>>> slave number, making agent numbers greater and equal to 2^24 (3 bytes)
> >>>> unusable.
> >>> 
> >>> There is no reason for this to be an ida, just do something like
> >>> 
> >>> mad_agent_priv->agent.hi_tid = atomic_inc_return(&ib_mad_client_id) & mad_agent_priv->ib_dev->tid_mask;
> >>> 
> >>> And have the driver set tid_mask to 3 bytes of 0xFF
> >> 
> >> The issue is that some of the mad agents are long-lived, so you will
> >> wrap and use the same TID twice.
> > 
> > We already have that problem, and using ida is problematic because we
> > need to maximize the time between TID re-use, which ida isn't doing.
> 
> Initially, I thought that too, but consulted the spec which states:
> 
> C13-18.1.1: When initiating a new operation, MADHeader:TransactionID
> shall be set to such a value that within that MAD the combination of TID,
> SGID, and MgmtClass is different from that of any other currently executing
> operation. []
> 
> "currently executing operation" that is.

'currently executing operation' encompasses a wide range of behaviors
and it is not just 'completing the MAD at the local node'

You have to exceed all the timeouts before a single node can be
certain that the condition is satisfied.

This is why we just simply maximize the time between allocations, and
encourage the user providing the rest of the tid to do the same.

> > Preventing re-use seems like a seperate issue from limiting the range
> > to be compatible with mlx4.
> 
> Yes. But a v2 of Hans' patch using idr_alloc_cyclic() solves both issues.

That might work, but still have to get rid of the sysctl

Jason