libibmad: To reserve upper 8 bits of tid used by solaris SRIOV driver
PATCH 1/1 libibmad: To reserve upper 8 bits of tid used by solaris SRIOV driver
From: Brendan Doyle <[email protected]>
---src/mad.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
src/mad.c +0 -0
@@ -62,6 +62,9 @@ uint64_t mad_trid(void)
trid = random(); } next = ++trid | (base << 32); +#if defined(__SVR4) && defined(__sun) + next &= 0x00ffffffffffffff; +#endif return next; }
Signature · 7 lines
1.7.1 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Why does this need to be done in user space? This seems like something which should be done in the kernel for all Transaction ID's which might be sent to the umad layer.
Ira
Quoted 22 lines · Boris Chiu
From: Brendan Doyle <[email protected]>
Signed-off-by Brendan Doyle <[email protected]>---
src/mad.c| 3 +++1 files changed, 3 insertions(+), 0 deletions(-)src/mad.c +3
-1@@ -62,6 +62,9 @@ uint64_t mad_trid(void) trid =random (); } next = ++trid | (base <<32 ); +#if defined(__SVR4) && defined(__sun) + next &=0x00ffffffffffffff ; +#endif return next; } --
Quoted 3 lines
1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 3 lines · Ira Weiny
Why does this need to be done in user space? This seems like something which should be done in the kernel for all Transaction ID's which might be sent to the umad layer.
Agreed, if Solaris is going to emulate the Linux umad dev FD it should be done properly. The kernel overrides some bits and returns the TRID it actually put on the wire after the MAD is sent.
Linux is already doing this to add per-process uniqueness, per-guest SRIOV uniqueness should use the same mechanism.
Thus, this masking is either redundant, or hiding a kernel bug..
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 14 lines · Boris Chiu
fyi,
Boris
-------- Original Message --------
Subject: Re: [PATCH] libibmad: To reserve upper 8 bits of tid used by solaris SRIOV driver
Date: Wed, 20 Mar 2013 16:33:22 -0600
From: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
To: Ira Weiny <[email protected]>
CC: Boris Chiu <[email protected]>, [email protected]
Quoted 4 lines
Quoted 3 lines · Ira Weiny
Why does this need to be done in user space? This seems like something which should be done in the kernel for all Transaction ID's which might be sent to the umad layer.
So the reason we "reserved" not set the upper two bytes of TID in the user space is so that libs and/or applications that rely on TID matching won't break. If we changed the TID in the kernel then the response TID returned to the lib/application would not match the request TID.
Currently the only place I see TID check is in libibmad's _do_madrpc(), and here it effectively does something similar where it truncates the TID to 32bits. We send a MAD with a full 64 bit TID, record the lower 32 bits of the TID, on on receipt of the response check that the lower 32 bits of TID are the same, the upper 32 could be completely different.
Quoted 4 lines
Agreed, if Solaris is going to emulate the Linux umad dev FD it should be done properly. The kernel overrides some bits and returns the TRID it actually put on the wire after the MAD is sent.
I don't see where the TID sent is returned to the caller of umad_send(), I see that we pass down a MAD over the fd to the kernel that has a full 64bit TID and then I see that the kernel over writes the upper 32 bits, but I don't see how that is communicated back to the sender of the MAD, so if the sender were doing 64 bit TID matching it would not find its MAD, the upper 32bits it set are lost.
Note in what we propose, in the userland we just reserve the upper byte, it is the kernel that then uses/sets and unsets this upper byte to include a VF id so MADs send on a VF device can get tunneled to the PF and received on a PF can be directed to the correct VF, the upper byte is cleared before handing back to the userland so that it can do 64 bit TID matching and the TID it specified in the request MAD is the same as the one it gets in the response MAD.
Quoted 3 lines
Linux is already doing this to add per-process uniqueness, per-guest SRIOV uniqueness should use the same mechanism.
The way it is done in Linux effectively makes the TID a 32bit entity from the userland perspective.
Quoted 2 lines
Thus, this masking is either redundant, or hiding a kernel bug..
I might say it is the other way around. In Linux from the userland the TID you get in a response MAD is not the same as the one you specified in the request MAD.
I'm not debating that the kernel should use upper bits of the TID to demux per process, per guest, that is what we do in Solaris, what I'm saying is that those bits used by the kernel should be "reserved" in userland so that the TID of its request and response are the same across all 64 bits.
Brendan
Quoted 3 lines
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 16 lines
-----Original Message-----
From: brendan doyle [mailto:[email protected]]Quoted 11 lines · Boris Chiu
fyi,
Boris
-------- Original Message --------
Subject: Re: [PATCH] libibmad: To reserve upper 8 bits of tid used by
solaris SRIOV driver
Quoted 6 lines
Quoted 4 lines · Ira Weiny
Quoted 3 lines
Why does this need to be done in user space? This seems like something which should be done in the kernel for all Transaction ID's which might be sent to the umad layer.
So the reason we "reserved" not set the upper two bytes of TID
You mean 1 byte?
Quoted 4 lines
in the user
space is so that libs and/or applications that rely on TID matching won't break. If we changed the TID in the kernel then the response TID returned to the lib/application would not match the request TID.
While this makes sense I can't believe that libibmad is the only place which TID's get generated. Do you also do this in OpenSM?
Quoted 7 lines
Currently the only place I see TID check is in libibmad's
_do_madrpc(), and here it effectively does something similar where it truncates the TID to 32bits. We send a MAD with a full 64 bit TID, record the lower 32 bits of the TID, on on receipt of the response check that the lower 32 bits of TID are the same, the upper 32 could be completely different.
Right, it ignores the upper 32bits because it knows the umad layer reserves the upper 32bits. So for libibmad your patch does not fix anything unless you are doing some 64bit compare of TID's above the libibmad layer?
Quoted 11 lines
Quoted 4 lines
Agreed, if Solaris is going to emulate the Linux umad dev FD it should be done properly. The kernel overrides some bits and returns the TRID it actually put on the wire after the MAD is sent.
I don't see where the TID sent is returned to the caller of
umad_send(), I see that we pass down a MAD over the fd to the kernel that has a full 64bit TID and then I see that the kernel over writes the upper 32 bits, but I don't see how that is communicated back to the sender of the MAD, so if the sender were doing
64 bit TID matching it would not find its MAD, the upper 32bits it set are lost.
I believe you are correct. It is just up to the user of umad to "know" that the upper 32bits are not valid. While unfortunate, this can only really be fixed through a major change in the umad ABI and/or documentation of that library.
Quoted 8 lines
Note in what we propose, in the userland we just reserve the upper byte, it is the kernel that then uses/sets and unsets this upper byte to include a VF id so MADs send on a VF device can get tunneled to the PF and received on a PF can be directed to the correct VF, the upper byte is cleared before handing back to the userland so that it can do 64 bit TID matching and the TID it specified in the request MAD is the same as the one it gets in the response MAD.
This makes sense but I worry about users who do not use libibmad. Frankly, I think people should be moving away from using libibmad. It is very complicated and undocumented. Putting transaction ID processing like this should probably be in libibumad since it reserves and uses bits from the TID.
Quoted 8 lines
Quoted 4 lines
Linux is already doing this to add per-process uniqueness, per-guest SRIOV uniqueness should use the same mechanism.
The way it is done in Linux effectively makes the TID a 32bit entity from the userland perspective.
Yep, and I wish it was better documented and/or done a different way.
Quoted 7 lines
Quoted 3 lines
Thus, this masking is either redundant, or hiding a kernel bug..
I might say it is the other way around. In Linux from the userland the TID you get in a response MAD is not the same as the one you specified in the request MAD.
And this is yet another unfortunate "gotcha" of the libibumad interface.
Quoted 5 lines
I'm not debating that the kernel should use upper bits of the TID to demux per process, per guest, that is what we do in Solaris, what I'm saying is that those bits used by the kernel should be "reserved" in userland so that the TID of its request and response are the same across all 64 bits.
So in Solaris do you combine the reservation of umad and this SRIOV like this:
0xXX YYYYYY ZZZZZZZZ
Where XX is the VF mask YYYYYY is the agent mask and ZZZZZZZZ is the "actual" TID?
It seems like we should standardize on some format and then put this functionality into libibumad where all software can consistently use it.
Or leave things the way they are and define the TID to be 32bits in userspace and leave the rest up to the kernel. In that respect libibmad probably should not be generating 64bit TID's.
I think I would rather see a patch which generates a 32bit TID from mad_trid. Would you be happy with that?
Ira
Quoted 6 lines
Brendan
Quoted 3 lines
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 16 lines · Ira
Quoted 16 lines
-----Original Message-----
From: brendan doyle [mailto:[email protected]]Quoted 11 lines · Boris Chiu
fyi,
Boris
-------- Original Message --------
Subject: Re: [PATCH] libibmad: To reserve upper 8 bits of tid used by
solaris SRIOV driver
Quoted 6 lines
Quoted 5 lines
Quoted 4 lines · Ira Weiny
Quoted 3 lines
Why does this need to be done in user space? This seems like something which should be done in the kernel for all Transaction ID's which might be sent to the umad layer.
So the reason we "reserved" not set the upper two bytes of TID
You mean 1 byte?
yeah sorry typo..
Quoted 5 lines
Quoted 4 lines
in the user
space is so that libs and/or applications that rely on TID matching won't break. If we changed the TID in the kernel then the response TID returned to the lib/application would not match the request TID.While this makes sense I can't believe that libibmad is the only place which TID's get generated. Do you also do this in OpenSM?
I didn't personal check, butt I ask a colleague who was making the change to check this specifically, and the answer was that all tids were set/generated via mad_trid().
Quoted 7 lines
Quoted 6 lines
Currently the only place I see TID check is in libibmad's
_do_madrpc(), and here it effectively does something similar where it truncates the TID to 32bits. We send a MAD with a full 64 bit TID, record the lower 32 bits of the TID, on on receipt of the response check that the lower 32 bits of TID are the same, the upper 32 could be completely different.Right, it ignores the upper 32bits because it knows the umad layer reserves the upper 32bits. So for libibmad your patch does not fix anything unless you are doing some 64bit compare of TID's above the libibmad layer?
we are not so, i agree you could say that patch is moot, but I do think that tids can be handled better than they are, and that if we expect the kernel to use some of the upper bytes, then they should be marked reserved in userland.
Quoted 10 lines
Quoted 9 lines
Quoted 3 lines
Agreed, if Solaris is going to emulate the Linux umad dev FD it should be done properly. The kernel overrides some bits and returns the TRID it actually put on the wire after the MAD is sent.
I don't see where the TID sent is returned to the caller of
umad_send(), I see that we pass down a MAD over the fd to the kernel that has a full 64bit TID and then I see that the kernel over writes the upper 32 bits, but I don't see how that is communicated back to the sender of the MAD, so if the sender were doing
64 bit TID matching it would not find its MAD, the upper 32bits it set are lost.I believe you are correct. It is just up to the user of umad to "know" that the upper 32bits are not valid. While unfortunate, this can only really be fixed through a major change in the umad ABI and/or documentation of that library.
noted.
Quoted 8 lines
Quoted 7 lines
Note in what we propose, in the userland we just reserve the upper byte, it is the kernel that then uses/sets and unsets this upper byte to include a VF id so MADs send on a VF device can get tunneled to the PF and received on a PF can be directed to the correct VF, the upper byte is cleared before handing back to the userland so that it can do 64 bit TID matching and the TID it specified in the request MAD is the same as the one it gets in the response MAD.
This makes sense but I worry about users who do not use libibmad. Frankly, I think people should be moving away from using libibmad. It is very complicated and undocumented. Putting transaction ID processing like this should probably be in libibumad since it reserves and uses bits from the TID.
Humm, so may be getting beyond the scope of this patch, I think there are a lot of good parts to libibmad, especially the packing and unpacking/parsing of the MAD pkt, and if we were to drop libibmad these should be rolled into libibumd. Included in that should be a function to get a tid, and that all users of the umad interface must get their tid using that function, then we can enforce a consistent policy for tids.
Quoted 17 lines
Quoted 5 lines
Quoted 2 lines
Linux is already doing this to add per-process uniqueness, per-guest SRIOV uniqueness should use the same mechanism.
The way it is done in Linux effectively makes the TID a 32bit entity from the userland perspective.
Yep, and I wish it was better documented and/or done a different way.
Quoted 4 lines
Quoted 1 line
Thus, this masking is either redundant, or hiding a kernel bug..
I might say it is the other way around. In Linux from the userland the TID you get in a response MAD is not the same as the one you specified in the request MAD.
And this is yet another unfortunate "gotcha" of the libibumad interface.
Quoted 4 lines
I'm not debating that the kernel should use upper bits of the TID to demux per process, per guest, that is what we do in Solaris, what I'm saying is that those bits used by the kernel should be "reserved" in userland so that the TID of its request and response are the same across all 64 bits.
Quoted 5 lines
So in Solaris do you combine the reservation of umad and this SRIOV like this:
0xXX YYYYYY ZZZZZZZZ
Where XX is the VF mask YYYYYY is the agent mask and ZZZZZZZZ is the "actual" TID?
Actually we just reserve the upper byte, we currently don't include an agent mask, but that's something we could certainly add.
Quoted 2 lines
It seems like we should standardize on some format and then put this functionality into libibumad where all software can consistently use it.
I'd agree.
Quoted 2 lines
Or leave things the way they are and define the TID to be 32bits in userspace and leave the rest up to the kernel. In that respect libibmad probably should not be generating 64bit TID's.
So I think that could be a short term thing, until we have an agreed standard as above.
Quoted 2 lines
I think I would rather see a patch which generates a 32bit TID from
mad_trid. Would you be happy with that?
yeup, I'll ask Boris to rework the patch.
Thanks
Quoted 8 lines
Ira
Quoted 5 lines
Brendan
Quoted 3 lines
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 10 lines · brendan doyle
Quoted 4 lines · Ira Weiny
Quoted 3 lines
Why does this need to be done in user space? This seems like something which should be done in the kernel for all Transaction ID's which might be sent to the umad layer.
So the reason we "reserved" not set the upper two bytes of TID in the user space is so that libs and/or applications that rely on TID matching won't break. If we changed the TID in the kernel then the response TID returned to the lib/application would not match the request TID.
This is already the case, only the lower 32 bits are available for application use, the upper 32 bits are all reserved for the kernel, and you can learn what they are by doing a SMP to the local SMA. IIRC some of my stuff captures the upper 32 bits when it gets the first reply - but all userspace demux is only done on the low 32 bits anyhow.
It is mysterious why umad sends in all 64 bits, but it isn't harmful. If you are going to change it then send 32 bits only.
Quoted 11 lines
Quoted 3 lines
Agreed, if Solaris is going to emulate the Linux umad dev FD it should be done properly. The kernel overrides some bits and returns the TRID it actually put on the wire after the MAD is sent.
I don't see where the TID sent is returned to the caller of
umad_send(), I see that we pass down a MAD over the fd to the kernel that has a full 64bit TID and then I see that the kernel over writes the upper 32 bits, but I don't see how that is communicated back to the sender of the MAD, so if the sender were doing 64 bit TID matching it would not find its MAD, the upper 32bits it set are lost.
Certainly, the umad API is problematic in many ways :|
Quoted 7 lines
Note in what we propose, in the userland we just reserve the upper byte, it is the kernel that then uses/sets and unsets this upper byte to include a VF id so MADs send on a VF device can get tunneled to the PF and received on a PF can be directed to the correct VF, the upper byte is cleared before handing back to the userland so that it can do 64 bit TID matching and the TID it specified in the request MAD is the same as the one it gets in the response MAD.
Masking the TID off in messages coming from the kernel to umad is a really bad idea, the TID returned to user space should exactly match the on-the-wire value. This is why 64 bits are allocated for a 32 bit value. Otherwise you have to create ugly special cases for requests vs reply vs trap and it precludes userspace producing sensible packet traces and the like.
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Ok so I think we are essentially on the same page...
Quoted 2 lines
This is already the case, only the lower 32 bits are available for application use, the upper 32 bits are all reserved for the kernel,
This is not an IB spec defined delineation, but an OF one. Perhaps there was some community discussion to architect it this way but it is not documented. At the very least this patch should document this.
So how about modifying the patch such that mad_trid() is something like:
#define GET_IB_USERLAND_TID(tid) (tid & 0x00000000ffffffff) /*
* Generate the 64 bit MAD transaction ID. The upper 32 bits are
reserved for use
* by the kernel. We clear the upper 32 bits here, but MADs received
from the kernel
* may contain kernel specific data in these bits, consequently
userland TID matching
* should only be done on the lower 32 bits.
*/uint64_t mad_trid(void)
{
static uint64_t trid;
uint64_t next;
if (!trid) {
srandom((int)time(0) * getpid());
trid = random();
}
next = ++trid;
next = GET_IB_USERLAND_TID(next);
return next;
}and in _do_madrpc
-uint32_t trid; /* only low 32 bits */
+uint32_t trid; /* only low 32 bits - see mad_trid() */
Brendan
Quoted 40 lines · Jason Gunthorpe
Quoted 9 lines · brendan doyle
Quoted 4 lines · Ira Weiny
Quoted 3 lines
Why does this need to be done in user space? This seems like something which should be done in the kernel for all Transaction ID's which might be sent to the umad layer.
So the reason we "reserved" not set the upper two bytes of TID in the user space is so that libs and/or applications that rely on TID matching won't break. If we changed the TID in the kernel then the response TID returned to the lib/application would not match the request TID.
This is already the case, only the lower 32 bits are available for application use, the upper 32 bits are all reserved for the kernel, and you can learn what they are by doing a SMP to the local SMA. IIRC some of my stuff captures the upper 32 bits when it gets the first reply - but all userspace demux is only done on the low 32 bits anyhow.
It is mysterious why umad sends in all 64 bits, but it isn't harmful. If you are going to change it then send 32 bits only.
Quoted 10 lines
Quoted 3 lines
Agreed, if Solaris is going to emulate the Linux umad dev FD it should be done properly. The kernel overrides some bits and returns the TRID it actually put on the wire after the MAD is sent.
I don't see where the TID sent is returned to the caller of
umad_send(), I see that we pass down a MAD over the fd to the kernel that has a full 64bit TID and then I see that the kernel over writes the upper 32 bits, but I don't see how that is communicated back to the sender of the MAD, so if the sender were doing 64 bit TID matching it would not find its MAD, the upper 32bits it set are lost.Certainly, the umad API is problematic in many ways :|
Quoted 7 lines
Note in what we propose, in the userland we just reserve the upper byte, it is the kernel that then uses/sets and unsets this upper byte to include a VF id so MADs send on a VF device can get tunneled to the PF and received on a PF can be directed to the correct VF, the upper byte is cleared before handing back to the userland so that it can do 64 bit TID matching and the TID it specified in the request MAD is the same as the one it gets in the response MAD.
Quoted 8 lines
Masking the TID off in messages coming from the kernel to umad is a really bad idea, the TID returned to user space should exactly match the on-the-wire value. This is why 64 bits are allocated for a 32 bit value. Otherwise you have to create ugly special cases for requests vs reply vs trap and it precludes userspace producing sensible packet traces and the like.
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 9 lines · brendan doyle
Ok so I think we are essentially on the same page...
Quoted 2 lines
This is already the case, only the lower 32 bits are available for application use, the upper 32 bits are all reserved for the kernel,
This is not an IB spec defined delineation, but an OF one. Perhaps there was some community discussion to architect it this way but it is not documented. At the very least this patch should document this.
Well, it is how /dev/umad works - IBA does not define the umad api, Solaris is copying the Linux umad API, so it should work the same..
There is no need to specify the format of the upper 32 bits, it is kernel specific and has no impact on the application. The kernel should use those bits as necessary to demux flows between users of umad, be they processes or VMs.
Quoted 1 line
So how about modifying the patch such that
mad_trid()is something like:
Looks like a reasonable clarification of existing usage to me.
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 12 lines · Jason Gunthorpe
Quoted 8 lines · brendan doyle
Ok so I think we are essentially on the same page...
Quoted 2 lines
This is already the case, only the lower 32 bits are available for application use, the upper 32 bits are all reserved for the kernel,
This is not an IB spec defined delineation, but an OF one. Perhaps there was some community discussion to architect it this way but it is not documented. At the very least this patch should document this.
Well, it is how
/dev/umadworks - IBA does not define the umad api, Solaris is copying the Linux umad API, so it should work the same..
Quoted 7 lines
There is no need to specify the format of the upper 32 bits, it is kernel specific and has no impact on the application. The kernel should use those bits as necessary to demux flows between users of umad, be they processes or VMs.
Quoted 1 line
So how about modifying the patch such that
mad_trid()is something like:Looks like a reasonable clarification of existing usage to me.
Great, so we'll modify the patch and resubmit.
Thanks
Quoted 2 lines
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html