[PATCH] IB/core: Make ib_mad_client_id atomic

[PATCH] IB/core: Make ib_mad_client_id atomic

[Håkon Bugge]

Two kernel threads may get the same value for agent.hi_tid, if the
agents are registered for different ports. As of now, this works, as
the agent list is per port.

It is however confusing and not future robust. Hence, making it
atomic.

Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com>
Reviewed-by: Jack Morgenstein <jackm@dev.mellanox.co.il>
---
 drivers/infiniband/core/mad.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/core/mad.c b/drivers/infiniband/core/mad.c
index c50596f7f98a..b28452a55a08 100644
--- a/drivers/infiniband/core/mad.c
+++ b/drivers/infiniband/core/mad.c
@@ -59,7 +59,7 @@ module_param_named(recv_queue_size, mad_recvq_size, int, 0444);
 MODULE_PARM_DESC(recv_queue_size, "Size of receive queue in number of work requests");
 
 static struct list_head ib_mad_port_list;
-static u32 ib_mad_client_id = 0;
+static atomic_t ib_mad_client_id = ATOMIC_INIT(0);
 
 /* Port list lock */
 static DEFINE_SPINLOCK(ib_mad_port_list_lock);
@@ -377,7 +377,7 @@ struct ib_mad_agent *ib_register_mad_agent(struct ib_device *device,
 	}
 
 	spin_lock_irqsave(&port_priv->reg_lock, flags);
-	mad_agent_priv->agent.hi_tid = ++ib_mad_client_id;
+	mad_agent_priv->agent.hi_tid = atomic_inc_return(&ib_mad_client_id);
 
 	/*
 	 * Make sure MAD registration (if supplied)
-- 
2.13.6

RE: [PATCH] IB/core: Make ib_mad_client_id atomic

[Weiny, Ira]

> 
> Two kernel threads may get the same value for agent.hi_tid, if the agents are
> registered for different ports. As of now, this works, as the agent list is per port.
> 
> It is however confusing and not future robust. Hence, making it atomic.
> 
> Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com>
> Reviewed-by: Jack Morgenstein <jackm@dev.mellanox.co.il>

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> ---
>  drivers/infiniband/core/mad.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/infiniband/core/mad.c b/drivers/infiniband/core/mad.c index
> c50596f7f98a..b28452a55a08 100644
> --- a/drivers/infiniband/core/mad.c
> +++ b/drivers/infiniband/core/mad.c
> @@ -59,7 +59,7 @@ module_param_named(recv_queue_size, mad_recvq_size,
> int, 0444);  MODULE_PARM_DESC(recv_queue_size, "Size of receive queue in
> number of work requests");
> 
>  static struct list_head ib_mad_port_list;
> -static u32 ib_mad_client_id = 0;
> +static atomic_t ib_mad_client_id = ATOMIC_INIT(0);
> 
>  /* Port list lock */
>  static DEFINE_SPINLOCK(ib_mad_port_list_lock);
> @@ -377,7 +377,7 @@ struct ib_mad_agent *ib_register_mad_agent(struct
> ib_device *device,
>  	}
> 
>  	spin_lock_irqsave(&port_priv->reg_lock, flags);
> -	mad_agent_priv->agent.hi_tid = ++ib_mad_client_id;
> +	mad_agent_priv->agent.hi_tid =
> atomic_inc_return(&ib_mad_client_id);
> 
>  	/*
>  	 * Make sure MAD registration (if supplied)
> --
> 2.13.6

Re: [PATCH] IB/core: Make ib_mad_client_id atomic

[Yanjun Zhu]

On 2018/4/18 22:24, Håkon Bugge wrote:
> Two kernel threads may get the same value for agent.hi_tid, if the
> agents are registered for different ports. As of now, this works, as
> the agent list is per port.
>
> It is however confusing and not future robust. Hence, making it
> atomic.
>
> Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com>
> Reviewed-by: Jack Morgenstein <jackm@dev.mellanox.co.il>
Reviewed-by: Zhu Yanjun <yanjun.zhu@oracle.com>
> ---
>   drivers/infiniband/core/mad.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/infiniband/core/mad.c b/drivers/infiniband/core/mad.c
> index c50596f7f98a..b28452a55a08 100644
> --- a/drivers/infiniband/core/mad.c
> +++ b/drivers/infiniband/core/mad.c
> @@ -59,7 +59,7 @@ module_param_named(recv_queue_size, mad_recvq_size, int, 0444);
>   MODULE_PARM_DESC(recv_queue_size, "Size of receive queue in number of work requests");
>   
>   static struct list_head ib_mad_port_list;
> -static u32 ib_mad_client_id = 0;
> +static atomic_t ib_mad_client_id = ATOMIC_INIT(0);
>   
>   /* Port list lock */
>   static DEFINE_SPINLOCK(ib_mad_port_list_lock);
> @@ -377,7 +377,7 @@ struct ib_mad_agent *ib_register_mad_agent(struct ib_device *device,
>   	}
>   
>   	spin_lock_irqsave(&port_priv->reg_lock, flags);
> -	mad_agent_priv->agent.hi_tid = ++ib_mad_client_id;
> +	mad_agent_priv->agent.hi_tid = atomic_inc_return(&ib_mad_client_id);
>   
>   	/*
>   	 * Make sure MAD registration (if supplied)

Re: [PATCH] IB/core: Make ib_mad_client_id atomic

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 801 bytes --]

On Wed, 2018-04-18 at 16:24 +0200, Håkon Bugge wrote:
> Two kernel threads may get the same value for agent.hi_tid, if the
> agents are registered for different ports. As of now, this works, as
> the agent list is per port.
> 
> It is however confusing and not future robust. Hence, making it
> atomic.
> 

People sometimes underestimate the performance penalty of atomic ops. 
Every atomic op is the equivalent of a spin_lock/spin_unlock pair.  This
is why two atomics are worse than taking a spin_lock, doing what you
have to do, and releasing the spin_lock.  Is this really what you want
for a "confusing, let's make it robust" issue?

-- 
Doug Ledford <dledford@redhat.com>
    GPG KeyID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] IB/core: Make ib_mad_client_id atomic

[Jason Gunthorpe]

On Thu, Apr 19, 2018 at 11:55:55PM -0400, Doug Ledford wrote:
> On Wed, 2018-04-18 at 16:24 +0200, Håkon Bugge wrote:
> > Two kernel threads may get the same value for agent.hi_tid, if the
> > agents are registered for different ports. As of now, this works, as
> > the agent list is per port.
> > 
> > It is however confusing and not future robust. Hence, making it
> > atomic.
> > 
> 
> People sometimes underestimate the performance penalty of atomic ops. 
> Every atomic op is the equivalent of a spin_lock/spin_unlock pair.  This
> is why two atomics are worse than taking a spin_lock, doing what you
> have to do, and releasing the spin_lock.  Is this really what you want
> for a "confusing, let's make it robust" issue?

But it is on the ib_register_mad_agent() path which is not a
performance path..

This actually looks like a genuine bug, why is it described only as
'confusing'? ib_register_mad_agent is callable from userspace, so at
least two userspace agents can race and get the same TID's.

TIDs need to be globally unique on the entire machine.

Jason

Re: [PATCH] IB/core: Make ib_mad_client_id atomic

[Håkon Bugge]

+Jack

> On 20 Apr 2018, at 17:34, Jason Gunthorpe <jgg@ziepe.ca> wrote:
> 
> On Thu, Apr 19, 2018 at 11:55:55PM -0400, Doug Ledford wrote:
>> On Wed, 2018-04-18 at 16:24 +0200, Håkon Bugge wrote:
>>> Two kernel threads may get the same value for agent.hi_tid, if the
>>> agents are registered for different ports. As of now, this works, as
>>> the agent list is per port.
>>> 
>>> It is however confusing and not future robust. Hence, making it
>>> atomic.
>>> 
>> 
>> People sometimes underestimate the performance penalty of atomic ops. 
>> Every atomic op is the equivalent of a spin_lock/spin_unlock pair.

Well, may be this holds true if the mutex and the variable is located in the same cacheline.

>>  This
>> is why two atomics are worse than taking a spin_lock, doing what you
>> have to do, and releasing the spin_lock.  Is this really what you want
>> for a "confusing, let's make it robust" issue?
> 
> But it is on the ib_register_mad_agent() path which is not a
> performance path..
> 
> This actually looks like a genuine bug, why is it described only as
> 'confusing'? ib_register_mad_agent is callable from userspace, so at
> least two userspace agents can race and get the same TID’s.

My understanding is that every lookup is using the {port, TID} tuple. As such, it is not a bug, but, very confusing.

> TIDs need to be globally unique on the entire machine.

If you are correct Jason, let me reword the commit message.


Thxs, Håkon

Re: [PATCH] IB/core: Make ib_mad_client_id atomic

[jackm]

On Mon, 23 Apr 2018 16:19:57 +0200
Håkon Bugge <haakon.bugge@oracle.com> wrote:


> > 
> > This actually looks like a genuine bug, why is it described only as
> > 'confusing'? ib_register_mad_agent is callable from userspace, so at
> > least two userspace agents can race and get the same TID’s.  
> 
> My understanding is that every lookup is using the {port, TID} tuple.
> As such, it is not a bug, but, very confusing.
Haakon, you are correct (see snippet from the IB spec, below).

We will NOT have a situation where there are 2 threads/apps
with the same agent ID on the *same port* (accessing the agent ID
allocator is protected by a per-port spinlock). Having the same agent ID
on DIFFERENT ports is OK.
Thus, there is NO bug here. (But as Haakon says, IMHO it is more robust
to avoid having the same agent ID for 2 agents even if those agents are
on different ports).

> 
> > TIDs need to be globally unique on the entire machine.  
> 
Jason, that is not exactly correct.

>From the IB Spec 1.3, C13-18.1.1 (in section 13.4.6.4 - TransactionID
usage):
"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. If the MAD does not have a GRH, its SLID is used
in the combination in place of an SGID."

Since the SGID/SLID is different for each port, the per-port guarantee
of no 2 agents receiving the same agent-ID value is sufficient.

-Jack

Re: [PATCH] IB/core: Make ib_mad_client_id atomic

[Håkon Bugge]

> On 23 Apr 2018, at 21:16, jackm <jackm@dev.mellanox.co.il> wrote:
> 
> On Mon, 23 Apr 2018 16:19:57 +0200
> Håkon Bugge <haakon.bugge@oracle.com> wrote:
> 
> 
>>> 
>>> This actually looks like a genuine bug, why is it described only as
>>> 'confusing'? ib_register_mad_agent is callable from userspace, so at
>>> least two userspace agents can race and get the same TID’s.  
>> 
>> My understanding is that every lookup is using the {port, TID} tuple.
>> As such, it is not a bug, but, very confusing.
> Haakon, you are correct (see snippet from the IB spec, below).
> 
> We will NOT have a situation where there are 2 threads/apps
> with the same agent ID on the *same port* (accessing the agent ID
> allocator is protected by a per-port spinlock). Having the same agent ID
> on DIFFERENT ports is OK.
> Thus, there is NO bug here. (But as Haakon says, IMHO it is more robust
> to avoid having the same agent ID for 2 agents even if those agents are
> on different ports).
> 
>> 
>>> TIDs need to be globally unique on the entire machine.  
>> 
> Jason, that is not exactly correct.
> 
> From the IB Spec 1.3, C13-18.1.1 (in section 13.4.6.4 - TransactionID
> usage):
> "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. If the MAD does not have a GRH, its SLID is used
> in the combination in place of an SGID."
> 
> Since the SGID/SLID is different for each port, the per-port guarantee
> of no 2 agents receiving the same agent-ID value is sufficient.
> 
> -Jack

Shall I interpret this silence as my commit is good to go or that I should add Jack’s tangible information to the commit message?


Thxs, Håkon

Re: [PATCH] IB/core: Make ib_mad_client_id atomic

[jackm]

On Thu, 26 Apr 2018 18:06:10 +0200
Håkon Bugge <haakon.bugge@oracle.com> wrote:

> > On 23 Apr 2018, at 21:16, jackm <jackm@dev.mellanox.co.il> wrote:
> > 
> > On Mon, 23 Apr 2018 16:19:57 +0200
> > Håkon Bugge <haakon.bugge@oracle.com> wrote:
> > 
> >   
> >>> 
> >>> This actually looks like a genuine bug, why is it described only
> >>> as 'confusing'? ib_register_mad_agent is callable from userspace,
> >>> so at least two userspace agents can race and get the same
> >>> TID’s.    
> >> 
> >> My understanding is that every lookup is using the {port, TID}
> >> tuple. As such, it is not a bug, but, very confusing.  
> > Haakon, you are correct (see snippet from the IB spec, below).
> > 
> > We will NOT have a situation where there are 2 threads/apps
> > with the same agent ID on the *same port* (accessing the agent ID
> > allocator is protected by a per-port spinlock). Having the same
> > agent ID on DIFFERENT ports is OK.
> > Thus, there is NO bug here. (But as Haakon says, IMHO it is more
> > robust to avoid having the same agent ID for 2 agents even if those
> > agents are on different ports).
> >   
> >>   
> >>> TIDs need to be globally unique on the entire machine.    
> >>   
> > Jason, that is not exactly correct.
> > 
> > From the IB Spec 1.3, C13-18.1.1 (in section 13.4.6.4 -
> > TransactionID usage):
> > "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. If the MAD does not have a GRH, its
> > SLID is used in the combination in place of an SGID."
> > 
> > Since the SGID/SLID is different for each port, the per-port
> > guarantee of no 2 agents receiving the same agent-ID value is
> > sufficient.
> > 
> > -Jack  
> 
> Shall I interpret this silence as my commit is good to go or that I
> should add Jack’s tangible information to the commit message?
> 
> 
Haakon, I think Jason is on vacation this week. Best to wait for his
response.

-Jack

> Thxs, Håkon
> 

Re: [PATCH] IB/core: Make ib_mad_client_id atomic

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 2097 bytes --]

On Thu, 2018-04-26 at 20:51 +0200, Håkon Bugge wrote:
> > Jason is out this week.  I'll end up processing this one (probably later
> > today).  But I’ll fix up the commit message to suit my tastes when I do.
> 
> Thank you, Doug and Jack,

I reworded the commit message, let me know if you think I worded it
wrong:

commit 69f01b81539c62f3dd96f9f02138ad7b839a0c70 (HEAD -> k.o/wip/dl-for-rc)
Author: Håkon Bugge <haakon.bugge@oracle.com>
Date:   Wed Apr 18 16:24:50 2018 +0200

    IB/core: Make ib_mad_client_id atomic
    
    Currently, kernel protects access to the agent ID allocator on a per
    port basis using a spinlock, so it is impossible for two apps/threads on
    the same port to get the same TID, but it is entirely possible for two
    threads on different ports to end up with the same TID.  As this can be
    confusing (regardless of it being legal according to the IB Spec 1.3,
    C13-18.1.1, in section 13.4.6.4 - TransactionID usage: "Then initiating
    a new operation, MADHeader:TransactionID shall be set to such a value
    that within that MAD the combination of TIG, SGID, and MgmtClass is
    different from that of any other currently executing operation. If the
    MAD does not have a GRH, its SLID is used in the combination in place of
    an SGID." which guarantees we are legal because our different ports will
    have different SGID/SLID creating a unique tuple even if the TIDs are
    identical), and as we might want to open the TID allocator up to more
    parallel usage later, make the TID an atomic type so that no two
    allocations, regardless of port number, will be the same.
    
    Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com>
    Reviewed-by: Jack Morgenstein <jackm@dev.mellanox.co.il>
    Reviewed-by: Ira Weiny <ira.weiny@intel.com>
    Reviewed-by: Zhu Yanjun <yanjun.zhu@oracle.com>
    Signed-off-by: Doug Ledford <dledford@redhat.com>

-- 
Doug Ledford <dledford@redhat.com>
    GPG KeyID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] IB/core: Make ib_mad_client_id atomic

[Håkon Bugge]

> On 27 Apr 2018, at 21:08, Doug Ledford <dledford@redhat.com> wrote:
> 
> On Thu, 2018-04-26 at 20:51 +0200, Håkon Bugge wrote:
>>> Jason is out this week.  I'll end up processing this one (probably later
>>> today).  But I’ll fix up the commit message to suit my tastes when I do.
>> 
>> Thank you, Doug and Jack,
> 
> I reworded the commit message, let me know if you think I worded it
> wrong:
> 
> commit 69f01b81539c62f3dd96f9f02138ad7b839a0c70 (HEAD -> k.o/wip/dl-for-rc)
> Author: Håkon Bugge <haakon.bugge@oracle.com>
> Date:   Wed Apr 18 16:24:50 2018 +0200
> 
>    IB/core: Make ib_mad_client_id atomic
> 
>    Currently, kernel protects access to the agent ID allocator on a per
>    port basis using a spinlock, so it is impossible for two apps/threads on
>    the same port to get the same TID, but it is entirely possible for two
>    threads on different ports to end up with the same TID.  As this can be
>    confusing (regardless of it being legal according to the IB Spec 1.3,
>    C13-18.1.1, in section 13.4.6.4 - TransactionID usage: "Then initiating
>    a new operation, MADHeader:TransactionID shall be set to such a value
>    that within that MAD the combination of TIG, SGID, and MgmtClass is
>    different from that of any other currently executing operation. If the
>    MAD does not have a GRH, its SLID is used in the combination in place of
>    an SGID." which guarantees we are legal because our different ports will
>    have different SGID/SLID creating a unique tuple even if the TIDs are
>    identical), and as we might want to open the TID allocator up to more
>    parallel usage later, make the TID an atomic type so that no two
>    allocations, regardless of port number, will be the same.

The wording is OK per se, but the last sentence spans 12 lines - and as such - a little bit hard to comprehend. I suggest just to refer to IB Spec 1.3, clause C13-18.1.1 and not quote it and make the last sub-sentence (starting with “and as we might want to open”) a sentence by its own.


Thxs, Håkon

> 
>    Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com>
>    Reviewed-by: Jack Morgenstein <jackm@dev.mellanox.co.il>
>    Reviewed-by: Ira Weiny <ira.weiny@intel.com>
>    Reviewed-by: Zhu Yanjun <yanjun.zhu@oracle.com>
>    Signed-off-by: Doug Ledford <dledford@redhat.com>
> 
> -- 
> Doug Ledford <dledford@redhat.com>
>    GPG KeyID: B826A3330E572FDD
>    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

Re: [PATCH] IB/core: Make ib_mad_client_id atomic

[Jason Gunthorpe]

On Mon, Apr 23, 2018 at 10:16:18PM +0300, jackm wrote:

> > > TIDs need to be globally unique on the entire machine.  

> Jason, that is not exactly correct.

The expecation for /dev/umad users is that they all receive locally
unique TID prefixes. The kernel may be OK to keep things port-specific
but it is slightly breaking the API we are presenting to userspace to
allow them to alias..

Jason

Re: [PATCH] IB/core: Make ib_mad_client_id atomic

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 1627 bytes --]

On Mon, 2018-04-30 at 08:49 -0600, Jason Gunthorpe wrote:
> On Mon, Apr 23, 2018 at 10:16:18PM +0300, jackm wrote:
> 
> > > > TIDs need to be globally unique on the entire machine.  
> > Jason, that is not exactly correct.
> 
> The expecation for /dev/umad users is that they all receive locally
> unique TID prefixes. The kernel may be OK to keep things port-specific
> but it is slightly breaking the API we are presenting to userspace to
> allow them to alias..
> 
> Jason

Would people be happier with this commit message then:

IB/core: Make ib_mad_client_id atomic
  
Currently, the kernel protects access to the agent ID allocator on a per
port basis using a spinlock, so it is impossible for two apps/threads on
the same port to get the same TID, but it is entirely possible for two
threads on different ports to end up with the same TID.  

As this can be confusing (regardless of it being legal according to the
IB Spec 1.3, C13-18.1.1, in section 13.4.6.4 - TransactionID usage),
and as the rdma-core user space API for /dev/umad devices implies unique
TIDs even across ports, make the TID an atomic type so that no two
allocations, regardless of port number, will be the same.

Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com>
Reviewed-by: Jack Morgenstein <jackm@dev.mellanox.co.il>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Reviewed-by: Zhu Yanjun <yanjun.zhu@oracle.com>
Signed-off-by: Doug Ledford <dledford@redhat.com>


-- 
Doug Ledford <dledford@redhat.com>
    GPG KeyID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH] IB/core: Make ib_mad_client_id atomic

[Weiny, Ira]

Looks fine to me.

> -----Original Message-----
> From: Doug Ledford [mailto:dledford@redhat.com]
> Sent: Monday, April 30, 2018 10:11 AM
> To: Jason Gunthorpe <jgg@ziepe.ca>; jackm <jackm@dev.mellanox.co.il>
> Cc: Håkon Bugge <haakon.bugge@oracle.com>; Hiatt, Don
> <don.hiatt@intel.com>; Dasaratharaman Chandramouli
> <dasaratharaman.chandramouli@intel.com>; Weiny, Ira <ira.weiny@intel.com>;
> Hefty, Sean <sean.hefty@intel.com>; OFED mailing list <linux-
> rdma@vger.kernel.org>; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] IB/core: Make ib_mad_client_id atomic
> 
> On Mon, 2018-04-30 at 08:49 -0600, Jason Gunthorpe wrote:
> > On Mon, Apr 23, 2018 at 10:16:18PM +0300, jackm wrote:
> >
> > > > > TIDs need to be globally unique on the entire machine.
> > > Jason, that is not exactly correct.
> >
> > The expecation for /dev/umad users is that they all receive locally
> > unique TID prefixes. The kernel may be OK to keep things port-specific
> > but it is slightly breaking the API we are presenting to userspace to
> > allow them to alias..
> >
> > Jason
> 
> Would people be happier with this commit message then:
> 
> IB/core: Make ib_mad_client_id atomic
> 
> Currently, the kernel protects access to the agent ID allocator on a per port basis
> using a spinlock, so it is impossible for two apps/threads on the same port to get
> the same TID, but it is entirely possible for two threads on different ports to end
> up with the same TID.
> 
> As this can be confusing (regardless of it being legal according to the IB Spec 1.3,
> C13-18.1.1, in section 13.4.6.4 - TransactionID usage), and as the rdma-core user
> space API for /dev/umad devices implies unique TIDs even across ports, make
> the TID an atomic type so that no two allocations, regardless of port number, will
> be the same.
> 
> Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com>
> Reviewed-by: Jack Morgenstein <jackm@dev.mellanox.co.il>
> Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> Reviewed-by: Zhu Yanjun <yanjun.zhu@oracle.com>
> Signed-off-by: Doug Ledford <dledford@redhat.com>
> 
> 
> --
> Doug Ledford <dledford@redhat.com>
>     GPG KeyID: B826A3330E572FDD
>     Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

Re: [PATCH] IB/core: Make ib_mad_client_id atomic

[Jason Gunthorpe]

On Mon, Apr 30, 2018 at 01:10:49PM -0400, Doug Ledford wrote:
> On Mon, 2018-04-30 at 08:49 -0600, Jason Gunthorpe wrote:
> > On Mon, Apr 23, 2018 at 10:16:18PM +0300, jackm wrote:
> > 
> > > > > TIDs need to be globally unique on the entire machine.  
> > > Jason, that is not exactly correct.
> > 
> > The expecation for /dev/umad users is that they all receive locally
> > unique TID prefixes. The kernel may be OK to keep things port-specific
> > but it is slightly breaking the API we are presenting to userspace to
> > allow them to alias..
> > 
> > Jason
> 
> Would people be happier with this commit message then:
> 
> IB/core: Make ib_mad_client_id atomic
>   
> Currently, the kernel protects access to the agent ID allocator on a per
> port basis using a spinlock, so it is impossible for two apps/threads on
> the same port to get the same TID, but it is entirely possible for two
> threads on different ports to end up with the same TID.  
> 
> As this can be confusing (regardless of it being legal according to the
> IB Spec 1.3, C13-18.1.1, in section 13.4.6.4 - TransactionID usage),
> and as the rdma-core user space API for /dev/umad devices implies unique
> TIDs even across ports, make the TID an atomic type so that no two
> allocations, regardless of port number, will be the same.
> 
> Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com>
> Reviewed-by: Jack Morgenstein <jackm@dev.mellanox.co.il>
> Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> Reviewed-by: Zhu Yanjun <yanjun.zhu@oracle.com>
> Signed-off-by: Doug Ledford <dledford@redhat.com>

fine for me

Jason

Re: [PATCH] IB/core: Make ib_mad_client_id atomic

[jackm]

On Mon, 30 Apr 2018 13:10:49 -0400
Doug Ledford <dledford@redhat.com> wrote:

Looks good!

-Jack

> On Mon, 2018-04-30 at 08:49 -0600, Jason Gunthorpe wrote:
> > On Mon, Apr 23, 2018 at 10:16:18PM +0300, jackm wrote:
> >   
> > > > > TIDs need to be globally unique on the entire machine.    
> > > Jason, that is not exactly correct.  
> > 
> > The expecation for /dev/umad users is that they all receive locally
> > unique TID prefixes. The kernel may be OK to keep things
> > port-specific but it is slightly breaking the API we are presenting
> > to userspace to allow them to alias..
> > 
> > Jason  
> 
> Would people be happier with this commit message then:
> 
> IB/core: Make ib_mad_client_id atomic
>   
> Currently, the kernel protects access to the agent ID allocator on a
> per port basis using a spinlock, so it is impossible for two
> apps/threads on the same port to get the same TID, but it is entirely
> possible for two threads on different ports to end up with the same
> TID.  
> 
> As this can be confusing (regardless of it being legal according to
> the IB Spec 1.3, C13-18.1.1, in section 13.4.6.4 - TransactionID
> usage), and as the rdma-core user space API for /dev/umad devices
> implies unique TIDs even across ports, make the TID an atomic type so
> that no two allocations, regardless of port number, will be the same.
> 
> Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com>
> Reviewed-by: Jack Morgenstein <jackm@dev.mellanox.co.il>
> Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> Reviewed-by: Zhu Yanjun <yanjun.zhu@oracle.com>
> Signed-off-by: Doug Ledford <dledford@redhat.com>
> 
> 

Re: [PATCH] IB/core: Make ib_mad_client_id atomic

[Håkon Bugge]

> On 1 May 2018, at 06:38, jackm <jackm@dev.mellanox.co.il> wrote:
> 
> On Mon, 30 Apr 2018 13:10:49 -0400
> Doug Ledford <dledford@redhat.com> wrote:
> 
> Looks good!

Yes, absolutely!

Håkon

> 
> -Jack
> 
>> On Mon, 2018-04-30 at 08:49 -0600, Jason Gunthorpe wrote:
>>> On Mon, Apr 23, 2018 at 10:16:18PM +0300, jackm wrote:
>>> 
>>>>>> TIDs need to be globally unique on the entire machine.    
>>>> Jason, that is not exactly correct.  
>>> 
>>> The expecation for /dev/umad users is that they all receive locally
>>> unique TID prefixes. The kernel may be OK to keep things
>>> port-specific but it is slightly breaking the API we are presenting
>>> to userspace to allow them to alias..
>>> 
>>> Jason  
>> 
>> Would people be happier with this commit message then:
>> 
>> IB/core: Make ib_mad_client_id atomic
>> 
>> Currently, the kernel protects access to the agent ID allocator on a
>> per port basis using a spinlock, so it is impossible for two
>> apps/threads on the same port to get the same TID, but it is entirely
>> possible for two threads on different ports to end up with the same
>> TID.  
>> 
>> As this can be confusing (regardless of it being legal according to
>> the IB Spec 1.3, C13-18.1.1, in section 13.4.6.4 - TransactionID
>> usage), and as the rdma-core user space API for /dev/umad devices
>> implies unique TIDs even across ports, make the TID an atomic type so
>> that no two allocations, regardless of port number, will be the same.
>> 
>> Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com>
>> Reviewed-by: Jack Morgenstein <jackm@dev.mellanox.co.il>
>> Reviewed-by: Ira Weiny <ira.weiny@intel.com>
>> Reviewed-by: Zhu Yanjun <yanjun.zhu@oracle.com>
>> Signed-off-by: Doug Ledford <dledford@redhat.com>
>> 
>> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html