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

[jackm <jackm@dev.mellanox.co.il>]

On Tue, 29 May 2018 10:40:32 -0600
Jason Gunthorpe <jgg@ziepe.ca> 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@ziepe.ca> 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.
> 
> Preventing re-use seems like a seperate issue from limiting the range
> to be compatible with mlx4.
> 

Preventing immediate re-use can be accomplished by judicious use of the
start argument (second argument) in the call to ida_simple_get (to
introduce hysteresis into the id allocations).

For example, can do something like:

static atomic_t ib_mad_client_id_min = ATOMIC_INIT(1);
...
        ib_mad_client_id = ida_simple_get(&ib_mad_client_ids,
                                          atomic_read(&ib_mad_client_id_min),
                                          ib_mad_sysctl_client_id_max,
                                          GFP_KERNEL);
....
        if (!(ib_mad_client_id % 1000) ||
            ib_mad_sysctl_client_id_max - ib_mad_client_id <= 1000)
		atomic_set(&ib_mad_client_id_min, 1);
	else
                atomic_set(&ib_mad_client_id_min, ib_mad_client_id + 1);

The above avoids immediate re-use of ids, and only searches for past
freed ids if the last allocated-id is zero mod 1000.

This is just suggestion -- will probably need some variation of the
above to handle what happens over time (i.e., to not depend on the
modulo operation to reset the search start to 1), to properly handle
how we deal with the start value when we are close to the allowed
client_id_max, and also to implement some sort of locking.

-Jack