LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] bus: mhi: pci-generic: configurable network interface MRU
@ 2021-07-14 21:18 richard.laing
  2021-07-15 17:20 ` patchwork-bot+netdevbpf
  2021-07-19 10:11 ` Loic Poulain
  0 siblings, 2 replies; 6+ messages in thread
From: richard.laing @ 2021-07-14 21:18 UTC (permalink / raw)
  To: davem; +Cc: netdev, linux-kernel, loic.poulain, Richard Laing

From: Richard Laing <richard.laing@alliedtelesis.co.nz>

The MRU value used by the MHI MBIM network interface affects
the throughput performance of the interface. Different modem
models use different default MRU sizes based on their bandwidth
capabilities. Large values generally result in higher throughput
for larger packet sizes.

In addition if the MRU used by the MHI device is larger than that
specified in the MHI net device the data is fragmented and needs
to be re-assembled which generates a (single) warning message about
the fragmented packets. Setting the MRU on both ends avoids the
extra processing to re-assemble the packets.

This patch allows the documented MRU for a modem to be automatically
set as the MHI net device MRU avoiding fragmentation and improving
throughput performance.

Signed-off-by: Richard Laing <richard.laing@alliedtelesis.co.nz>
---
 drivers/bus/mhi/pci_generic.c | 6 +++++-
 drivers/net/mhi/net.c         | 1 +
 drivers/net/mhi/proto_mbim.c  | 4 +++-
 include/linux/mhi.h           | 2 ++
 4 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/bus/mhi/pci_generic.c b/drivers/bus/mhi/pci_generic.c
index b3357a8a2fdb..ae7a201dad95 100644
--- a/drivers/bus/mhi/pci_generic.c
+++ b/drivers/bus/mhi/pci_generic.c
@@ -32,6 +32,7 @@
  * @edl: emergency download mode firmware path (if any)
  * @bar_num: PCI base address register to use for MHI MMIO register space
  * @dma_data_width: DMA transfer word size (32 or 64 bits)
+ * @mru_default: default MRU size for MBIM network packets
  */
 struct mhi_pci_dev_info {
 	const struct mhi_controller_config *config;
@@ -40,6 +41,7 @@ struct mhi_pci_dev_info {
 	const char *edl;
 	unsigned int bar_num;
 	unsigned int dma_data_width;
+	unsigned int mru_default;
 };
 
 #define MHI_CHANNEL_CONFIG_UL(ch_num, ch_name, el_count, ev_ring) \
@@ -251,7 +253,8 @@ static const struct mhi_pci_dev_info mhi_qcom_sdx55_info = {
 	.edl = "qcom/sdx55m/edl.mbn",
 	.config = &modem_qcom_v1_mhiv_config,
 	.bar_num = MHI_PCI_DEFAULT_BAR_NUM,
-	.dma_data_width = 32
+	.dma_data_width = 32,
+	.mru_default = 32768
 };
 
 static const struct mhi_pci_dev_info mhi_qcom_sdx24_info = {
@@ -643,6 +646,7 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	mhi_cntrl->wake_get = mhi_pci_wake_get_nop;
 	mhi_cntrl->wake_put = mhi_pci_wake_put_nop;
 	mhi_cntrl->wake_toggle = mhi_pci_wake_toggle_nop;
+	mhi_cntrl->mru = info->mru_default;
 
 	err = mhi_pci_claim(mhi_cntrl, info->bar_num, DMA_BIT_MASK(info->dma_data_width));
 	if (err)
diff --git a/drivers/net/mhi/net.c b/drivers/net/mhi/net.c
index e60e38c1f09d..a5a2aa19bb91 100644
--- a/drivers/net/mhi/net.c
+++ b/drivers/net/mhi/net.c
@@ -329,6 +329,7 @@ static int mhi_net_newlink(void *ctxt, struct net_device *ndev, u32 if_id,
 	mhi_netdev->mdev = mhi_dev;
 	mhi_netdev->skbagg_head = NULL;
 	mhi_netdev->proto = info->proto;
+	mhi_netdev->mru = mhi_dev->mhi_cntrl->mru;
 
 	INIT_DELAYED_WORK(&mhi_netdev->rx_refill, mhi_net_rx_refill_work);
 	u64_stats_init(&mhi_netdev->stats.rx_syncp);
diff --git a/drivers/net/mhi/proto_mbim.c b/drivers/net/mhi/proto_mbim.c
index bf1ad863237d..f1cc7f35bb85 100644
--- a/drivers/net/mhi/proto_mbim.c
+++ b/drivers/net/mhi/proto_mbim.c
@@ -292,7 +292,9 @@ static int mbim_init(struct mhi_net_dev *mhi_netdev)
 
 	ndev->needed_headroom = sizeof(struct mbim_tx_hdr);
 	ndev->mtu = MHI_MBIM_DEFAULT_MTU;
-	mhi_netdev->mru = MHI_MBIM_DEFAULT_MRU;
+
+	if (!mhi_netdev->mru)
+		mhi_netdev->mru = MHI_MBIM_DEFAULT_MRU;
 
 	return 0;
 }
diff --git a/include/linux/mhi.h b/include/linux/mhi.h
index 944aa3aa3035..beb918328eef 100644
--- a/include/linux/mhi.h
+++ b/include/linux/mhi.h
@@ -356,6 +356,7 @@ struct mhi_controller_config {
  * @fbc_download: MHI host needs to do complete image transfer (optional)
  * @wake_set: Device wakeup set flag
  * @irq_flags: irq flags passed to request_irq (optional)
+ * @mru: the default MRU for the MHI device
  *
  * Fields marked as (required) need to be populated by the controller driver
  * before calling mhi_register_controller(). For the fields marked as (optional)
@@ -448,6 +449,7 @@ struct mhi_controller {
 	bool fbc_download;
 	bool wake_set;
 	unsigned long irq_flags;
+	u32 mru;
 };
 
 /**
-- 
2.32.0


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] bus: mhi: pci-generic: configurable network interface MRU
  2021-07-14 21:18 [PATCH] bus: mhi: pci-generic: configurable network interface MRU richard.laing
@ 2021-07-15 17:20 ` patchwork-bot+netdevbpf
  2021-07-19 10:11 ` Loic Poulain
  1 sibling, 0 replies; 6+ messages in thread
From: patchwork-bot+netdevbpf @ 2021-07-15 17:20 UTC (permalink / raw)
  To: None; +Cc: davem, netdev, linux-kernel, loic.poulain

Hello:

This patch was applied to netdev/net-next.git (refs/heads/master):

On Thu, 15 Jul 2021 09:18:05 +1200 you wrote:
> From: Richard Laing <richard.laing@alliedtelesis.co.nz>
> 
> The MRU value used by the MHI MBIM network interface affects
> the throughput performance of the interface. Different modem
> models use different default MRU sizes based on their bandwidth
> capabilities. Large values generally result in higher throughput
> for larger packet sizes.
> 
> [...]

Here is the summary with links:
  - bus: mhi: pci-generic: configurable network interface MRU
    https://git.kernel.org/netdev/net-next/c/5c2c85315948

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] bus: mhi: pci-generic: configurable network interface MRU
  2021-07-14 21:18 [PATCH] bus: mhi: pci-generic: configurable network interface MRU richard.laing
  2021-07-15 17:20 ` patchwork-bot+netdevbpf
@ 2021-07-19 10:11 ` Loic Poulain
  2021-07-19 21:44   ` Richard Laing
  1 sibling, 1 reply; 6+ messages in thread
From: Loic Poulain @ 2021-07-19 10:11 UTC (permalink / raw)
  To: richard.laing; +Cc: David Miller, Network Development, open list

Hi Richard,

On Wed, 14 Jul 2021 at 23:18, <richard.laing@alliedtelesis.co.nz> wrote:
>
> From: Richard Laing <richard.laing@alliedtelesis.co.nz>
>
> The MRU value used by the MHI MBIM network interface affects
> the throughput performance of the interface. Different modem
> models use different default MRU sizes based on their bandwidth
> capabilities. Large values generally result in higher throughput
> for larger packet sizes.


For my interest do you have some numbers here highlighting improvement?

> In addition if the MRU used by the MHI device is larger than that
> specified in the MHI net device the data is fragmented and needs
> to be re-assembled which generates a (single) warning message about
> the fragmented packets. Setting the MRU on both ends avoids the
> extra processing to re-assemble the packets.


Re-assembly is quite free since it's about chaining buffers (no copy
or re-alloc).

>
>
> This patch allows the documented MRU for a modem to be automatically
> set as the MHI net device MRU avoiding fragmentation and improving
> throughput performance.


I would be interested in some numbers (throughput, CPU usage...) since
I've not been able to test that at very high throughput for MBIM. The
default MRU has been set so that MHI packets fit into 4K pages, for
faster allocation. As you said, that causes more MHI transfers but
there is Interrupt Coalescing at hardware level, which mitigates
overhead.

Regards,
Loic

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] bus: mhi: pci-generic: configurable network interface MRU
  2021-07-19 10:11 ` Loic Poulain
@ 2021-07-19 21:44   ` Richard Laing
  2021-07-27  9:21     ` Loic Poulain
  0 siblings, 1 reply; 6+ messages in thread
From: Richard Laing @ 2021-07-19 21:44 UTC (permalink / raw)
  To: Loic Poulain; +Cc: David Miller, Network Development, open list

Hi Loic,

On 7/19/21 10:11 PM, Loic Poulain wrote:
> For my interest do you have some numbers here highlighting improvement?
These are some of the numbers we found from initial testing using an 
external packet generator:

packet size    packets sent  throughput (%pps)
64             1000000        6.21%
128            1000000        7.42%
256            1000000        10.79%
512            1000000        16.40%
1024           1000000        34.34%
1262           1000000        43.82%
1263           1000000        22.45%    <--
1280           1000000        23.15%
1500           1000000        46.32%
1518           1000000        46.84%

You can see the sudden drop of almost 50% between 1262 and 1263 byte 
packets. This is what caused us to investigate further. Following the 
change to 32KB buffers the drop in throughput is no longer seen.

packet size    packets sent  throughput (%pps)
64             1000000       4.41%
128            1000000       7.70%
256            1000000       14.26%
512            1000000       27.06%
1024           1000000       49.39%
1280           1000000       58.82%
1428           1000000       62.63%

In all cases we were testing with the modem itself in internal loopback 
mode.

We have noted that our modem defaults to 32KB buffers (and a maximum of 
32 packets per buffer) and also that these values can be changed. We are 
considering adding the ability to tune the buffer size, perhaps adding a 
sysfs entry or netlink message to change the buffer size instead of the 
hard coded value. Any comments would be appreciated.

Regards,
Richard




^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] bus: mhi: pci-generic: configurable network interface MRU
  2021-07-19 21:44   ` Richard Laing
@ 2021-07-27  9:21     ` Loic Poulain
  2021-07-27 20:49       ` Richard Laing
  0 siblings, 1 reply; 6+ messages in thread
From: Loic Poulain @ 2021-07-27  9:21 UTC (permalink / raw)
  To: Richard Laing; +Cc: David Miller, Network Development, open list

On Mon, 19 Jul 2021 at 23:44, Richard Laing
<Richard.Laing@alliedtelesis.co.nz> wrote:
>
> Hi Loic,
>
> On 7/19/21 10:11 PM, Loic Poulain wrote:
> > For my interest do you have some numbers here highlighting improvement?
> These are some of the numbers we found from initial testing using an
> external packet generator:
>
> packet size    packets sent  throughput (%pps)
> 64             1000000        6.21%
> 128            1000000        7.42%
> 256            1000000        10.79%
> 512            1000000        16.40%
> 1024           1000000        34.34%
> 1262           1000000        43.82%
> 1263           1000000        22.45%    <--
> 1280           1000000        23.15%
> 1500           1000000        46.32%
> 1518           1000000        46.84%
>
> You can see the sudden drop of almost 50% between 1262 and 1263 byte
> packets. This is what caused us to investigate further. Following the
> change to 32KB buffers the drop in throughput is no longer seen.
>
> packet size    packets sent  throughput (%pps)
> 64             1000000       4.41%
> 128            1000000       7.70%
> 256            1000000       14.26%
> 512            1000000       27.06%
> 1024           1000000       49.39%
> 1280           1000000       58.82%
> 1428           1000000       62.63%
>
> In all cases we were testing with the modem itself in internal loopback
> mode.
>
> We have noted that our modem defaults to 32KB buffers (and a maximum of
> 32 packets per buffer) and also that these values can be changed. We are
> considering adding the ability to tune the buffer size, perhaps adding a
> sysfs entry or netlink message to change the buffer size instead of the
> hard coded value. Any comments would be appreciated.

Thanks for the info, that's interesting.

Note that the default MRU you define is not MHI controller specific
but MHI channel specific (IP/MBIM channel), so it should not be a
property of the MHI controller. AFAIK, The MHI specification already
defines MRU for the transfered buffers which is 65535. I would
recommend to move this prop to the channel config.

Regards,
Loic

>
> Regards,
> Richard
>
>
>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] bus: mhi: pci-generic: configurable network interface MRU
  2021-07-27  9:21     ` Loic Poulain
@ 2021-07-27 20:49       ` Richard Laing
  0 siblings, 0 replies; 6+ messages in thread
From: Richard Laing @ 2021-07-27 20:49 UTC (permalink / raw)
  To: Loic Poulain; +Cc: David Miller, Network Development, open list



On 7/27/21 9:21 PM, Loic Poulain wrote:
> Note that the default MRU you define is not MHI controller specific
> but MHI channel specific (IP/MBIM channel), so it should not be a
> property of the MHI controller. AFAIK, The MHI specification already
> defines MRU for the transfered buffers which is 65535. I would
> recommend to move this prop to the channel config.

That makes sense thank you. I assume the UL and DL channels could be 
expected to have the same MRU?

Regards,
Richard

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-07-27 20:49 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-14 21:18 [PATCH] bus: mhi: pci-generic: configurable network interface MRU richard.laing
2021-07-15 17:20 ` patchwork-bot+netdevbpf
2021-07-19 10:11 ` Loic Poulain
2021-07-19 21:44   ` Richard Laing
2021-07-27  9:21     ` Loic Poulain
2021-07-27 20:49       ` Richard Laing

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).