Re: [PATCH v2 4/8] NTB: ntb_pingpong: Choose doorbells based on port number
On 24/07/18 12:12 PM, Allen Hubbe wrote: > On Tue, Jul 24, 2018 at 1:37 PM Logan Gunthorpe wrote: >> Not really. Given that we know there are only two peers, we always use >> the other side's doorbell register. You'd only use the nearby doorbell >> register if you wanted to trigger your own interrupt -- that would be >> weird and we don't really have the API sophistication to do that. >> >> If we wanted to support multiple peers with some number in crosslink >> then we'd need to revamp things _significantly_. In this case we'd have >> multiple doorbell registers which each apply to a different subset of >> peers. This gets _very_ complicated and hurts my head. > > ...huh, looks like peer index was omitted from ntb_peer_db_set and > friends. Adding peer index there would make the interface consistent > with other ntb_peer functions. Peer index would allow the hw driver > to select which doorbell register to use for each peer. Adding a > ntb_peer_db_valid_bits to that would allow a subset of bits in the > shared register to be associated with each peer. The way the switch hardware works (switchtec and I assume IDT) is that there is only one doorbell register for all peers which can trigger any peer that doesn't have the corresponding bit masked. So the way it was designed without the index maxes sense except when we start to add multi-peer crosslink insanity. I don't think adding a peer index would be sufficient as the clients would need to know which peers share which doorbells. If I were to attempt something like this, I'd probably look at introducing a segment index to the entire API. So that each switch in a crosslink topology is it's own segment with it's own set of peers, peer index space, doorbell registers, etc. But that's all very complicated and messy and probably something I wouldn't even look at until we have finished with traditional multi-host setups. > I thought for sure ntb_peer_db_set already had peer index, and I was > wrong. Go ahead with the change as in your patch, I won't force the > issue or that you to do that extra work and touch all the drivers > again for this. It can be addressed when there is renued interest in > making things work more than one peer. > > This patch, and the others in this series: > Acked-by: Allen Hubbe Thanks! Logan
Re: [PATCH v2 4/8] NTB: ntb_pingpong: Choose doorbells based on port number
On Tue, Jul 24, 2018 at 1:37 PM Logan Gunthorpe wrote: > Not really. Given that we know there are only two peers, we always use > the other side's doorbell register. You'd only use the nearby doorbell > register if you wanted to trigger your own interrupt -- that would be > weird and we don't really have the API sophistication to do that. > > If we wanted to support multiple peers with some number in crosslink > then we'd need to revamp things _significantly_. In this case we'd have > multiple doorbell registers which each apply to a different subset of > peers. This gets _very_ complicated and hurts my head. ...huh, looks like peer index was omitted from ntb_peer_db_set and friends. Adding peer index there would make the interface consistent with other ntb_peer functions. Peer index would allow the hw driver to select which doorbell register to use for each peer. Adding a ntb_peer_db_valid_bits to that would allow a subset of bits in the shared register to be associated with each peer. I think that's all that would need to change, not significantly more, to support multiple doorbell registers associated with different subsets of peers. The complication would at least be hidden in the hw driver, where it would need to maintain some mapping from peer index to the right set of registers. > But as I said, > I'm not trying to add new functionality for multi-peer crosslink or > anything like that. I'm just trying to fix the 2 crosslink peer case so > it works like it did when it was originally merged. I thought for sure ntb_peer_db_set already had peer index, and I was wrong. Go ahead with the change as in your patch, I won't force the issue or that you to do that extra work and touch all the drivers again for this. It can be addressed when there is renued interest in making things work more than one peer. This patch, and the others in this series: Acked-by: Allen Hubbe
Re: [PATCH v2 4/8] NTB: ntb_pingpong: Choose doorbells based on port number
On 24/07/18 11:26 AM, Allen Hubbe wrote: > On Mon, Jul 23, 2018 at 12:08 PM Logan Gunthorpe wrote: >> I don't think you'll ever have a case where two peers have the same >> index, as the index is really an abstract concept the hardware doesn't >> really know about. > > That is the point of index, that there should never be two peers with > the same index, and also that the range of index values is bounded. Yes, I think we are making the same point. > Port numbers are problematic, so I'm worried about the change to use > port number in the client drivers instead of using index. For > example, this change assumes that the index value is < bits per long > long, because the value is used in BIT_ULL(port number). Huh?, I'm not making that change... We still use the index to refer to the peer, but the resource we are using is based on the port number, just as it was before my changes. Perhaps you can point out in my patches what change you are worried about? > Maybe I'm missing something... In the crosslink case, there is > another doorbell register on the other side of the crosslink. Whether > to use the nearby or via-crosslink doorbell depends on the peer > index... making assumptions about the hw driver, but is that about > right? Not really. Given that we know there are only two peers, we always use the other side's doorbell register. You'd only use the nearby doorbell register if you wanted to trigger your own interrupt -- that would be weird and we don't really have the API sophistication to do that. If we wanted to support multiple peers with some number in crosslink then we'd need to revamp things _significantly_. In this case we'd have multiple doorbell registers which each apply to a different subset of peers. This gets _very_ complicated and hurts my head. But as I said, I'm not trying to add new functionality for multi-peer crosslink or anything like that. I'm just trying to fix the 2 crosslink peer case so it works like it did when it was originally merged. Logan
Re: [PATCH v2 4/8] NTB: ntb_pingpong: Choose doorbells based on port number
On Mon, Jul 23, 2018 at 12:08 PM Logan Gunthorpe wrote: > I don't think you'll ever have a case where two peers have the same > index, as the index is really an abstract concept the hardware doesn't > really know about. That is the point of index, that there should never be two peers with the same index, and also that the range of index values is bounded. Port numbers are problematic, so I'm worried about the change to use port number in the client drivers instead of using index. For example, this change assumes that the index value is < bits per long long, because the value is used in BIT_ULL(port number). Maybe I'm missing something... In the crosslink case, there is another doorbell register on the other side of the crosslink. Whether to use the nearby or via-crosslink doorbell depends on the peer index... making assumptions about the hw driver, but is that about right? Then you are selecting bits in the doorbell register based on port number, ok, that must be how the bits of the shared db register are associated with ports in your configuration. Maybe what's actually needed is a ntb_peer_db_valid_mask(peer index), and if only the port-numbered db bit (or any other combination) is valid for that peer, so be it, that can be an implementation choice of the hw driver and below.
Re: [PATCH v2 4/8] NTB: ntb_pingpong: Choose doorbells based on port number
On 23/07/18 08:01 AM, Allen Hubbe wrote: > Does this solve the issue where two of the the port numbers are the > same, because of symmetry over a crosslink? I think the two ports > with the "same" number should be identified as different peer index, > even if the port numbers are the same. I'm not sure if it's that general. It solves it for the case that both peers have port numbers equal to zero. It doesn't solve it for the more general case where you may have multiple partitions, including one or more crosslink partitions. That is _much_ harder to solve and right now I'm only focused on fixing the code to work as it did before. If someone is interested in making a complex setup like that work, they'll have to figure out how and post patches. I don't think you'll ever have a case where two peers have the same index, as the index is really an abstract concept the hardware doesn't really know about. > Maybe the port of any peer connected over the crosslink should be the > local switch's crosslink port. Well, the switches could in theory be configured in any way. But the whole point and benefit of crosslink is for both switches to be configured to be identical. The logical configuration is to configure both hosts to be on port 0 for both switches and there's really no benefit to doing it differently. > The local port number might be needed > to configure translation tables in the local switch. It's not. The crosslink partition between the switches would typically always be "port number" 1. (Though there may be instances where it's a different port number, but that shouldn't effect anything and is handled by the driver.) > If a globally > unique port number is needed, maybe encode a chip number in some high > bits of the port number? With crosslink, we can't figure out a chip number for the same reasons we can't figure out a port number. > If a locally unique port number is needed, > maybe encode a path, that could be useful for configuring address > translations across multiple crosslinks. Encoding a path, then each > port will have a different number, depending on the perspective of the > source port, which could be confusing (already, peer index is local > perspective, so can cause the same kind of confusion). I don't follow this. Any path you might get will be exactly the same for both hosts in a symmetric crosslink configuration. The only way to find a locally unique port number would be for the two hosts to negotiate somehow and that's cumbersome and would introduce some randomness (based on which of two identical machines happened to be first) into the process which I really don't want seeing that makes debugging much more difficult. >IMO port > number can be anything useful for specific ntb driver and devices, or > maybe just be informative, but peer index should be useful for ntb > client drivers. Yes, the port number is commonly used to decide which resource (MW, DB, etc) will be used to point to another peer. This is how Serge has coded the existing clients and makes sense. The problem comes with crosslink which can not provide these port numbers but worked fine in the previous two-host code. These patches fix the clients so that they support crosslink (and other two host systems) in a very similar way to how they were supported before. In theory, we could now rip out the ugly code for the Intel and AMD drivers that assign port numbers based on primary/secondary and such. But I have no interest in doing so. Logan
Re: [PATCH v2 4/8] NTB: ntb_pingpong: Choose doorbells based on port number
On Fri, Jul 20, 2018 at 2:00 PM Logan Gunthorpe wrote:
>
> This commit fixes pingpong support for existing drivers that do not
> implement ntb_default_port_number() and ntb_default_peer_port_number().
> This is required for hardware (like the crosslink topology of
> switchtec) which cannot assign reasonable port numbers to each port due
> to its perfect symmetry.
>
> Instead of picking the doorbell to use based on the the index of the
> peer, we use the peer's port number. This is a bit clearer and easier
> to understand.
Does this solve the issue where two of the the port numbers are the
same, because of symmetry over a crosslink? I think the two ports
with the "same" number should be identified as different peer index,
even if the port numbers are the same.
Maybe the port of any peer connected over the crosslink should be the
local switch's crosslink port. The local port number might be needed
to configure translation tables in the local switch. If a globally
unique port number is needed, maybe encode a chip number in some high
bits of the port number? If a locally unique port number is needed,
maybe encode a path, that could be useful for configuring address
translations across multiple crosslinks. Encoding a path, then each
port will have a different number, depending on the perspective of the
source port, which could be confusing (already, peer index is local
perspective, so can cause the same kind of confusion). IMO port
number can be anything useful for specific ntb driver and devices, or
maybe just be informative, but peer index should be useful for ntb
client drivers.
> Fixes: c7aeb0afdcc2 ("NTB: ntb_pp: Add full multi-port NTB API support")
> Signed-off-by: Logan Gunthorpe
> ---
> drivers/ntb/test/ntb_pingpong.c | 14 ++
> 1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/ntb/test/ntb_pingpong.c b/drivers/ntb/test/ntb_pingpong.c
> index 65865e460ab8..18d00eec7b02 100644
> --- a/drivers/ntb/test/ntb_pingpong.c
> +++ b/drivers/ntb/test/ntb_pingpong.c
> @@ -121,15 +121,14 @@ static int pp_find_next_peer(struct pp_ctx *pp)
> link = ntb_link_is_up(pp->ntb, NULL, NULL);
>
> /* Find next available peer */
> - if (link & pp->nmask) {
> + if (link & pp->nmask)
> pidx = __ffs64(link & pp->nmask);
> - out_db = BIT_ULL(pidx + 1);
Without +1 here, does this ring the same bell again?
> - } else if (link & pp->pmask) {
> + else if (link & pp->pmask)
> pidx = __ffs64(link & pp->pmask);
> - out_db = BIT_ULL(pidx);
> - } else {
> + else
> return -ENODEV;
> - }
> +
> + out_db = BIT_ULL(ntb_peer_port_number(pp->ntb, pidx));
Can it not be made to work with peer index?
>
> spin_lock(&pp->lock);
> pp->out_pidx = pidx;
> @@ -303,7 +302,7 @@ static void pp_init_flds(struct pp_ctx *pp)
> break;
> }
>
> - pp->in_db = BIT_ULL(pidx);
> + pp->in_db = BIT_ULL(lport);
> pp->pmask = GENMASK_ULL(pidx, 0) >> 1;
> pp->nmask = GENMASK_ULL(pcnt - 1, pidx);
>
> @@ -435,4 +434,3 @@ static void __exit pp_exit(void)
> debugfs_remove_recursive(pp_dbgfs_topdir);
> }
> module_exit(pp_exit);
> -
> --
> 2.11.0
[PATCH v2 4/8] NTB: ntb_pingpong: Choose doorbells based on port number
This commit fixes pingpong support for existing drivers that do not
implement ntb_default_port_number() and ntb_default_peer_port_number().
This is required for hardware (like the crosslink topology of
switchtec) which cannot assign reasonable port numbers to each port due
to its perfect symmetry.
Instead of picking the doorbell to use based on the the index of the
peer, we use the peer's port number. This is a bit clearer and easier
to understand.
Fixes: c7aeb0afdcc2 ("NTB: ntb_pp: Add full multi-port NTB API support")
Signed-off-by: Logan Gunthorpe
---
drivers/ntb/test/ntb_pingpong.c | 14 ++
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/drivers/ntb/test/ntb_pingpong.c b/drivers/ntb/test/ntb_pingpong.c
index 65865e460ab8..18d00eec7b02 100644
--- a/drivers/ntb/test/ntb_pingpong.c
+++ b/drivers/ntb/test/ntb_pingpong.c
@@ -121,15 +121,14 @@ static int pp_find_next_peer(struct pp_ctx *pp)
link = ntb_link_is_up(pp->ntb, NULL, NULL);
/* Find next available peer */
- if (link & pp->nmask) {
+ if (link & pp->nmask)
pidx = __ffs64(link & pp->nmask);
- out_db = BIT_ULL(pidx + 1);
- } else if (link & pp->pmask) {
+ else if (link & pp->pmask)
pidx = __ffs64(link & pp->pmask);
- out_db = BIT_ULL(pidx);
- } else {
+ else
return -ENODEV;
- }
+
+ out_db = BIT_ULL(ntb_peer_port_number(pp->ntb, pidx));
spin_lock(&pp->lock);
pp->out_pidx = pidx;
@@ -303,7 +302,7 @@ static void pp_init_flds(struct pp_ctx *pp)
break;
}
- pp->in_db = BIT_ULL(pidx);
+ pp->in_db = BIT_ULL(lport);
pp->pmask = GENMASK_ULL(pidx, 0) >> 1;
pp->nmask = GENMASK_ULL(pcnt - 1, pidx);
@@ -435,4 +434,3 @@ static void __exit pp_exit(void)
debugfs_remove_recursive(pp_dbgfs_topdir);
}
module_exit(pp_exit);
-
--
2.11.0
