| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20250718190832.GL2459@horms.kernel.org>
Date: Fri, 18 Jul 2025 20:08:32 +0100
From: Simon Horman <horms@...nel.org>
To: Mingming Cao <mmc@...ux.ibm.com>
Cc: netdev@...r.kernel.org, bjking1@...ux.ibm.com, haren@...ux.ibm.com,
ricklind@...ux.ibm.com, davemarq@...ux.ibm.com, maddy@...ux.ibm.com,
mpe@...erman.id.au, npiggin@...il.com, christophe.leroy@...roup.eu,
andrew+netdev@...n.ch, davem@...emloft.net, kuba@...nel.org,
edumazet@...gle.com, pabeni@...hat.com,
linuxppc-dev@...ts.ozlabs.org, Brian King <bjking@...ibm.com>
Subject: Re: [PATCH net-next] ibmveth: Add multi buffers rx replenishment
hcall support
On Thu, Jul 17, 2025 at 04:10:49PM -0400, Mingming Cao wrote:
> This patch enables batched RX buffer replenishment in ibmveth by
> using the new firmware-supported h_add_logical_lan_buffers() hcall
> to submit up to 8 RX buffers in a single call, instead of repeatedly
> calling the single-buffer h_add_logical_lan_buffer() hcall.
>
> During the probe, with the patch, the driver queries ILLAN attributes
> to detect IBMVETH_ILLAN_RX_MULTI_BUFF_SUPPORT bit. If the attribute is
> present, rx_buffers_per_hcall is set to 8, enabling batched replenishment.
> Otherwise, it defaults to 1, preserving the original upstream behavior
> with no change in code flow for unsupported systems.
>
> The core rx replenish logic remains the same. But when batching
> is enabled, the driver aggregates up to 8 fully prepared descriptors
> into a single h_add_logical_lan_buffers() hypercall. If any allocation
> or DMA mapping fails while preparing a batch, only the successfully
> prepared buffers are submitted, and the remaining are deferred for
> the next replenish cycle.
>
> If at runtime the firmware stops accepting the batched hcall—e,g,
> after a Live Partition Migration (LPM) to a host that does not
> support h_add_logical_lan_buffers(), the hypercall returns H_FUNCTION.
> In that case, the driver transparently disables batching, resets
> rx_buffers_per_hcall to 1, and falls back to the single-buffer hcall
> in next future replenishments to take care of these and future buffers.
>
> Test were done on systems with firmware that both supports and
> does not support the new h_add_logical_lan_buffers hcall.
>
> On supported firmware, this reduces hypercall overhead significantly
> over multiple buffers. SAR measurements showed about a 15% improvement
> in packet processing rate under moderate RX load, with heavier traffic
> seeing gains more than 30%
>
> Signed-off-by: Mingming Cao <mmc@...ux.ibm.com>
> Reviewed-by: Brian King <bjking@...ibm.com>
> Reviewed-by: Haren Myneni <haren@...ux.ibm.com>
> Reviewed-by: Dave Marquardt <davemarq@...ux.ibm.com>
Thanks, overall this change looks good to me.
However - perhaps it's not important, but here goes anyway - it
would be nice if the code moved towards two style aspects that
are preferred for networking code.
1. Wrap lines to 80 columns or less where it can be achieved
without reducing readability.
checkpatch.pl --max-line-length=80 can be of assistance here.
2. Arrange local variables in reverse xmas tree order - longest like to
shortest.
Edward Cree's tool can be of assistance here
https://github.com/ecree-solarflare/xmastree/commits/master/
My fingers got a bit itchy so I had a go at adapting your code to match the
above. It is a very simplistic approach to this. But in case it is of any
use the relative diff is as follows.
Compile tested only!!!
diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
index 8e885270d546..f407b963b3d2 100644
--- a/drivers/net/ethernet/ibm/ibmveth.c
+++ b/drivers/net/ethernet/ibm/ibmveth.c
@@ -211,15 +211,19 @@ static inline void ibmveth_flush_buffer(void *addr, unsigned long length)
static void ibmveth_replenish_buffer_pool(struct ibmveth_adapter *adapter,
struct ibmveth_buff_pool *pool)
{
- struct device *dev = &adapter->vdev->dev;
u32 remaining = pool->size - atomic_read(&pool->available);
union ibmveth_buf_desc descs[IBMVETH_MAX_RX_PER_HCALL] = {0};
u64 correlators[IBMVETH_MAX_RX_PER_HCALL] = {0};
- u32 index;
- u32 i, filled, batch;
unsigned long lpar_rc;
- dma_addr_t dma_addr;
u32 buffers_added = 0;
+ u32 i, filled, batch;
+ struct vio_dev *vdev;
+ dma_addr_t dma_addr;
+ struct device *dev;
+ u32 index;
+
+ vdev = adapter->vdev;
+ dev = &vdev->dev;
mb();
@@ -234,7 +238,8 @@ static void ibmveth_replenish_buffer_pool(struct ibmveth_adapter *adapter,
if (WARN_ON(index == IBM_VETH_INVALID_MAP)) {
adapter->replenish_add_buff_failure++;
netdev_info(adapter->netdev,
- "Invalid map index %u, reset\n", index);
+ "Invalid map index %u, reset\n",
+ index);
schedule_work(&adapter->work);
break;
}
@@ -242,16 +247,18 @@ static void ibmveth_replenish_buffer_pool(struct ibmveth_adapter *adapter,
if (!pool->skbuff[index]) {
struct sk_buff *skb = NULL;
- skb = netdev_alloc_skb(adapter->netdev, pool->buff_size);
+ skb = netdev_alloc_skb(adapter->netdev,
+ pool->buff_size);
if (!skb) {
adapter->replenish_no_mem++;
adapter->replenish_add_buff_failure++;
break;
}
- dma_addr = dma_map_single(&adapter->vdev->dev, skb->data,
- pool->buff_size, DMA_FROM_DEVICE);
- if (dma_mapping_error(&adapter->vdev->dev, dma_addr)) {
+ dma_addr = dma_map_single(dev, skb->data,
+ pool->buff_size,
+ DMA_FROM_DEVICE);
+ if (dma_mapping_error(dev, dma_addr)) {
dev_kfree_skb_any(skb);
adapter->replenish_add_buff_failure++;
break;
@@ -265,12 +272,16 @@ static void ibmveth_replenish_buffer_pool(struct ibmveth_adapter *adapter,
}
if (rx_flush) {
- unsigned int len = min(pool->buff_size,
- adapter->netdev->mtu + IBMVETH_BUFF_OH);
- ibmveth_flush_buffer(pool->skbuff[index]->data, len);
+ unsigned int len;
+
+ len = adapter->netdev->mtu + IBMVETH_BUFF_OH;
+ len = min(pool->buff_size, len);
+ ibmveth_flush_buffer(pool->skbuff[index]->data,
+ len);
}
- descs[filled].fields.flags_len = IBMVETH_BUF_VALID | pool->buff_size;
+ descs[filled].fields.flags_len = IBMVETH_BUF_VALID |
+ pool->buff_size;
descs[filled].fields.address = dma_addr;
correlators[filled] = ((u64)pool->index << 32) | index;
@@ -286,15 +297,19 @@ static void ibmveth_replenish_buffer_pool(struct ibmveth_adapter *adapter,
/* single buffer case*/
if (filled == 1)
- lpar_rc = h_add_logical_lan_buffer(adapter->vdev->unit_address,
+ lpar_rc = h_add_logical_lan_buffer(vdev->unit_address,
descs[0].desc);
else
/* Multi-buffer hcall */
- lpar_rc = h_add_logical_lan_buffers(adapter->vdev->unit_address,
- descs[0].desc, descs[1].desc,
- descs[2].desc, descs[3].desc,
- descs[4].desc, descs[5].desc,
- descs[6].desc, descs[7].desc);
+ lpar_rc = h_add_logical_lan_buffers(vdev->unit_address,
+ descs[0].desc,
+ descs[1].desc,
+ descs[2].desc,
+ descs[3].desc,
+ descs[4].desc,
+ descs[5].desc,
+ descs[6].desc,
+ descs[7].desc);
if (lpar_rc != H_SUCCESS) {
dev_warn_ratelimited(dev,
"RX h_add_logical_lan failed: filled=%u, rc=%lu, batch=%u\n",
@@ -327,9 +342,10 @@ static void ibmveth_replenish_buffer_pool(struct ibmveth_adapter *adapter,
if (pool->skbuff[index]) {
if (dma_addr &&
- !dma_mapping_error(&adapter->vdev->dev, dma_addr))
- dma_unmap_single(&adapter->vdev->dev, dma_addr,
- pool->buff_size, DMA_FROM_DEVICE);
+ !dma_mapping_error(dev, dma_addr))
+ dma_unmap_single(dev, dma_addr,
+ pool->buff_size,
+ DMA_FROM_DEVICE);
dev_kfree_skb_any(pool->skbuff[index]);
pool->skbuff[index] = NULL;
diff --git a/drivers/net/ethernet/ibm/ibmveth.h b/drivers/net/ethernet/ibm/ibmveth.h
index f19b7267b2ae..625f92ee6c39 100644
--- a/drivers/net/ethernet/ibm/ibmveth.h
+++ b/drivers/net/ethernet/ibm/ibmveth.h
@@ -48,15 +48,20 @@
plpar_hcall_norets(H_ADD_LOGICAL_LAN_BUFFER, ua, buf)
static inline long h_add_logical_lan_buffers(unsigned long unit_address,
- unsigned long desc1, unsigned long desc2,
- unsigned long desc3, unsigned long desc4,
- unsigned long desc5, unsigned long desc6,
- unsigned long desc7, unsigned long desc8)
+ unsigned long desc1,
+ unsigned long desc2,
+ unsigned long desc3,
+ unsigned long desc4,
+ unsigned long desc5,
+ unsigned long desc6,
+ unsigned long desc7,
+ unsigned long desc8)
{
unsigned long retbuf[PLPAR_HCALL9_BUFSIZE];
- return plpar_hcall9(H_ADD_LOGICAL_LAN_BUFFERS, retbuf, unit_address, desc1, desc2,
- desc3, desc4, desc5, desc6, desc7, desc8);
+ return plpar_hcall9(H_ADD_LOGICAL_LAN_BUFFERS, retbuf, unit_address,
+ desc1, desc2, desc3, desc4, desc5, desc6, desc7,
+ desc8);
}
/* FW allows us to send 6 descriptors but we only use one so mark
Powered by blists - more mailing lists