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

[Serge Semin]

On Fri, Jun 08, 2018 at 06:08:14PM -0600, Logan Gunthorpe <logang@xxxxxxxxxxxx> 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.
> 

Thanks for the patch. It was the original version of the ping-pong driver,
I was going to submit. But I've decided to develop it a bit different. And
here is why.

My goal was to create the multi-port version of the ping-pong test.
The idea of the new driver was to implement the cyclic port-to-port
ping-pong algorithm. Simply speaking each port selects two partner-ports,
one partner would be used as the source of pings and another one would be
target of pongs sent to with the defined delay.

Since IDT got a global Doorbell register, which is shared between all
the ports, I had to assign an unique doorbell bit to each port. I created a
simple algorithm, which linearised in general non-linear port numbers.
Then I used the globally unique port index to select the corresponding
doorbell bit. pp_init_flds() methods implements the corresponding algorithm,
while pp_find_next_peer() performs the next port selection to convey the
pong to.

Regarding the patch. The idea of using the port number instead of linearised
unique index should also work for Intel/AMD/IDT drivers. But the ports-space
linearization algorithm was created for the case if the real port numbers
would exceed the available Doorbell bits. I thought this might be the case of
multi-ports version of the switchtec driver.

Needless to say, that if Switchtec driver had the ports-index API implementation,
this patch wouldn't be needed.

Regards,
-Sergey

> Fixes: c7aeb0afdcc2 ("NTB: ntb_pp: Add full multi-port NTB API support")
> Signed-off-by: Logan Gunthorpe <logang@xxxxxxxxxxxx>
> ---
>  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
> 
> -- 
> You received this message because you are subscribed to the Google Groups "linux-ntb" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-ntb+unsubscribe@xxxxxxxxxxxxxxxxx
> To post to this group, send email to linux-ntb@xxxxxxxxxxxxxxxxx
> To view this discussion on the web visit https://groups.google.com/d/msgid/linux-ntb/20180609000819.13883-5-logang%40deltatee.com.
> For more options, visit https://groups.google.com/d/optout.