Re: [PATCH v2 4/8] NTB: ntb_pingpong: Choose doorbells based on port number

[Logan Gunthorpe]

On 24/07/18 12:12 PM, Allen Hubbe wrote:
> On Tue, Jul 24, 2018 at 1:37 PM Logan Gunthorpe <logang@xxxxxxxxxxxx> 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 <allenbh@xxxxxxxxx>

Thanks!

Logan