[PATCH] ntb_netdev: fix sleep time mismatch

[PATCH] ntb_netdev: fix sleep time mismatch

[Jon Mason]

The tx_time should be in usecs (according to the comment above the
variable), but the setting of the timer during the rearming is done in
msecs.  Change it to match the expected units.

Fixes: e74bfeedad08 ("NTB: Add flow control to the ntb_netdev")
Suggested-by: Gerd W. Haeussler <gerd.haeussler@cesys-it.com>
Signed-off-by: Jon Mason <jdmason@kudzu.us>
---
 drivers/net/ntb_netdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ntb_netdev.c b/drivers/net/ntb_netdev.c
index 9f6f7ccd44f7..306a662eba94 100644
--- a/drivers/net/ntb_netdev.c
+++ b/drivers/net/ntb_netdev.c
@@ -236,7 +236,7 @@ static void ntb_netdev_tx_timer(struct timer_list *t)
 	struct net_device *ndev = dev->ndev;
 
 	if (ntb_transport_tx_free_entry(dev->qp) < tx_stop) {
-		mod_timer(&dev->tx_timer, jiffies + msecs_to_jiffies(tx_time));
+		mod_timer(&dev->tx_timer, jiffies + usecs_to_jiffies(tx_time));
 	} else {
 		/* Make sure anybody stopping the queue after this sees the new
 		 * value of ntb_transport_tx_free_entry()
-- 
2.14.4

Re: [PATCH] ntb_netdev: fix sleep time mismatch

[Dave Jiang]

On 06/11/2018 01:39 PM, Jon Mason wrote:
> The tx_time should be in usecs (according to the comment above the
> variable), but the setting of the timer during the rearming is done in
> msecs.  Change it to match the expected units.
> 
> Fixes: e74bfeedad08 ("NTB: Add flow control to the ntb_netdev")
> Suggested-by: Gerd W. Haeussler <gerd.haeussler@cesys-it.com>
> Signed-off-by: Jon Mason <jdmason@kudzu.us>

Acked-by: Dave Jiang <dave.jiang@intel.com>

> ---
>  drivers/net/ntb_netdev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ntb_netdev.c b/drivers/net/ntb_netdev.c
> index 9f6f7ccd44f7..306a662eba94 100644
> --- a/drivers/net/ntb_netdev.c
> +++ b/drivers/net/ntb_netdev.c
> @@ -236,7 +236,7 @@ static void ntb_netdev_tx_timer(struct timer_list *t)
>  	struct net_device *ndev = dev->ndev;
>  
>  	if (ntb_transport_tx_free_entry(dev->qp) < tx_stop) {
> -		mod_timer(&dev->tx_timer, jiffies + msecs_to_jiffies(tx_time));
> +		mod_timer(&dev->tx_timer, jiffies + usecs_to_jiffies(tx_time));
>  	} else {
>  		/* Make sure anybody stopping the queue after this sees the new
>  		 * value of ntb_transport_tx_free_entry()
> 

Re: [PATCH] ntb_netdev: fix sleep time mismatch

[Logan Gunthorpe]

On 11/06/18 02:39 PM, Jon Mason wrote:
> The tx_time should be in usecs (according to the comment above the
> variable), but the setting of the timer during the rearming is done in
> msecs.  Change it to match the expected units.

I'm curious what the user visible effects of this bug were. Will this
significantly improve the performance of ntb_netdev?

Logan

Re: [PATCH] ntb_netdev: fix sleep time mismatch

[Jon Mason]

On Mon, Jun 11, 2018 at 6:33 PM, Logan Gunthorpe <logang@deltatee.com> wrote:
>
>
> On 11/06/18 02:39 PM, Jon Mason wrote:
>> The tx_time should be in usecs (according to the comment above the
>> variable), but the setting of the timer during the rearming is done in
>> msecs.  Change it to match the expected units.
>
> I'm curious what the user visible effects of this bug were. Will this
> significantly improve the performance of ntb_netdev?

Unknown.  I'm unable to benchmark it, but it could make the
performance more jittery.  Unless you have an objection, I'll apply it
to the ntb-next branch, and we can benchmark it (and if it is worse
then we can drop it or do a follow-on to find more optimal numbers for
the timeout).

Thanks,
Jon

>
> Logan

Re: [PATCH] ntb_netdev: fix sleep time mismatch

[Logan Gunthorpe]

On 11/06/18 09:15 PM, Jon Mason wrote:
> On Mon, Jun 11, 2018 at 6:33 PM, Logan Gunthorpe <logang@deltatee.com> wrote:
>>
>>
>> On 11/06/18 02:39 PM, Jon Mason wrote:
>>> The tx_time should be in usecs (according to the comment above the
>>> variable), but the setting of the timer during the rearming is done in
>>> msecs.  Change it to match the expected units.
>>
>> I'm curious what the user visible effects of this bug were. Will this
>> significantly improve the performance of ntb_netdev?
> 
> Unknown.  I'm unable to benchmark it, but it could make the
> performance more jittery.  Unless you have an objection, I'll apply it
> to the ntb-next branch, and we can benchmark it (and if it is worse
> then we can drop it or do a follow-on to find more optimal numbers for
> the timeout).

I tried it and didn't see any noticeable change but it's clear the
author intended what you say so

Reviewed-By: Logan Gunthorpe <logang@deltatee.com>

Logan

Re: [PATCH] ntb_netdev: fix sleep time mismatch

[Eric Pilmore]

On Tue, Jun 12, 2018 at 9:11 AM, Logan Gunthorpe <logang@deltatee.com> wrote:
>
>
> On 11/06/18 09:15 PM, Jon Mason wrote:
>> On Mon, Jun 11, 2018 at 6:33 PM, Logan Gunthorpe <logang@deltatee.com> wrote:
>>>
>>>
>>> On 11/06/18 02:39 PM, Jon Mason wrote:
>>>> The tx_time should be in usecs (according to the comment above the
>>>> variable), but the setting of the timer during the rearming is done in
>>>> msecs.  Change it to match the expected units.
>>>
>>> I'm curious what the user visible effects of this bug were. Will this
>>> significantly improve the performance of ntb_netdev?
>>
>> Unknown.  I'm unable to benchmark it, but it could make the
>> performance more jittery.  Unless you have an objection, I'll apply it
>> to the ntb-next branch, and we can benchmark it (and if it is worse
>> then we can drop it or do a follow-on to find more optimal numbers for
>> the timeout).
>
> I tried it and didn't see any noticeable change but it's clear the
> author intended what you say so


How many entries are in your TX queue (tx_max_entry)?
I assume the tx_stop in ntb_netdev was still set to its default, 5.

If the TX queue is large then it may be difficult to go below the
tx_stop threshold.

Eric



>
> Reviewed-By: Logan Gunthorpe <logang@deltatee.com>
>
> Logan
>
> --
> 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@googlegroups.com.
> To post to this group, send email to linux-ntb@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/linux-ntb/d51b4fbc-3d56-2802-11f3-401876373b4c%40deltatee.com.
> For more options, visit https://groups.google.com/d/optout.

Re: [PATCH] ntb_netdev: fix sleep time mismatch

[Logan Gunthorpe]

On 6/12/2018 3:33 PM, Eric Pilmore wrote:
> How many entries are in your TX queue (tx_max_entry)?
> I assume the tx_stop in ntb_netdev was still set to its default, 5.
> 
> If the TX queue is large then it may be difficult to go below the
> tx_stop threshold.

That makes sense. I didn't really dig into it too much. All I know is
the performance of ntb_netdev has always been slower than I wanted it to
be (almost an order of magnitude slower than ntb_perf) and I was hoping
this would help that.

Logan

Re: [PATCH] ntb_netdev: fix sleep time mismatch

[Eric Pilmore]

On Tue, Jun 12, 2018 at 2:44 PM, Logan Gunthorpe <logang@deltatee.com> wrote:
> On 6/12/2018 3:33 PM, Eric Pilmore wrote:
>> How many entries are in your TX queue (tx_max_entry)?
>> I assume the tx_stop in ntb_netdev was still set to its default, 5.
>>
>> If the TX queue is large then it may be difficult to go below the
>> tx_stop threshold.
>
> That makes sense. I didn't really dig into it too much. All I know is
> the performance of ntb_netdev has always been slower than I wanted it to
> be (almost an order of magnitude slower than ntb_perf) and I was hoping
> this would help that.


I agree. There are too many interrupts and too much bounce buffering.
We have played around with eliminating the interrupts all together, but haven't
seen a noticeable improvement, although our queue size is artificially small
for other reasons.  We're hoping to devote more resources to benchmarking
and improving it. Eliminating or reducing the bounce buffering would help
quite a bit also. Ideally the sender could copy straight into a skb on the
receiving side instead of the intermediate buffering into the shared memory.
Again, we're hoping to get time and resources to play around with some
ideas.

Eric



>
> Logan
>
> --
> 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@googlegroups.com.
> To post to this group, send email to linux-ntb@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/linux-ntb/9261d4bc-f5a5-8b54-2227-57e5a3cf9d5c%40deltatee.com.
> For more options, visit https://groups.google.com/d/optout.

Re: [PATCH] ntb_netdev: fix sleep time mismatch

[Logan Gunthorpe]

On 12/06/18 03:59 PM, Eric Pilmore wrote:
> I agree. There are too many interrupts and too much bounce buffering.
> We have played around with eliminating the interrupts all together, but haven't
> seen a noticeable improvement, although our queue size is artificially small
> for other reasons.  We're hoping to devote more resources to benchmarking
> and improving it. Eliminating or reducing the bounce buffering would help
> quite a bit also. Ideally the sender could copy straight into a skb on the
> receiving side instead of the intermediate buffering into the shared memory.
> Again, we're hoping to get time and resources to play around with some
> ideas.

Awesome! I'm really glad someone is looking into this! I'd really
appreciate it if you keep the list informed if you make progress.

The only idea I had was to start by writing an ntb_transport_perf to see
if the slow down is in ntb_transport (due to bounce buffering, etc) or
if it's all ntb_netdev/tcp layer.

Logan

Re: [PATCH] ntb_netdev: fix sleep time mismatch

[Dave Jiang]

On 06/12/2018 03:04 PM, Logan Gunthorpe wrote:
> 
> 
> On 12/06/18 03:59 PM, Eric Pilmore wrote:
>> I agree. There are too many interrupts and too much bounce buffering.
>> We have played around with eliminating the interrupts all together, but haven't
>> seen a noticeable improvement, although our queue size is artificially small
>> for other reasons.  We're hoping to devote more resources to benchmarking
>> and improving it. Eliminating or reducing the bounce buffering would help
>> quite a bit also. Ideally the sender could copy straight into a skb on the
>> receiving side instead of the intermediate buffering into the shared memory.
>> Again, we're hoping to get time and resources to play around with some
>> ideas.
> 
> Awesome! I'm really glad someone is looking into this! I'd really
> appreciate it if you keep the list informed if you make progress.
> 
> The only idea I had was to start by writing an ntb_transport_perf to see
> if the slow down is in ntb_transport (due to bounce buffering, etc) or
> if it's all ntb_netdev/tcp layer.

The bounce buffering definitely decreases the performance. So the idea
of directly writing to the remote skbuff would be great. It would also
resolve issue of failing to allocate a large enough DMA region for the
bounce buffer. Allen and I talked about netpoll support but never got
around to look into it. So that may be another idea for performance
enhancement. I do wonder if we will have any resistance from upstream
due to security concerns since in order to do that we'll have to open up
a very large BAR and allow access to entire memory region of the
opposing node. Not an IOMMU expert, but will we have any issues when it
comes to DMA to the remote buffers in this way? Probably not since Allen
has NTRDMA doing something similar. I guess we can see what the change
looks like. Looking forward to that though.

Re: [PATCH] ntb_netdev: fix sleep time mismatch

[Logan Gunthorpe]

On 12/06/18 04:17 PM, Dave Jiang wrote:
> The bounce buffering definitely decreases the performance. So the idea
> of directly writing to the remote skbuff would be great. It would also
> resolve issue of failing to allocate a large enough DMA region for the
> bounce buffer. Allen and I talked about netpoll support but never got
> around to look into it. So that may be another idea for performance
> enhancement. I do wonder if we will have any resistance from upstream
> due to security concerns since in order to do that we'll have to open up
> a very large BAR and allow access to entire memory region of the
> opposing node. Not an IOMMU expert, but will we have any issues when it
> comes to DMA to the remote buffers in this way? Probably not since Allen
> has NTRDMA doing something similar. I guess we can see what the change
> looks like. Looking forward to that though.

Yeah, it's a pain and the large BAR idea is a huge security risk for
systems that are not using an IOMMU. The RDMA guys inadvertently did the
same thing once (allow remote hosts to DMA to any address) and
developers went berserk when they found out.

I really wish the hardware guys would just give us an NTB aware high
performance DMA engine in a switch. That would simplify things
considerably. If it was well designed, it could make it really easy to
implement RDMA between partitions and then NTB transport would be
trivial to implement -- not only in a performant way but also for
multi-port systems.

Logan

Re: [PATCH] ntb_netdev: fix sleep time mismatch

[Eric Pilmore]

On Tue, Jun 12, 2018 at 3:27 PM, Logan Gunthorpe <logang@deltatee.com> wrote:
>
>
> On 12/06/18 04:17 PM, Dave Jiang wrote:
>> The bounce buffering definitely decreases the performance. So the idea
>> of directly writing to the remote skbuff would be great. It would also
>> resolve issue of failing to allocate a large enough DMA region for the
>> bounce buffer. Allen and I talked about netpoll support but never got
>> around to look into it. So that may be another idea for performance


Well, our original motivation for eliminating the interrupts was that our
particular switch's firmware was having issues with getting inundated with
doorbell requests. Eliminating the usage of interrupts (doorbells) was not
too painful in ntb_transport, although our approach was kind of a work
around hack to get around the switch firmware issues. I set up a RX side
tasklet to effectively poll for work to do. As you might imagine this has the
drawbacks of a slight latency in getting a stream started and a waste of cpu
cycles when there is no activity in the pipe. You really want a hybrid
approach, but for our temporary work around this was sufficient for us at the
time so we could move on to other things. We want to revisit this and
maybe take a NAPI approach (if you're familiar with that hybrid approach
of polling and minimizing interrupts).


>> enhancement. I do wonder if we will have any resistance from upstream
>> due to security concerns since in order to do that we'll have to open up
>> a very large BAR and allow access to entire memory region of the
>> opposing node. Not an IOMMU expert, but will we have any issues when it
>> comes to DMA to the remote buffers in this way? Probably not since Allen
>> has NTRDMA doing something similar. I guess we can see what the change
>> looks like. Looking forward to that though.
>
> Yeah, it's a pain and the large BAR idea is a huge security risk for
> systems that are not using an IOMMU. The RDMA guys inadvertently did the
> same thing once (allow remote hosts to DMA to any address) and
> developers went berserk when they found out.
>
> I really wish the hardware guys would just give us an NTB aware high
> performance DMA engine in a switch. That would simplify things
> considerably. If it was well designed, it could make it really easy to
> implement RDMA between partitions and then NTB transport would be
> trivial to implement -- not only in a performant way but also for
> multi-port systems.


I'm not quite seeing how a DMA in the switch would eliminate the security
risk of a wide open memory. Are you suggesting that the driver for the DMA
would have more control over filtering what source/dest addresses are
allowed and you wouldn't advertise the entire address space via a BAR?

Eric


>
> Logan
>
> --
> 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@googlegroups.com.
> To post to this group, send email to linux-ntb@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/linux-ntb/9f354d1f-af37-b378-b4f8-a8286adfe5ee%40deltatee.com.
> For more options, visit https://groups.google.com/d/optout.

Re: [PATCH] ntb_netdev: fix sleep time mismatch

[Logan Gunthorpe]

On 12/06/18 04:55 PM, Eric Pilmore wrote:
> I'm not quite seeing how a DMA in the switch would eliminate the security
> risk of a wide open memory. Are you suggesting that the driver for the DMA
> would have more control over filtering what source/dest addresses are
> allowed and you wouldn't advertise the entire address space via a BAR?

Well imagine a DMA engine designed similar to how RDMA is designed in RNICs:

Side A programs the DMA engine with an available local buffer (DMA
address and length) as well as a random key. The key is sent to side B
via some side channel (message/scratchpad/etc).

Side B then programs the DMA engine with another local buffer, plus the
direction and the key. The DMA engine uses the key to lookup the remote
address and partition ID or fails if the key is invalid. Once this is
done the DMA engine copies the data between Side B's buffer and Side A's
buffer; then triggers an interrupt back to software. Assuming the
hardware supports enough keys this could be used to great effect. Much
simpler than managing a limited set of memory windows (though you
wouldn't want to get rid of MWs as they'd be useful for other things).

Also, in addition to the key, you might want permission flags (eg. maybe
a buffer can be read but not written or vice-versa).

Logan