Re: [PATCH net-next 04/12] ibmvnic: Introduce xmit_more support using batched subCRQ hcalls

2020-11-16 Thread Thomas Falcon



On 11/14/20 5:46 PM, Jakub Kicinski wrote:

On Thu, 12 Nov 2020 13:09:59 -0600 Thomas Falcon wrote:

Include support for the xmit_more feature utilizing the
H_SEND_SUB_CRQ_INDIRECT hypervisor call which allows the sending
of multiple subordinate Command Response Queue descriptors in one
hypervisor call via a DMA-mapped buffer. This update reduces hypervisor
calls and thus hypervisor call overhead per TX descriptor.

Signed-off-by: Thomas Falcon 

The common bug with xmit_more is not flushing the already queued
notifications when there is a drop. Any time you drop a skb you need
to check it's not an skb that was the end of an xmit_more train and
if so flush notifications (or just always flush on error).

Looking at the driver e.g. this starting goto:

 if (ibmvnic_xmit_workarounds(skb, netdev)) {
 tx_dropped++;
 tx_send_failed++;
 ret = NETDEV_TX_OK;
 goto out;
 }

Does not seem to hit any flush on its way out AFAICS.


Hi, I included those updates in a later patch to ease review but see now 
that that was a mistake. I will merge those bits back into this patch 
and resubmit.


Thanks!



Re: [PATCH net-next 04/12] ibmvnic: Introduce xmit_more support using batched subCRQ hcalls

2020-11-14 Thread Jakub Kicinski
On Thu, 12 Nov 2020 13:09:59 -0600 Thomas Falcon wrote:
> Include support for the xmit_more feature utilizing the
> H_SEND_SUB_CRQ_INDIRECT hypervisor call which allows the sending
> of multiple subordinate Command Response Queue descriptors in one
> hypervisor call via a DMA-mapped buffer. This update reduces hypervisor
> calls and thus hypervisor call overhead per TX descriptor.
> 
> Signed-off-by: Thomas Falcon 

The common bug with xmit_more is not flushing the already queued
notifications when there is a drop. Any time you drop a skb you need 
to check it's not an skb that was the end of an xmit_more train and 
if so flush notifications (or just always flush on error).

Looking at the driver e.g. this starting goto:

if (ibmvnic_xmit_workarounds(skb, netdev)) {
tx_dropped++;   
tx_send_failed++;   
ret = NETDEV_TX_OK; 
goto out;   
}  

Does not seem to hit any flush on its way out AFAICS.


[PATCH net-next 04/12] ibmvnic: Introduce xmit_more support using batched subCRQ hcalls

2020-11-12 Thread Thomas Falcon
Include support for the xmit_more feature utilizing the
H_SEND_SUB_CRQ_INDIRECT hypervisor call which allows the sending
of multiple subordinate Command Response Queue descriptors in one
hypervisor call via a DMA-mapped buffer. This update reduces hypervisor
calls and thus hypervisor call overhead per TX descriptor.

Signed-off-by: Thomas Falcon 
---
 drivers/net/ethernet/ibm/ibmvnic.c | 151 +
 1 file changed, 91 insertions(+), 60 deletions(-)

diff --git a/drivers/net/ethernet/ibm/ibmvnic.c 
b/drivers/net/ethernet/ibm/ibmvnic.c
index 524020691ef8..0f6aba760d65 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -1165,6 +1165,7 @@ static int __ibmvnic_open(struct net_device *netdev)
if (prev_state == VNIC_CLOSED)
enable_irq(adapter->tx_scrq[i]->irq);
enable_scrq_irq(adapter, adapter->tx_scrq[i]);
+   netdev_tx_reset_queue(netdev_get_tx_queue(netdev, i));
}
 
rc = set_link_state(adapter, IBMVNIC_LOGICAL_LNK_UP);
@@ -1529,10 +1530,12 @@ static netdev_tx_t ibmvnic_xmit(struct sk_buff *skb, 
struct net_device *netdev)
int queue_num = skb_get_queue_mapping(skb);
u8 *hdrs = (u8 *)>tx_rx_desc_req;
struct device *dev = >vdev->dev;
+   struct ibmvnic_ind_xmit_queue *ind_bufp;
struct ibmvnic_tx_buff *tx_buff = NULL;
struct ibmvnic_sub_crq_queue *tx_scrq;
struct ibmvnic_tx_pool *tx_pool;
unsigned int tx_send_failed = 0;
+   netdev_tx_t ret = NETDEV_TX_OK;
unsigned int tx_map_failed = 0;
unsigned int tx_dropped = 0;
unsigned int tx_packets = 0;
@@ -1547,7 +1550,7 @@ static netdev_tx_t ibmvnic_xmit(struct sk_buff *skb, 
struct net_device *netdev)
int index = 0;
u8 proto = 0;
u64 handle;
-   netdev_tx_t ret = NETDEV_TX_OK;
+   int i;
 
if (test_bit(0, >resetting)) {
if (!netif_subqueue_stopped(netdev, skb))
@@ -1666,55 +1669,37 @@ static netdev_tx_t ibmvnic_xmit(struct sk_buff *skb, 
struct net_device *netdev)
tx_crq.v1.mss = cpu_to_be16(skb_shinfo(skb)->gso_size);
hdrs += 2;
}
-   /* determine if l2/3/4 headers are sent to firmware */
-   if ((*hdrs >> 7) & 1) {
+
+   if ((*hdrs >> 7) & 1)
build_hdr_descs_arr(tx_buff, _entries, *hdrs);
-   tx_crq.v1.n_crq_elem = num_entries;
-   tx_buff->num_entries = num_entries;
-   tx_buff->indir_arr[0] = tx_crq;
-   tx_buff->indir_dma = dma_map_single(dev, tx_buff->indir_arr,
-   sizeof(tx_buff->indir_arr),
-   DMA_TO_DEVICE);
-   if (dma_mapping_error(dev, tx_buff->indir_dma)) {
-   dev_kfree_skb_any(skb);
-   tx_buff->skb = NULL;
-   if (!firmware_has_feature(FW_FEATURE_CMO))
-   dev_err(dev, "tx: unable to map descriptor 
array\n");
-   tx_map_failed++;
-   tx_dropped++;
-   ret = NETDEV_TX_OK;
-   goto tx_err_out;
-   }
-   lpar_rc = send_subcrq_indirect(adapter, handle,
-  (u64)tx_buff->indir_dma,
-  (u64)num_entries);
-   dma_unmap_single(dev, tx_buff->indir_dma,
-sizeof(tx_buff->indir_arr), DMA_TO_DEVICE);
-   } else {
-   tx_buff->num_entries = num_entries;
-   lpar_rc = send_subcrq(adapter, handle,
- _crq);
-   }
-   if (lpar_rc != H_SUCCESS) {
-   if (lpar_rc != H_CLOSED && lpar_rc != H_PARAMETER)
-   dev_err_ratelimited(dev, "tx: send failed\n");
-   dev_kfree_skb_any(skb);
-   tx_buff->skb = NULL;
 
-   if (lpar_rc == H_CLOSED || adapter->failover_pending) {
-   /* Disable TX and report carrier off if queue is closed
-* or pending failover.
-* Firmware guarantees that a signal will be sent to the
-* driver, triggering a reset or some other action.
-*/
-   netif_tx_stop_all_queues(netdev);
-   netif_carrier_off(netdev);
-   }
+   netdev_tx_sent_queue(txq, skb->len);
 
-   tx_send_failed++;
-   tx_dropped++;
-   ret = NETDEV_TX_OK;
-   goto tx_err_out;
+   tx_crq.v1.n_crq_elem = num_entries;
+   tx_buff->num_entries = num_entries;
+   ind_bufp = _scrq->ind_buf;
+   /* flush buffer if current entry can not fit */
+   if (num_entries + ind_bufp->index >