| 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: <e7878a08-440c-4ca7-a982-1b9a71ea9072@amd.com>
Date: Thu, 1 Aug 2024 16:07:37 -0700
From: "Nelson, Shannon" <shannon.nelson@....com>
To: Nick Child <nnac123@...ux.ibm.com>, netdev@...r.kernel.org
Cc: bjking1@...ux.ibm.com, haren@...ux.ibm.com, ricklind@...ibm.com
Subject: Re: [PATCH net-next 2/2] ibmveth: Recycle buffers during replenish
phase
On 8/1/2024 2:12 PM, Nick Child wrote:
>
> When the length of a packet is under the rx_copybreak threshold, the
> buffer is copied into a new skb and sent up the stack. This allows the
> dma mapped memory to be recycled back to FW.
>
> Previously, the reuse of the DMA space was handled immediately.
> This means that further packet processing has to wait until
> h_add_logical_lan finishes for this packet.
>
> Therefore, when reusing a packet, offload the hcall to the replenish
> function. As a result, much of the shared logic between the recycle and
> replenish functions can be removed.
>
> This change increases TCP_RR packet rate by another 15% (370k to 430k
> txns). We can see the ftrace data supports this:
> PREV: ibmveth_poll = 8078553.0 us / 190999.0 hits = AVG 42.3 us
> NEW: ibmveth_poll = 7632787.0 us / 224060.0 hits = AVG 34.07 us
>
> Signed-off-by: Nick Child <nnac123@...ux.ibm.com>
> ---
> drivers/net/ethernet/ibm/ibmveth.c | 144 ++++++++++++-----------------
> 1 file changed, 60 insertions(+), 84 deletions(-)
>
> diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
> index e6eb594f0751..b619a3ec245b 100644
> --- a/drivers/net/ethernet/ibm/ibmveth.c
> +++ b/drivers/net/ethernet/ibm/ibmveth.c
> @@ -39,7 +39,8 @@
> #include "ibmveth.h"
>
> static irqreturn_t ibmveth_interrupt(int irq, void *dev_instance);
> -static void ibmveth_rxq_harvest_buffer(struct ibmveth_adapter *adapter);
> +static void ibmveth_rxq_harvest_buffer(struct ibmveth_adapter *adapter,
> + bool reuse);
> static unsigned long ibmveth_get_desired_dma(struct vio_dev *vdev);
>
> static struct kobj_type ktype_veth_pool;
> @@ -226,6 +227,16 @@ static void ibmveth_replenish_buffer_pool(struct ibmveth_adapter *adapter,
> for (i = 0; i < count; ++i) {
> union ibmveth_buf_desc desc;
>
> + free_index = pool->consumer_index;
> + index = pool->free_map[free_index];
> + skb = NULL;
> +
> + BUG_ON(index == IBM_VETH_INVALID_MAP);
Maybe can replace with a WARN_ON with a break out of the loop?
Otherwise this looks reasonable.
Reviewed-by: Shannon Nelson <shannon.nelson@....com>
> +
> + /* are we allocating a new buffer or recycling an old one */
> + if (pool->skbuff[index])
> + goto reuse;
> +
> skb = netdev_alloc_skb(adapter->netdev, pool->buff_size);
>
> if (!skb) {
> @@ -235,46 +246,46 @@ static void ibmveth_replenish_buffer_pool(struct ibmveth_adapter *adapter,
> break;
> }
>
> - free_index = pool->consumer_index;
> - pool->consumer_index++;
> - if (pool->consumer_index >= pool->size)
> - pool->consumer_index = 0;
> - index = pool->free_map[free_index];
> -
> - BUG_ON(index == IBM_VETH_INVALID_MAP);
> - BUG_ON(pool->skbuff[index] != NULL);
> -
> 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))
> goto failure;
>
> - pool->free_map[free_index] = IBM_VETH_INVALID_MAP;
> pool->dma_addr[index] = dma_addr;
> pool->skbuff[index] = skb;
>
> - correlator = ((u64)pool->index << 32) | index;
> - *(u64 *)skb->data = correlator;
> -
> - desc.fields.flags_len = IBMVETH_BUF_VALID | pool->buff_size;
> - desc.fields.address = dma_addr;
> -
> if (rx_flush) {
> unsigned int len = min(pool->buff_size,
> - adapter->netdev->mtu +
> - IBMVETH_BUFF_OH);
> + adapter->netdev->mtu +
> + IBMVETH_BUFF_OH);
> ibmveth_flush_buffer(skb->data, len);
> }
> +reuse:
> + dma_addr = pool->dma_addr[index];
> + desc.fields.flags_len = IBMVETH_BUF_VALID | pool->buff_size;
> + desc.fields.address = dma_addr;
> +
> + correlator = ((u64)pool->index << 32) | index;
> + *(u64 *)pool->skbuff[index]->data = correlator;
> +
> lpar_rc = h_add_logical_lan_buffer(adapter->vdev->unit_address,
> desc.desc);
>
> if (lpar_rc != H_SUCCESS) {
> + netdev_warn(adapter->netdev,
> + "%sadd_logical_lan failed %lu\n",
> + skb ? "" : "When recycling: ", lpar_rc);
> goto failure;
> - } else {
> - buffers_added++;
> - adapter->replenish_add_buff_success++;
> }
> +
> + pool->free_map[free_index] = IBM_VETH_INVALID_MAP;
> + pool->consumer_index++;
> + if (pool->consumer_index >= pool->size)
> + pool->consumer_index = 0;
> +
> + buffers_added++;
> + adapter->replenish_add_buff_success++;
> }
>
> mb();
> @@ -282,17 +293,13 @@ static void ibmveth_replenish_buffer_pool(struct ibmveth_adapter *adapter,
> return;
>
> failure:
> - pool->free_map[free_index] = index;
> - pool->skbuff[index] = NULL;
> - if (pool->consumer_index == 0)
> - pool->consumer_index = pool->size - 1;
> - else
> - pool->consumer_index--;
> - if (!dma_mapping_error(&adapter->vdev->dev, dma_addr))
> +
> + if (dma_addr && !dma_mapping_error(&adapter->vdev->dev, dma_addr))
> dma_unmap_single(&adapter->vdev->dev,
> pool->dma_addr[index], pool->buff_size,
> DMA_FROM_DEVICE);
> - dev_kfree_skb_any(skb);
> + dev_kfree_skb_any(pool->skbuff[index]);
> + pool->skbuff[index] = NULL;
> adapter->replenish_add_buff_failure++;
>
> mb();
> @@ -365,7 +372,7 @@ static void ibmveth_free_buffer_pool(struct ibmveth_adapter *adapter,
>
> /* remove a buffer from a pool */
> static void ibmveth_remove_buffer_from_pool(struct ibmveth_adapter *adapter,
> - u64 correlator)
> + u64 correlator, bool reuse)
> {
> unsigned int pool = correlator >> 32;
> unsigned int index = correlator & 0xffffffffUL;
> @@ -376,15 +383,23 @@ static void ibmveth_remove_buffer_from_pool(struct ibmveth_adapter *adapter,
> BUG_ON(index >= adapter->rx_buff_pool[pool].size);
>
> skb = adapter->rx_buff_pool[pool].skbuff[index];
> -
> BUG_ON(skb == NULL);
>
> - adapter->rx_buff_pool[pool].skbuff[index] = NULL;
> + /* if we are going to reuse the buffer then keep the pointers around
> + * but mark index as available. replenish will see the skb pointer and
> + * assume it is to be recycled.
> + */
> + if (!reuse) {
> + /* remove the skb pointer to mark free. actual freeing is done
> + * by upper level networking after gro_recieve
> + */
> + adapter->rx_buff_pool[pool].skbuff[index] = NULL;
>
> - dma_unmap_single(&adapter->vdev->dev,
> - adapter->rx_buff_pool[pool].dma_addr[index],
> - adapter->rx_buff_pool[pool].buff_size,
> - DMA_FROM_DEVICE);
> + dma_unmap_single(&adapter->vdev->dev,
> + adapter->rx_buff_pool[pool].dma_addr[index],
> + adapter->rx_buff_pool[pool].buff_size,
> + DMA_FROM_DEVICE);
> + }
>
> free_index = adapter->rx_buff_pool[pool].producer_index;
> adapter->rx_buff_pool[pool].producer_index++;
> @@ -411,51 +426,13 @@ static inline struct sk_buff *ibmveth_rxq_get_buffer(struct ibmveth_adapter *ada
> return adapter->rx_buff_pool[pool].skbuff[index];
> }
>
> -/* recycle the current buffer on the rx queue */
> -static int ibmveth_rxq_recycle_buffer(struct ibmveth_adapter *adapter)
> +static void ibmveth_rxq_harvest_buffer(struct ibmveth_adapter *adapter,
> + bool reuse)
> {
> - u32 q_index = adapter->rx_queue.index;
> - u64 correlator = adapter->rx_queue.queue_addr[q_index].correlator;
> - unsigned int pool = correlator >> 32;
> - unsigned int index = correlator & 0xffffffffUL;
> - union ibmveth_buf_desc desc;
> - unsigned long lpar_rc;
> - int ret = 1;
> -
> - BUG_ON(pool >= IBMVETH_NUM_BUFF_POOLS);
> - BUG_ON(index >= adapter->rx_buff_pool[pool].size);
> + u64 cor;
>
> - if (!adapter->rx_buff_pool[pool].active) {
> - ibmveth_rxq_harvest_buffer(adapter);
> - ibmveth_free_buffer_pool(adapter, &adapter->rx_buff_pool[pool]);
> - goto out;
> - }
> -
> - desc.fields.flags_len = IBMVETH_BUF_VALID |
> - adapter->rx_buff_pool[pool].buff_size;
> - desc.fields.address = adapter->rx_buff_pool[pool].dma_addr[index];
> -
> - lpar_rc = h_add_logical_lan_buffer(adapter->vdev->unit_address, desc.desc);
> -
> - if (lpar_rc != H_SUCCESS) {
> - netdev_dbg(adapter->netdev, "h_add_logical_lan_buffer failed "
> - "during recycle rc=%ld", lpar_rc);
> - ibmveth_remove_buffer_from_pool(adapter, adapter->rx_queue.queue_addr[adapter->rx_queue.index].correlator);
> - ret = 0;
> - }
> -
> - if (++adapter->rx_queue.index == adapter->rx_queue.num_slots) {
> - adapter->rx_queue.index = 0;
> - adapter->rx_queue.toggle = !adapter->rx_queue.toggle;
> - }
> -
> -out:
> - return ret;
> -}
> -
> -static void ibmveth_rxq_harvest_buffer(struct ibmveth_adapter *adapter)
> -{
> - ibmveth_remove_buffer_from_pool(adapter, adapter->rx_queue.queue_addr[adapter->rx_queue.index].correlator);
> + cor = adapter->rx_queue.queue_addr[adapter->rx_queue.index].correlator;
> + ibmveth_remove_buffer_from_pool(adapter, cor, reuse);
>
> if (++adapter->rx_queue.index == adapter->rx_queue.num_slots) {
> adapter->rx_queue.index = 0;
> @@ -1347,7 +1324,7 @@ static int ibmveth_poll(struct napi_struct *napi, int budget)
> wmb(); /* suggested by larson1 */
> adapter->rx_invalid_buffer++;
> netdev_dbg(netdev, "recycling invalid buffer\n");
> - ibmveth_rxq_recycle_buffer(adapter);
> + ibmveth_rxq_harvest_buffer(adapter, true);
> } else {
> struct sk_buff *skb, *new_skb;
> int length = ibmveth_rxq_frame_length(adapter);
> @@ -1380,11 +1357,10 @@ static int ibmveth_poll(struct napi_struct *napi, int budget)
> if (rx_flush)
> ibmveth_flush_buffer(skb->data,
> length + offset);
> - if (!ibmveth_rxq_recycle_buffer(adapter))
> - kfree_skb(skb);
> + ibmveth_rxq_harvest_buffer(adapter, true);
> skb = new_skb;
> } else {
> - ibmveth_rxq_harvest_buffer(adapter);
> + ibmveth_rxq_harvest_buffer(adapter, false);
> skb_reserve(skb, offset);
> }
>
> --
> 2.43.0
>
>
Powered by blists - more mailing lists