* [PATCH net-next 02/16] qlge: Remove page_chunk.last_flag
2019-06-17 7:48 [PATCH net-next 01/16] qlge: Remove irq_cnt Benjamin Poirier
@ 2019-06-17 7:48 ` Benjamin Poirier
2019-06-26 9:12 ` Manish Chopra
2019-06-17 7:48 ` [PATCH net-next 03/16] qlge: Deduplicate lbq_buf_size Benjamin Poirier
` (16 subsequent siblings)
17 siblings, 1 reply; 46+ messages in thread
From: Benjamin Poirier @ 2019-06-17 7:48 UTC (permalink / raw)
To: Manish Chopra, GR-Linux-NIC-Dev, netdev
As already done in ql_get_curr_lchunk(), this member can be replaced by a
simple test.
Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
---
drivers/net/ethernet/qlogic/qlge/qlge.h | 1 -
drivers/net/ethernet/qlogic/qlge/qlge_main.c | 13 +++++--------
2 files changed, 5 insertions(+), 9 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qlge/qlge.h b/drivers/net/ethernet/qlogic/qlge/qlge.h
index 5d9a36deda08..0a156a95e981 100644
--- a/drivers/net/ethernet/qlogic/qlge/qlge.h
+++ b/drivers/net/ethernet/qlogic/qlge/qlge.h
@@ -1363,7 +1363,6 @@ struct page_chunk {
char *va; /* virt addr for this chunk */
u64 map; /* mapping for master */
unsigned int offset; /* offset for this chunk */
- unsigned int last_flag; /* flag set for last chunk in page */
};
struct bq_desc {
diff --git a/drivers/net/ethernet/qlogic/qlge/qlge_main.c b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
index 0bfbe11db795..038a6bfc79c7 100644
--- a/drivers/net/ethernet/qlogic/qlge/qlge_main.c
+++ b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
@@ -1077,11 +1077,9 @@ static int ql_get_next_chunk(struct ql_adapter *qdev, struct rx_ring *rx_ring,
rx_ring->pg_chunk.offset += rx_ring->lbq_buf_size;
if (rx_ring->pg_chunk.offset == ql_lbq_block_size(qdev)) {
rx_ring->pg_chunk.page = NULL;
- lbq_desc->p.pg_chunk.last_flag = 1;
} else {
rx_ring->pg_chunk.va += rx_ring->lbq_buf_size;
get_page(rx_ring->pg_chunk.page);
- lbq_desc->p.pg_chunk.last_flag = 0;
}
return 0;
}
@@ -2778,6 +2776,8 @@ static int ql_alloc_tx_resources(struct ql_adapter *qdev,
static void ql_free_lbq_buffers(struct ql_adapter *qdev, struct rx_ring *rx_ring)
{
+ unsigned int last_offset = ql_lbq_block_size(qdev) -
+ rx_ring->lbq_buf_size;
struct bq_desc *lbq_desc;
uint32_t curr_idx, clean_idx;
@@ -2787,13 +2787,10 @@ static void ql_free_lbq_buffers(struct ql_adapter *qdev, struct rx_ring *rx_ring
while (curr_idx != clean_idx) {
lbq_desc = &rx_ring->lbq[curr_idx];
- if (lbq_desc->p.pg_chunk.last_flag) {
- pci_unmap_page(qdev->pdev,
- lbq_desc->p.pg_chunk.map,
- ql_lbq_block_size(qdev),
+ if (lbq_desc->p.pg_chunk.offset == last_offset)
+ pci_unmap_page(qdev->pdev, lbq_desc->p.pg_chunk.map,
+ ql_lbq_block_size(qdev),
PCI_DMA_FROMDEVICE);
- lbq_desc->p.pg_chunk.last_flag = 0;
- }
put_page(lbq_desc->p.pg_chunk.page);
lbq_desc->p.pg_chunk.page = NULL;
--
2.21.0
^ permalink raw reply related [flat|nested] 46+ messages in thread* RE: [PATCH net-next 02/16] qlge: Remove page_chunk.last_flag
2019-06-17 7:48 ` [PATCH net-next 02/16] qlge: Remove page_chunk.last_flag Benjamin Poirier
@ 2019-06-26 9:12 ` Manish Chopra
0 siblings, 0 replies; 46+ messages in thread
From: Manish Chopra @ 2019-06-26 9:12 UTC (permalink / raw)
To: Benjamin Poirier, GR-Linux-NIC-Dev, netdev
> -----Original Message-----
> From: Benjamin Poirier <bpoirier@suse.com>
> Sent: Monday, June 17, 2019 1:19 PM
> To: Manish Chopra <manishc@marvell.com>; GR-Linux-NIC-Dev <GR-Linux-
> NIC-Dev@marvell.com>; netdev@vger.kernel.org
> Subject: [PATCH net-next 02/16] qlge: Remove page_chunk.last_flag
>
> As already done in ql_get_curr_lchunk(), this member can be replaced by a
> simple test.
>
> Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
> ---
> drivers/net/ethernet/qlogic/qlge/qlge.h | 1 -
> drivers/net/ethernet/qlogic/qlge/qlge_main.c | 13 +++++--------
> 2 files changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/ethernet/qlogic/qlge/qlge.h
> b/drivers/net/ethernet/qlogic/qlge/qlge.h
> index 5d9a36deda08..0a156a95e981 100644
> --- a/drivers/net/ethernet/qlogic/qlge/qlge.h
> +++ b/drivers/net/ethernet/qlogic/qlge/qlge.h
> @@ -1363,7 +1363,6 @@ struct page_chunk {
> char *va; /* virt addr for this chunk */
> u64 map; /* mapping for master */
> unsigned int offset; /* offset for this chunk */
> - unsigned int last_flag; /* flag set for last chunk in page */
> };
>
> struct bq_desc {
> diff --git a/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> index 0bfbe11db795..038a6bfc79c7 100644
> --- a/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> +++ b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> @@ -1077,11 +1077,9 @@ static int ql_get_next_chunk(struct ql_adapter
> *qdev, struct rx_ring *rx_ring,
> rx_ring->pg_chunk.offset += rx_ring->lbq_buf_size;
> if (rx_ring->pg_chunk.offset == ql_lbq_block_size(qdev)) {
> rx_ring->pg_chunk.page = NULL;
> - lbq_desc->p.pg_chunk.last_flag = 1;
> } else {
> rx_ring->pg_chunk.va += rx_ring->lbq_buf_size;
> get_page(rx_ring->pg_chunk.page);
> - lbq_desc->p.pg_chunk.last_flag = 0;
> }
> return 0;
> }
> @@ -2778,6 +2776,8 @@ static int ql_alloc_tx_resources(struct ql_adapter
> *qdev,
>
> static void ql_free_lbq_buffers(struct ql_adapter *qdev, struct rx_ring
> *rx_ring) {
> + unsigned int last_offset = ql_lbq_block_size(qdev) -
> + rx_ring->lbq_buf_size;
> struct bq_desc *lbq_desc;
>
> uint32_t curr_idx, clean_idx;
> @@ -2787,13 +2787,10 @@ static void ql_free_lbq_buffers(struct ql_adapter
> *qdev, struct rx_ring *rx_ring
> while (curr_idx != clean_idx) {
> lbq_desc = &rx_ring->lbq[curr_idx];
>
> - if (lbq_desc->p.pg_chunk.last_flag) {
> - pci_unmap_page(qdev->pdev,
> - lbq_desc->p.pg_chunk.map,
> - ql_lbq_block_size(qdev),
> + if (lbq_desc->p.pg_chunk.offset == last_offset)
> + pci_unmap_page(qdev->pdev, lbq_desc-
> >p.pg_chunk.map,
> + ql_lbq_block_size(qdev),
> PCI_DMA_FROMDEVICE);
> - lbq_desc->p.pg_chunk.last_flag = 0;
> - }
>
> put_page(lbq_desc->p.pg_chunk.page);
> lbq_desc->p.pg_chunk.page = NULL;
> --
> 2.21.0
Acked-by: Manish Chopra <manishc@marvell.com>
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH net-next 03/16] qlge: Deduplicate lbq_buf_size
2019-06-17 7:48 [PATCH net-next 01/16] qlge: Remove irq_cnt Benjamin Poirier
2019-06-17 7:48 ` [PATCH net-next 02/16] qlge: Remove page_chunk.last_flag Benjamin Poirier
@ 2019-06-17 7:48 ` Benjamin Poirier
2019-06-26 9:24 ` [EXT] " Manish Chopra
2019-06-17 7:48 ` [PATCH net-next 04/16] qlge: Remove bq_desc.maplen Benjamin Poirier
` (15 subsequent siblings)
17 siblings, 1 reply; 46+ messages in thread
From: Benjamin Poirier @ 2019-06-17 7:48 UTC (permalink / raw)
To: Manish Chopra, GR-Linux-NIC-Dev, netdev
lbq_buf_size is duplicated to every rx_ring structure whereas lbq_buf_order
is present once in the ql_adapter structure. All rings use the same buf
size, keep only one copy of it. Also factor out the calculation of
lbq_buf_size instead of having two copies.
Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
---
drivers/net/ethernet/qlogic/qlge/qlge.h | 2 +-
drivers/net/ethernet/qlogic/qlge/qlge_dbg.c | 2 +-
drivers/net/ethernet/qlogic/qlge/qlge_main.c | 61 +++++++++-----------
3 files changed, 28 insertions(+), 37 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qlge/qlge.h b/drivers/net/ethernet/qlogic/qlge/qlge.h
index 0a156a95e981..ba61b4559dd6 100644
--- a/drivers/net/ethernet/qlogic/qlge/qlge.h
+++ b/drivers/net/ethernet/qlogic/qlge/qlge.h
@@ -1433,7 +1433,6 @@ struct rx_ring {
/* Large buffer queue elements. */
u32 lbq_len; /* entry count */
u32 lbq_size; /* size in bytes of queue */
- u32 lbq_buf_size;
void *lbq_base;
dma_addr_t lbq_base_dma;
void *lbq_base_indirect;
@@ -2108,6 +2107,7 @@ struct ql_adapter {
struct rx_ring rx_ring[MAX_RX_RINGS];
struct tx_ring tx_ring[MAX_TX_RINGS];
unsigned int lbq_buf_order;
+ u32 lbq_buf_size;
int rx_csum;
u32 default_rx_queue;
diff --git a/drivers/net/ethernet/qlogic/qlge/qlge_dbg.c b/drivers/net/ethernet/qlogic/qlge/qlge_dbg.c
index 31389ab8bdf7..46599d74c6fb 100644
--- a/drivers/net/ethernet/qlogic/qlge/qlge_dbg.c
+++ b/drivers/net/ethernet/qlogic/qlge/qlge_dbg.c
@@ -1630,6 +1630,7 @@ void ql_dump_qdev(struct ql_adapter *qdev)
DUMP_QDEV_FIELD(qdev, "0x%08x", xg_sem_mask);
DUMP_QDEV_FIELD(qdev, "0x%08x", port_link_up);
DUMP_QDEV_FIELD(qdev, "0x%08x", port_init);
+ DUMP_QDEV_FIELD(qdev, "%u", lbq_buf_size);
}
#endif
@@ -1774,7 +1775,6 @@ void ql_dump_rx_ring(struct rx_ring *rx_ring)
pr_err("rx_ring->lbq_curr_idx = %d\n", rx_ring->lbq_curr_idx);
pr_err("rx_ring->lbq_clean_idx = %d\n", rx_ring->lbq_clean_idx);
pr_err("rx_ring->lbq_free_cnt = %d\n", rx_ring->lbq_free_cnt);
- pr_err("rx_ring->lbq_buf_size = %d\n", rx_ring->lbq_buf_size);
pr_err("rx_ring->sbq_base = %p\n", rx_ring->sbq_base);
pr_err("rx_ring->sbq_base_dma = %llx\n",
diff --git a/drivers/net/ethernet/qlogic/qlge/qlge_main.c b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
index 038a6bfc79c7..9df06ad3fb93 100644
--- a/drivers/net/ethernet/qlogic/qlge/qlge_main.c
+++ b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
@@ -995,15 +995,14 @@ static struct bq_desc *ql_get_curr_lchunk(struct ql_adapter *qdev,
struct bq_desc *lbq_desc = ql_get_curr_lbuf(rx_ring);
pci_dma_sync_single_for_cpu(qdev->pdev,
- dma_unmap_addr(lbq_desc, mapaddr),
- rx_ring->lbq_buf_size,
- PCI_DMA_FROMDEVICE);
+ dma_unmap_addr(lbq_desc, mapaddr),
+ qdev->lbq_buf_size, PCI_DMA_FROMDEVICE);
/* If it's the last chunk of our master page then
* we unmap it.
*/
- if ((lbq_desc->p.pg_chunk.offset + rx_ring->lbq_buf_size)
- == ql_lbq_block_size(qdev))
+ if (lbq_desc->p.pg_chunk.offset + qdev->lbq_buf_size ==
+ ql_lbq_block_size(qdev))
pci_unmap_page(qdev->pdev,
lbq_desc->p.pg_chunk.map,
ql_lbq_block_size(qdev),
@@ -1074,11 +1073,11 @@ static int ql_get_next_chunk(struct ql_adapter *qdev, struct rx_ring *rx_ring,
/* Adjust the master page chunk for next
* buffer get.
*/
- rx_ring->pg_chunk.offset += rx_ring->lbq_buf_size;
+ rx_ring->pg_chunk.offset += qdev->lbq_buf_size;
if (rx_ring->pg_chunk.offset == ql_lbq_block_size(qdev)) {
rx_ring->pg_chunk.page = NULL;
} else {
- rx_ring->pg_chunk.va += rx_ring->lbq_buf_size;
+ rx_ring->pg_chunk.va += qdev->lbq_buf_size;
get_page(rx_ring->pg_chunk.page);
}
return 0;
@@ -1110,12 +1109,12 @@ static void ql_update_lbq(struct ql_adapter *qdev, struct rx_ring *rx_ring)
lbq_desc->p.pg_chunk.offset;
dma_unmap_addr_set(lbq_desc, mapaddr, map);
dma_unmap_len_set(lbq_desc, maplen,
- rx_ring->lbq_buf_size);
+ qdev->lbq_buf_size);
*lbq_desc->addr = cpu_to_le64(map);
pci_dma_sync_single_for_device(qdev->pdev, map,
- rx_ring->lbq_buf_size,
- PCI_DMA_FROMDEVICE);
+ qdev->lbq_buf_size,
+ PCI_DMA_FROMDEVICE);
clean_idx++;
if (clean_idx == rx_ring->lbq_len)
clean_idx = 0;
@@ -1880,8 +1879,7 @@ static struct sk_buff *ql_build_rx_skb(struct ql_adapter *qdev,
}
do {
lbq_desc = ql_get_curr_lchunk(qdev, rx_ring);
- size = (length < rx_ring->lbq_buf_size) ? length :
- rx_ring->lbq_buf_size;
+ size = min(length, qdev->lbq_buf_size);
netif_printk(qdev, rx_status, KERN_DEBUG, qdev->ndev,
"Adding page %d to skb for %d bytes.\n",
@@ -2776,12 +2774,12 @@ static int ql_alloc_tx_resources(struct ql_adapter *qdev,
static void ql_free_lbq_buffers(struct ql_adapter *qdev, struct rx_ring *rx_ring)
{
- unsigned int last_offset = ql_lbq_block_size(qdev) -
- rx_ring->lbq_buf_size;
+ unsigned int last_offset;
struct bq_desc *lbq_desc;
uint32_t curr_idx, clean_idx;
+ last_offset = ql_lbq_block_size(qdev) - qdev->lbq_buf_size;
curr_idx = rx_ring->lbq_curr_idx;
clean_idx = rx_ring->lbq_clean_idx;
while (curr_idx != clean_idx) {
@@ -3149,8 +3147,8 @@ static int ql_start_rx_ring(struct ql_adapter *qdev, struct rx_ring *rx_ring)
} while (page_entries < MAX_DB_PAGES_PER_BQ(rx_ring->lbq_len));
cqicb->lbq_addr =
cpu_to_le64(rx_ring->lbq_base_indirect_dma);
- bq_len = (rx_ring->lbq_buf_size == 65536) ? 0 :
- (u16) rx_ring->lbq_buf_size;
+ bq_len = (qdev->lbq_buf_size == 65536) ? 0 :
+ (u16)qdev->lbq_buf_size;
cqicb->lbq_buf_size = cpu_to_le16(bq_len);
bq_len = (rx_ring->lbq_len == 65536) ? 0 :
(u16) rx_ring->lbq_len;
@@ -4048,16 +4046,21 @@ static int qlge_close(struct net_device *ndev)
return 0;
}
+static void qlge_set_lb_size(struct ql_adapter *qdev)
+{
+ if (qdev->ndev->mtu <= 1500)
+ qdev->lbq_buf_size = LARGE_BUFFER_MIN_SIZE;
+ else
+ qdev->lbq_buf_size = LARGE_BUFFER_MAX_SIZE;
+ qdev->lbq_buf_order = get_order(qdev->lbq_buf_size);
+}
+
static int ql_configure_rings(struct ql_adapter *qdev)
{
int i;
struct rx_ring *rx_ring;
struct tx_ring *tx_ring;
int cpu_cnt = min(MAX_CPUS, (int)num_online_cpus());
- unsigned int lbq_buf_len = (qdev->ndev->mtu > 1500) ?
- LARGE_BUFFER_MAX_SIZE : LARGE_BUFFER_MIN_SIZE;
-
- qdev->lbq_buf_order = get_order(lbq_buf_len);
/* In a perfect world we have one RSS ring for each CPU
* and each has it's own vector. To do that we ask for
@@ -4105,7 +4108,6 @@ static int ql_configure_rings(struct ql_adapter *qdev)
rx_ring->lbq_len = NUM_LARGE_BUFFERS;
rx_ring->lbq_size =
rx_ring->lbq_len * sizeof(__le64);
- rx_ring->lbq_buf_size = (u16)lbq_buf_len;
rx_ring->sbq_len = NUM_SMALL_BUFFERS;
rx_ring->sbq_size =
rx_ring->sbq_len * sizeof(__le64);
@@ -4121,7 +4123,6 @@ static int ql_configure_rings(struct ql_adapter *qdev)
rx_ring->cq_len * sizeof(struct ql_net_rsp_iocb);
rx_ring->lbq_len = 0;
rx_ring->lbq_size = 0;
- rx_ring->lbq_buf_size = 0;
rx_ring->sbq_len = 0;
rx_ring->sbq_size = 0;
rx_ring->sbq_buf_size = 0;
@@ -4140,6 +4141,7 @@ static int qlge_open(struct net_device *ndev)
if (err)
return err;
+ qlge_set_lb_size(qdev);
err = ql_configure_rings(qdev);
if (err)
return err;
@@ -4161,9 +4163,7 @@ static int qlge_open(struct net_device *ndev)
static int ql_change_rx_buffers(struct ql_adapter *qdev)
{
- struct rx_ring *rx_ring;
- int i, status;
- u32 lbq_buf_len;
+ int status;
/* Wait for an outstanding reset to complete. */
if (!test_bit(QL_ADAPTER_UP, &qdev->flags)) {
@@ -4186,16 +4186,7 @@ static int ql_change_rx_buffers(struct ql_adapter *qdev)
if (status)
goto error;
- /* Get the new rx buffer size. */
- lbq_buf_len = (qdev->ndev->mtu > 1500) ?
- LARGE_BUFFER_MAX_SIZE : LARGE_BUFFER_MIN_SIZE;
- qdev->lbq_buf_order = get_order(lbq_buf_len);
-
- for (i = 0; i < qdev->rss_ring_count; i++) {
- rx_ring = &qdev->rx_ring[i];
- /* Set the new size. */
- rx_ring->lbq_buf_size = lbq_buf_len;
- }
+ qlge_set_lb_size(qdev);
status = ql_adapter_up(qdev);
if (status)
--
2.21.0
^ permalink raw reply related [flat|nested] 46+ messages in thread* RE: [EXT] [PATCH net-next 03/16] qlge: Deduplicate lbq_buf_size
2019-06-17 7:48 ` [PATCH net-next 03/16] qlge: Deduplicate lbq_buf_size Benjamin Poirier
@ 2019-06-26 9:24 ` Manish Chopra
2019-06-26 11:37 ` Benjamin Poirier
0 siblings, 1 reply; 46+ messages in thread
From: Manish Chopra @ 2019-06-26 9:24 UTC (permalink / raw)
To: Benjamin Poirier, GR-Linux-NIC-Dev, netdev
> -----Original Message-----
> From: Benjamin Poirier <bpoirier@suse.com>
> Sent: Monday, June 17, 2019 1:19 PM
> To: Manish Chopra <manishc@marvell.com>; GR-Linux-NIC-Dev <GR-Linux-
> NIC-Dev@marvell.com>; netdev@vger.kernel.org
> Subject: [EXT] [PATCH net-next 03/16] qlge: Deduplicate lbq_buf_size
>
> External Email
>
> ----------------------------------------------------------------------
> lbq_buf_size is duplicated to every rx_ring structure whereas lbq_buf_order is
> present once in the ql_adapter structure. All rings use the same buf size, keep
> only one copy of it. Also factor out the calculation of lbq_buf_size instead of
> having two copies.
>
> Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
> ---
> drivers/net/ethernet/qlogic/qlge/qlge.h | 2 +-
> drivers/net/ethernet/qlogic/qlge/qlge_dbg.c | 2 +-
> drivers/net/ethernet/qlogic/qlge/qlge_main.c | 61 +++++++++-----------
> 3 files changed, 28 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/net/ethernet/qlogic/qlge/qlge.h
> b/drivers/net/ethernet/qlogic/qlge/qlge.h
> index 0a156a95e981..ba61b4559dd6 100644
> --- a/drivers/net/ethernet/qlogic/qlge/qlge.h
> +++ b/drivers/net/ethernet/qlogic/qlge/qlge.h
> @@ -1433,7 +1433,6 @@ struct rx_ring {
> /* Large buffer queue elements. */
> u32 lbq_len; /* entry count */
> u32 lbq_size; /* size in bytes of queue */
> - u32 lbq_buf_size;
> void *lbq_base;
> dma_addr_t lbq_base_dma;
> void *lbq_base_indirect;
> @@ -2108,6 +2107,7 @@ struct ql_adapter {
> struct rx_ring rx_ring[MAX_RX_RINGS];
> struct tx_ring tx_ring[MAX_TX_RINGS];
> unsigned int lbq_buf_order;
> + u32 lbq_buf_size;
>
> int rx_csum;
> u32 default_rx_queue;
> diff --git a/drivers/net/ethernet/qlogic/qlge/qlge_dbg.c
> b/drivers/net/ethernet/qlogic/qlge/qlge_dbg.c
> index 31389ab8bdf7..46599d74c6fb 100644
> --- a/drivers/net/ethernet/qlogic/qlge/qlge_dbg.c
> +++ b/drivers/net/ethernet/qlogic/qlge/qlge_dbg.c
> @@ -1630,6 +1630,7 @@ void ql_dump_qdev(struct ql_adapter *qdev)
> DUMP_QDEV_FIELD(qdev, "0x%08x", xg_sem_mask);
> DUMP_QDEV_FIELD(qdev, "0x%08x", port_link_up);
> DUMP_QDEV_FIELD(qdev, "0x%08x", port_init);
> + DUMP_QDEV_FIELD(qdev, "%u", lbq_buf_size);
> }
> #endif
>
> @@ -1774,7 +1775,6 @@ void ql_dump_rx_ring(struct rx_ring *rx_ring)
> pr_err("rx_ring->lbq_curr_idx = %d\n", rx_ring->lbq_curr_idx);
> pr_err("rx_ring->lbq_clean_idx = %d\n", rx_ring->lbq_clean_idx);
> pr_err("rx_ring->lbq_free_cnt = %d\n", rx_ring->lbq_free_cnt);
> - pr_err("rx_ring->lbq_buf_size = %d\n", rx_ring->lbq_buf_size);
>
> pr_err("rx_ring->sbq_base = %p\n", rx_ring->sbq_base);
> pr_err("rx_ring->sbq_base_dma = %llx\n", diff --git
> a/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> index 038a6bfc79c7..9df06ad3fb93 100644
> --- a/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> +++ b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> @@ -995,15 +995,14 @@ static struct bq_desc *ql_get_curr_lchunk(struct
> ql_adapter *qdev,
> struct bq_desc *lbq_desc = ql_get_curr_lbuf(rx_ring);
>
> pci_dma_sync_single_for_cpu(qdev->pdev,
> - dma_unmap_addr(lbq_desc,
> mapaddr),
> - rx_ring->lbq_buf_size,
> - PCI_DMA_FROMDEVICE);
> + dma_unmap_addr(lbq_desc, mapaddr),
> + qdev->lbq_buf_size,
> PCI_DMA_FROMDEVICE);
>
> /* If it's the last chunk of our master page then
> * we unmap it.
> */
> - if ((lbq_desc->p.pg_chunk.offset + rx_ring->lbq_buf_size)
> - == ql_lbq_block_size(qdev))
> + if (lbq_desc->p.pg_chunk.offset + qdev->lbq_buf_size ==
> + ql_lbq_block_size(qdev))
> pci_unmap_page(qdev->pdev,
> lbq_desc->p.pg_chunk.map,
> ql_lbq_block_size(qdev),
> @@ -1074,11 +1073,11 @@ static int ql_get_next_chunk(struct ql_adapter
> *qdev, struct rx_ring *rx_ring,
> /* Adjust the master page chunk for next
> * buffer get.
> */
> - rx_ring->pg_chunk.offset += rx_ring->lbq_buf_size;
> + rx_ring->pg_chunk.offset += qdev->lbq_buf_size;
> if (rx_ring->pg_chunk.offset == ql_lbq_block_size(qdev)) {
> rx_ring->pg_chunk.page = NULL;
> } else {
> - rx_ring->pg_chunk.va += rx_ring->lbq_buf_size;
> + rx_ring->pg_chunk.va += qdev->lbq_buf_size;
> get_page(rx_ring->pg_chunk.page);
> }
> return 0;
> @@ -1110,12 +1109,12 @@ static void ql_update_lbq(struct ql_adapter
> *qdev, struct rx_ring *rx_ring)
> lbq_desc->p.pg_chunk.offset;
> dma_unmap_addr_set(lbq_desc, mapaddr, map);
> dma_unmap_len_set(lbq_desc, maplen,
> - rx_ring->lbq_buf_size);
> + qdev->lbq_buf_size);
> *lbq_desc->addr = cpu_to_le64(map);
>
> pci_dma_sync_single_for_device(qdev->pdev, map,
> - rx_ring->lbq_buf_size,
> - PCI_DMA_FROMDEVICE);
> + qdev->lbq_buf_size,
> + PCI_DMA_FROMDEVICE);
> clean_idx++;
> if (clean_idx == rx_ring->lbq_len)
> clean_idx = 0;
> @@ -1880,8 +1879,7 @@ static struct sk_buff *ql_build_rx_skb(struct
> ql_adapter *qdev,
> }
> do {
> lbq_desc = ql_get_curr_lchunk(qdev, rx_ring);
> - size = (length < rx_ring->lbq_buf_size) ? length :
> - rx_ring->lbq_buf_size;
> + size = min(length, qdev->lbq_buf_size);
>
> netif_printk(qdev, rx_status, KERN_DEBUG, qdev-
> >ndev,
> "Adding page %d to skb for %d bytes.\n",
> @@ -2776,12 +2774,12 @@ static int ql_alloc_tx_resources(struct ql_adapter
> *qdev,
>
> static void ql_free_lbq_buffers(struct ql_adapter *qdev, struct rx_ring
> *rx_ring) {
> - unsigned int last_offset = ql_lbq_block_size(qdev) -
> - rx_ring->lbq_buf_size;
> + unsigned int last_offset;
> struct bq_desc *lbq_desc;
>
> uint32_t curr_idx, clean_idx;
>
> + last_offset = ql_lbq_block_size(qdev) - qdev->lbq_buf_size;
> curr_idx = rx_ring->lbq_curr_idx;
> clean_idx = rx_ring->lbq_clean_idx;
> while (curr_idx != clean_idx) {
> @@ -3149,8 +3147,8 @@ static int ql_start_rx_ring(struct ql_adapter *qdev,
> struct rx_ring *rx_ring)
> } while (page_entries < MAX_DB_PAGES_PER_BQ(rx_ring-
> >lbq_len));
> cqicb->lbq_addr =
> cpu_to_le64(rx_ring->lbq_base_indirect_dma);
> - bq_len = (rx_ring->lbq_buf_size == 65536) ? 0 :
> - (u16) rx_ring->lbq_buf_size;
> + bq_len = (qdev->lbq_buf_size == 65536) ? 0 :
> + (u16)qdev->lbq_buf_size;
> cqicb->lbq_buf_size = cpu_to_le16(bq_len);
> bq_len = (rx_ring->lbq_len == 65536) ? 0 :
> (u16) rx_ring->lbq_len;
> @@ -4048,16 +4046,21 @@ static int qlge_close(struct net_device *ndev)
> return 0;
> }
>
> +static void qlge_set_lb_size(struct ql_adapter *qdev) {
> + if (qdev->ndev->mtu <= 1500)
> + qdev->lbq_buf_size = LARGE_BUFFER_MIN_SIZE;
> + else
> + qdev->lbq_buf_size = LARGE_BUFFER_MAX_SIZE;
> + qdev->lbq_buf_order = get_order(qdev->lbq_buf_size); }
> +
> static int ql_configure_rings(struct ql_adapter *qdev) {
> int i;
> struct rx_ring *rx_ring;
> struct tx_ring *tx_ring;
> int cpu_cnt = min(MAX_CPUS, (int)num_online_cpus());
> - unsigned int lbq_buf_len = (qdev->ndev->mtu > 1500) ?
> - LARGE_BUFFER_MAX_SIZE : LARGE_BUFFER_MIN_SIZE;
> -
> - qdev->lbq_buf_order = get_order(lbq_buf_len);
>
> /* In a perfect world we have one RSS ring for each CPU
> * and each has it's own vector. To do that we ask for @@ -4105,7
> +4108,6 @@ static int ql_configure_rings(struct ql_adapter *qdev)
> rx_ring->lbq_len = NUM_LARGE_BUFFERS;
> rx_ring->lbq_size =
> rx_ring->lbq_len * sizeof(__le64);
> - rx_ring->lbq_buf_size = (u16)lbq_buf_len;
> rx_ring->sbq_len = NUM_SMALL_BUFFERS;
> rx_ring->sbq_size =
> rx_ring->sbq_len * sizeof(__le64); @@ -4121,7
> +4123,6 @@ static int ql_configure_rings(struct ql_adapter *qdev)
> rx_ring->cq_len * sizeof(struct ql_net_rsp_iocb);
> rx_ring->lbq_len = 0;
> rx_ring->lbq_size = 0;
> - rx_ring->lbq_buf_size = 0;
> rx_ring->sbq_len = 0;
> rx_ring->sbq_size = 0;
> rx_ring->sbq_buf_size = 0;
> @@ -4140,6 +4141,7 @@ static int qlge_open(struct net_device *ndev)
> if (err)
> return err;
>
> + qlge_set_lb_size(qdev);
> err = ql_configure_rings(qdev);
> if (err)
> return err;
> @@ -4161,9 +4163,7 @@ static int qlge_open(struct net_device *ndev)
>
> static int ql_change_rx_buffers(struct ql_adapter *qdev) {
> - struct rx_ring *rx_ring;
> - int i, status;
> - u32 lbq_buf_len;
> + int status;
>
> /* Wait for an outstanding reset to complete. */
> if (!test_bit(QL_ADAPTER_UP, &qdev->flags)) { @@ -4186,16 +4186,7
> @@ static int ql_change_rx_buffers(struct ql_adapter *qdev)
> if (status)
> goto error;
>
> - /* Get the new rx buffer size. */
> - lbq_buf_len = (qdev->ndev->mtu > 1500) ?
> - LARGE_BUFFER_MAX_SIZE : LARGE_BUFFER_MIN_SIZE;
> - qdev->lbq_buf_order = get_order(lbq_buf_len);
> -
> - for (i = 0; i < qdev->rss_ring_count; i++) {
> - rx_ring = &qdev->rx_ring[i];
> - /* Set the new size. */
> - rx_ring->lbq_buf_size = lbq_buf_len;
> - }
> + qlge_set_lb_size(qdev);
>
> status = ql_adapter_up(qdev);
> if (status)
> --
> 2.21.0
Not sure if this change is really required, I think fields relevant to rx_ring should be present in the rx_ring structure.
There are various other fields like "lbq_len" and "lbq_size" which would be same for all rx rings but still under the relevant rx_ring structure.
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [EXT] [PATCH net-next 03/16] qlge: Deduplicate lbq_buf_size
2019-06-26 9:24 ` [EXT] " Manish Chopra
@ 2019-06-26 11:37 ` Benjamin Poirier
2019-06-26 15:42 ` Willem de Bruijn
0 siblings, 1 reply; 46+ messages in thread
From: Benjamin Poirier @ 2019-06-26 11:37 UTC (permalink / raw)
To: Manish Chopra; +Cc: GR-Linux-NIC-Dev, netdev
On 2019/06/26 09:24, Manish Chopra wrote:
> > -----Original Message-----
> > From: Benjamin Poirier <bpoirier@suse.com>
> > Sent: Monday, June 17, 2019 1:19 PM
> > To: Manish Chopra <manishc@marvell.com>; GR-Linux-NIC-Dev <GR-Linux-
> > NIC-Dev@marvell.com>; netdev@vger.kernel.org
> > Subject: [EXT] [PATCH net-next 03/16] qlge: Deduplicate lbq_buf_size
> >
> > External Email
> >
> > ----------------------------------------------------------------------
> > lbq_buf_size is duplicated to every rx_ring structure whereas lbq_buf_order is
> > present once in the ql_adapter structure. All rings use the same buf size, keep
> > only one copy of it. Also factor out the calculation of lbq_buf_size instead of
> > having two copies.
> >
> > Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
> > ---
[...]
>
> Not sure if this change is really required, I think fields relevant to rx_ring should be present in the rx_ring structure.
> There are various other fields like "lbq_len" and "lbq_size" which would be same for all rx rings but still under the relevant rx_ring structure.
Indeed, those members are also removed by this patch series, in patch 11.
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [EXT] [PATCH net-next 03/16] qlge: Deduplicate lbq_buf_size
2019-06-26 11:37 ` Benjamin Poirier
@ 2019-06-26 15:42 ` Willem de Bruijn
2019-06-28 8:57 ` Benjamin Poirier
0 siblings, 1 reply; 46+ messages in thread
From: Willem de Bruijn @ 2019-06-26 15:42 UTC (permalink / raw)
To: Benjamin Poirier; +Cc: Manish Chopra, GR-Linux-NIC-Dev, netdev
On Wed, Jun 26, 2019 at 7:37 AM Benjamin Poirier <bpoirier@suse.com> wrote:
>
> On 2019/06/26 09:24, Manish Chopra wrote:
> > > -----Original Message-----
> > > From: Benjamin Poirier <bpoirier@suse.com>
> > > Sent: Monday, June 17, 2019 1:19 PM
> > > To: Manish Chopra <manishc@marvell.com>; GR-Linux-NIC-Dev <GR-Linux-
> > > NIC-Dev@marvell.com>; netdev@vger.kernel.org
> > > Subject: [EXT] [PATCH net-next 03/16] qlge: Deduplicate lbq_buf_size
> > >
> > > External Email
> > >
> > > ----------------------------------------------------------------------
> > > lbq_buf_size is duplicated to every rx_ring structure whereas lbq_buf_order is
> > > present once in the ql_adapter structure. All rings use the same buf size, keep
> > > only one copy of it. Also factor out the calculation of lbq_buf_size instead of
> > > having two copies.
> > >
> > > Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
> > > ---
> [...]
> >
> > Not sure if this change is really required, I think fields relevant to rx_ring should be present in the rx_ring structure.
> > There are various other fields like "lbq_len" and "lbq_size" which would be same for all rx rings but still under the relevant rx_ring structure.
The one argument against deduplicating might be if the original fields
are in a hot cacheline and the new location adds a cacheline access to
a hot path. Not sure if that is relevant here. But maybe something to
double check.
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [EXT] [PATCH net-next 03/16] qlge: Deduplicate lbq_buf_size
2019-06-26 15:42 ` Willem de Bruijn
@ 2019-06-28 8:57 ` Benjamin Poirier
2019-06-28 14:56 ` Willem de Bruijn
0 siblings, 1 reply; 46+ messages in thread
From: Benjamin Poirier @ 2019-06-28 8:57 UTC (permalink / raw)
To: Willem de Bruijn; +Cc: Manish Chopra, GR-Linux-NIC-Dev, netdev
On 2019/06/26 11:42, Willem de Bruijn wrote:
> On Wed, Jun 26, 2019 at 7:37 AM Benjamin Poirier <bpoirier@suse.com> wrote:
> >
> > On 2019/06/26 09:24, Manish Chopra wrote:
> > > > -----Original Message-----
> > > > From: Benjamin Poirier <bpoirier@suse.com>
> > > > Sent: Monday, June 17, 2019 1:19 PM
> > > > To: Manish Chopra <manishc@marvell.com>; GR-Linux-NIC-Dev <GR-Linux-
> > > > NIC-Dev@marvell.com>; netdev@vger.kernel.org
> > > > Subject: [EXT] [PATCH net-next 03/16] qlge: Deduplicate lbq_buf_size
> > > >
> > > > External Email
> > > >
> > > > ----------------------------------------------------------------------
> > > > lbq_buf_size is duplicated to every rx_ring structure whereas lbq_buf_order is
> > > > present once in the ql_adapter structure. All rings use the same buf size, keep
> > > > only one copy of it. Also factor out the calculation of lbq_buf_size instead of
> > > > having two copies.
> > > >
> > > > Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
> > > > ---
> > [...]
> > >
> > > Not sure if this change is really required, I think fields relevant to rx_ring should be present in the rx_ring structure.
> > > There are various other fields like "lbq_len" and "lbq_size" which would be same for all rx rings but still under the relevant rx_ring structure.
>
> The one argument against deduplicating might be if the original fields
> are in a hot cacheline and the new location adds a cacheline access to
> a hot path. Not sure if that is relevant here. But maybe something to
> double check.
>
Thanks for the hint. I didn't check before because my hunch was that
this driver is not near that level of optimization but I checked now and
got the following results.
The other side of the link is doing pktgen of 257B frames (smallest to
exercise the lbq path), over 10 runs of 100s, number of frames received
by the stack before/after applying this patch:
before: 1487000±2000pps
after: 1538000±2000pps
(3.4% improvement)
Looking with perf stat, boiling down the numbers to be per packet, most
interesting differences are:
cycles -4.32%
instructions -0.97%
L1-dcache-loads +6.30%
L1-dcache-load-misses -1.26%
LLC-loads +2.93%
LLC-load-misses -1.16%
So, fewer cycles/packet, fewer cache misses. Note however that
L1-dcache-loads had high variability between runs (~12%).
Looking with pahole, struct rx_ring is reduced by 8 bytes but its a 784
bytes structure full of holes to begin with.
Numbers are from an old Xeon E3-1275
Full output follows:
root@dtest:~# cat /tmp/mini.sh
#!/bin/bash
da=$(ethtool -S ql1 | awk '/rx_nic_fifo_drop/ {print $2}')
ra=$(ip -s link show dev ql1 | awk 'NR == 4 {print $2}')
sleep 100
db=$(ethtool -S ql1 | awk '/rx_nic_fifo_drop/ {print $2}')
rb=$(ip -s link show dev ql1 | awk 'NR == 4 {print $2}')
echo "rx $((rb - ra))"
echo "dropped $((db - da))"
root@dtest:~# perf stat -a -r10 -d /tmp/mini.sh
rx 148763519
dropped 296128797
rx 148784865
dropped 296107878
rx 148675349
dropped 296217234
rx 148634271
dropped 296260358
rx 148813581
dropped 296089785
rx 148755713
dropped 296136071
rx 148434116
dropped 296459743
rx 148390638
dropped 296501405
rx 148500812
dropped 296391286
rx 148973912
dropped 295920014
Performance counter stats for 'system wide' (10 runs):
800,245.24 msec cpu-clock # 8.000 CPUs utilized ( +- 0.00% )
11,559 context-switches # 14.445 M/sec ( +- 0.13% )
36 cpu-migrations # 0.044 M/sec ( +- 7.11% )
62,100 page-faults # 77.601 M/sec ( +- 0.51% )
362,221,639,977 cycles # 452638.486 GHz ( +- 0.77% ) (49.92%)
515,318,132,327 instructions # 1.42 insn per cycle ( +- 0.64% ) (62.42%)
91,744,969,260 branches # 114646115.533 M/sec ( +- 0.04% ) (62.50%)
549,681,601 branch-misses # 0.60% of all branches ( +- 0.25% ) (62.50%)
115,207,692,963 L1-dcache-loads # 143965544.751 M/sec ( +- 12.09% ) (40.59%)
2,629,809,584 L1-dcache-load-misses # 2.28% of all L1-dcache hits ( +- 3.24% ) (29.90%)
837,806,984 LLC-loads # 1046938.235 M/sec ( +- 0.73% ) (25.31%)
569,343,079 LLC-load-misses # 67.96% of all LL-cache hits ( +- 0.07% ) (37.50%)
100.03177 +- 0.00104 seconds time elapsed ( +- 0.00% )
#
# changed the driver now to include this patch
#
root@dtest:~# perf stat -a -r10 -d /tmp/mini.sh
rx 154094001
dropped 290799944
rx 153798301
dropped 291094799
rx 153942166
dropped 290950792
rx 154150506
dropped 290743194
rx 154179791
dropped 290712400
rx 153907213
dropped 290985227
rx 153813515
dropped 291078475
rx 153603554
dropped 291289250
rx 153521414
dropped 291372145
rx 153592955
dropped 291299734
Performance counter stats for 'system wide' (10 runs):
800,234.85 msec cpu-clock # 8.000 CPUs utilized ( +- 0.00% )
12,061 context-switches # 15.072 M/sec ( +- 0.61% )
32 cpu-migrations # 0.040 M/sec ( +- 8.40% )
62,210 page-faults # 77.740 M/sec ( +- 0.60% )
358,454,546,598 cycles # 447936.882 GHz ( +- 1.34% ) (49.84%)
527,804,259,267 instructions # 1.47 insn per cycle ( +- 1.11% ) (62.35%)
94,904,030,904 branches # 118595275.389 M/sec ( +- 0.05% ) (62.50%)
568,214,180 branch-misses # 0.60% of all branches ( +- 0.12% ) (62.50%)
126,667,377,528 L1-dcache-loads # 158287823.791 M/sec ( +- 12.48% ) (41.21%)
2,685,865,791 L1-dcache-load-misses # 2.12% of all L1-dcache hits ( +- 4.01% ) (29.42%)
891,939,954 LLC-loads # 1114598.225 M/sec ( +- 0.73% ) (25.16%)
582,037,612 LLC-load-misses # 65.26% of all LL-cache hits ( +- 0.09% ) (37.50%)
100.0307117 +- 0.0000810 seconds time elapsed ( +- 0.00% )
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [EXT] [PATCH net-next 03/16] qlge: Deduplicate lbq_buf_size
2019-06-28 8:57 ` Benjamin Poirier
@ 2019-06-28 14:56 ` Willem de Bruijn
0 siblings, 0 replies; 46+ messages in thread
From: Willem de Bruijn @ 2019-06-28 14:56 UTC (permalink / raw)
To: Benjamin Poirier; +Cc: Manish Chopra, GR-Linux-NIC-Dev, netdev
On Fri, Jun 28, 2019 at 4:57 AM Benjamin Poirier <bpoirier@suse.com> wrote:
>
> On 2019/06/26 11:42, Willem de Bruijn wrote:
> > On Wed, Jun 26, 2019 at 7:37 AM Benjamin Poirier <bpoirier@suse.com> wrote:
> > >
> > > On 2019/06/26 09:24, Manish Chopra wrote:
> > > > > -----Original Message-----
> > > > > From: Benjamin Poirier <bpoirier@suse.com>
> > > > > Sent: Monday, June 17, 2019 1:19 PM
> > > > > To: Manish Chopra <manishc@marvell.com>; GR-Linux-NIC-Dev <GR-Linux-
> > > > > NIC-Dev@marvell.com>; netdev@vger.kernel.org
> > > > > Subject: [EXT] [PATCH net-next 03/16] qlge: Deduplicate lbq_buf_size
> > > > >
> > > > > External Email
> > > > >
> > > > > ----------------------------------------------------------------------
> > > > > lbq_buf_size is duplicated to every rx_ring structure whereas lbq_buf_order is
> > > > > present once in the ql_adapter structure. All rings use the same buf size, keep
> > > > > only one copy of it. Also factor out the calculation of lbq_buf_size instead of
> > > > > having two copies.
> > > > >
> > > > > Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
> > > > > ---
> > > [...]
> > > >
> > > > Not sure if this change is really required, I think fields relevant to rx_ring should be present in the rx_ring structure.
> > > > There are various other fields like "lbq_len" and "lbq_size" which would be same for all rx rings but still under the relevant rx_ring structure.
> >
> > The one argument against deduplicating might be if the original fields
> > are in a hot cacheline and the new location adds a cacheline access to
> > a hot path. Not sure if that is relevant here. But maybe something to
> > double check.
> >
>
> Thanks for the hint. I didn't check before because my hunch was that
> this driver is not near that level of optimization but I checked now and
> got the following results.
Thanks for the data. I didn't mean to ask you to do a lot of extra work.
Sorry if it resulted in that.
Fully agreed on your point about optimization (see also.. that 784B
struct with holes). I support the patch and meant to argue against the
previous response: this cleanup makes sense to me, just take a second
look at struct layout. To be more crystal clear:
Acked-by: Willem de Bruijn <willemb@google.com>
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH net-next 04/16] qlge: Remove bq_desc.maplen
2019-06-17 7:48 [PATCH net-next 01/16] qlge: Remove irq_cnt Benjamin Poirier
2019-06-17 7:48 ` [PATCH net-next 02/16] qlge: Remove page_chunk.last_flag Benjamin Poirier
2019-06-17 7:48 ` [PATCH net-next 03/16] qlge: Deduplicate lbq_buf_size Benjamin Poirier
@ 2019-06-17 7:48 ` Benjamin Poirier
2019-06-26 9:31 ` Manish Chopra
2019-06-17 7:48 ` [PATCH net-next 05/16] qlge: Remove rx_ring.sbq_buf_size Benjamin Poirier
` (14 subsequent siblings)
17 siblings, 1 reply; 46+ messages in thread
From: Benjamin Poirier @ 2019-06-17 7:48 UTC (permalink / raw)
To: Manish Chopra, GR-Linux-NIC-Dev, netdev
The size of the mapping is known statically in all cases, there's no need
to save it at runtime. Remove this member.
Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
---
drivers/net/ethernet/qlogic/qlge/qlge.h | 1 -
drivers/net/ethernet/qlogic/qlge/qlge_main.c | 43 +++++++-------------
2 files changed, 15 insertions(+), 29 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qlge/qlge.h b/drivers/net/ethernet/qlogic/qlge/qlge.h
index ba61b4559dd6..f32da8c7679f 100644
--- a/drivers/net/ethernet/qlogic/qlge/qlge.h
+++ b/drivers/net/ethernet/qlogic/qlge/qlge.h
@@ -1373,7 +1373,6 @@ struct bq_desc {
__le64 *addr;
u32 index;
DEFINE_DMA_UNMAP_ADDR(mapaddr);
- DEFINE_DMA_UNMAP_LEN(maplen);
};
#define QL_TXQ_IDX(qdev, skb) (smp_processor_id()%(qdev->tx_ring_count))
diff --git a/drivers/net/ethernet/qlogic/qlge/qlge_main.c b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
index 9df06ad3fb93..25dbaa9cc55d 100644
--- a/drivers/net/ethernet/qlogic/qlge/qlge_main.c
+++ b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
@@ -1108,8 +1108,6 @@ static void ql_update_lbq(struct ql_adapter *qdev, struct rx_ring *rx_ring)
map = lbq_desc->p.pg_chunk.map +
lbq_desc->p.pg_chunk.offset;
dma_unmap_addr_set(lbq_desc, mapaddr, map);
- dma_unmap_len_set(lbq_desc, maplen,
- qdev->lbq_buf_size);
*lbq_desc->addr = cpu_to_le64(map);
pci_dma_sync_single_for_device(qdev->pdev, map,
@@ -1177,8 +1175,6 @@ static void ql_update_sbq(struct ql_adapter *qdev, struct rx_ring *rx_ring)
return;
}
dma_unmap_addr_set(sbq_desc, mapaddr, map);
- dma_unmap_len_set(sbq_desc, maplen,
- rx_ring->sbq_buf_size);
*sbq_desc->addr = cpu_to_le64(map);
}
@@ -1598,14 +1594,14 @@ static void ql_process_mac_rx_skb(struct ql_adapter *qdev,
pci_dma_sync_single_for_cpu(qdev->pdev,
dma_unmap_addr(sbq_desc, mapaddr),
- dma_unmap_len(sbq_desc, maplen),
+ rx_ring->sbq_buf_size,
PCI_DMA_FROMDEVICE);
skb_put_data(new_skb, skb->data, length);
pci_dma_sync_single_for_device(qdev->pdev,
dma_unmap_addr(sbq_desc, mapaddr),
- dma_unmap_len(sbq_desc, maplen),
+ rx_ring->sbq_buf_size,
PCI_DMA_FROMDEVICE);
skb = new_skb;
@@ -1727,8 +1723,7 @@ static struct sk_buff *ql_build_rx_skb(struct ql_adapter *qdev,
sbq_desc = ql_get_curr_sbuf(rx_ring);
pci_unmap_single(qdev->pdev,
dma_unmap_addr(sbq_desc, mapaddr),
- dma_unmap_len(sbq_desc, maplen),
- PCI_DMA_FROMDEVICE);
+ rx_ring->sbq_buf_size, PCI_DMA_FROMDEVICE);
skb = sbq_desc->p.skb;
ql_realign_skb(skb, hdr_len);
skb_put(skb, hdr_len);
@@ -1758,19 +1753,15 @@ static struct sk_buff *ql_build_rx_skb(struct ql_adapter *qdev,
*/
sbq_desc = ql_get_curr_sbuf(rx_ring);
pci_dma_sync_single_for_cpu(qdev->pdev,
- dma_unmap_addr
- (sbq_desc, mapaddr),
- dma_unmap_len
- (sbq_desc, maplen),
+ dma_unmap_addr(sbq_desc,
+ mapaddr),
+ rx_ring->sbq_buf_size,
PCI_DMA_FROMDEVICE);
skb_put_data(skb, sbq_desc->p.skb->data, length);
pci_dma_sync_single_for_device(qdev->pdev,
- dma_unmap_addr
- (sbq_desc,
- mapaddr),
- dma_unmap_len
- (sbq_desc,
- maplen),
+ dma_unmap_addr(sbq_desc,
+ mapaddr),
+ rx_ring->sbq_buf_size,
PCI_DMA_FROMDEVICE);
} else {
netif_printk(qdev, rx_status, KERN_DEBUG, qdev->ndev,
@@ -1781,10 +1772,8 @@ static struct sk_buff *ql_build_rx_skb(struct ql_adapter *qdev,
ql_realign_skb(skb, length);
skb_put(skb, length);
pci_unmap_single(qdev->pdev,
- dma_unmap_addr(sbq_desc,
- mapaddr),
- dma_unmap_len(sbq_desc,
- maplen),
+ dma_unmap_addr(sbq_desc, mapaddr),
+ rx_ring->sbq_buf_size,
PCI_DMA_FROMDEVICE);
sbq_desc->p.skb = NULL;
}
@@ -1822,9 +1811,8 @@ static struct sk_buff *ql_build_rx_skb(struct ql_adapter *qdev,
return NULL;
}
pci_unmap_page(qdev->pdev,
- dma_unmap_addr(lbq_desc,
- mapaddr),
- dma_unmap_len(lbq_desc, maplen),
+ dma_unmap_addr(lbq_desc, mapaddr),
+ qdev->lbq_buf_size,
PCI_DMA_FROMDEVICE);
skb_reserve(skb, NET_IP_ALIGN);
netif_printk(qdev, rx_status, KERN_DEBUG, qdev->ndev,
@@ -1858,8 +1846,7 @@ static struct sk_buff *ql_build_rx_skb(struct ql_adapter *qdev,
sbq_desc = ql_get_curr_sbuf(rx_ring);
pci_unmap_single(qdev->pdev,
dma_unmap_addr(sbq_desc, mapaddr),
- dma_unmap_len(sbq_desc, maplen),
- PCI_DMA_FROMDEVICE);
+ rx_ring->sbq_buf_size, PCI_DMA_FROMDEVICE);
if (!(ib_mac_rsp->flags4 & IB_MAC_IOCB_RSP_HS)) {
/*
* This is an non TCP/UDP IP frame, so
@@ -2820,7 +2807,7 @@ static void ql_free_sbq_buffers(struct ql_adapter *qdev, struct rx_ring *rx_ring
if (sbq_desc->p.skb) {
pci_unmap_single(qdev->pdev,
dma_unmap_addr(sbq_desc, mapaddr),
- dma_unmap_len(sbq_desc, maplen),
+ rx_ring->sbq_buf_size,
PCI_DMA_FROMDEVICE);
dev_kfree_skb(sbq_desc->p.skb);
sbq_desc->p.skb = NULL;
--
2.21.0
^ permalink raw reply related [flat|nested] 46+ messages in thread* RE: [PATCH net-next 04/16] qlge: Remove bq_desc.maplen
2019-06-17 7:48 ` [PATCH net-next 04/16] qlge: Remove bq_desc.maplen Benjamin Poirier
@ 2019-06-26 9:31 ` Manish Chopra
0 siblings, 0 replies; 46+ messages in thread
From: Manish Chopra @ 2019-06-26 9:31 UTC (permalink / raw)
To: Benjamin Poirier, GR-Linux-NIC-Dev, netdev
> -----Original Message-----
> From: Benjamin Poirier <bpoirier@suse.com>
> Sent: Monday, June 17, 2019 1:19 PM
> To: Manish Chopra <manishc@marvell.com>; GR-Linux-NIC-Dev <GR-Linux-
> NIC-Dev@marvell.com>; netdev@vger.kernel.org
> Subject: [PATCH net-next 04/16] qlge: Remove bq_desc.maplen
>
> The size of the mapping is known statically in all cases, there's no need to save
> it at runtime. Remove this member.
>
> Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
> ---
> drivers/net/ethernet/qlogic/qlge/qlge.h | 1 -
> drivers/net/ethernet/qlogic/qlge/qlge_main.c | 43 +++++++-------------
> 2 files changed, 15 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/net/ethernet/qlogic/qlge/qlge.h
> b/drivers/net/ethernet/qlogic/qlge/qlge.h
> index ba61b4559dd6..f32da8c7679f 100644
> --- a/drivers/net/ethernet/qlogic/qlge/qlge.h
> +++ b/drivers/net/ethernet/qlogic/qlge/qlge.h
> @@ -1373,7 +1373,6 @@ struct bq_desc {
> __le64 *addr;
> u32 index;
> DEFINE_DMA_UNMAP_ADDR(mapaddr);
> - DEFINE_DMA_UNMAP_LEN(maplen);
> };
>
> #define QL_TXQ_IDX(qdev, skb) (smp_processor_id()%(qdev->tx_ring_count))
> diff --git a/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> index 9df06ad3fb93..25dbaa9cc55d 100644
> --- a/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> +++ b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> @@ -1108,8 +1108,6 @@ static void ql_update_lbq(struct ql_adapter *qdev,
> struct rx_ring *rx_ring)
> map = lbq_desc->p.pg_chunk.map +
> lbq_desc->p.pg_chunk.offset;
> dma_unmap_addr_set(lbq_desc, mapaddr, map);
> - dma_unmap_len_set(lbq_desc, maplen,
> - qdev->lbq_buf_size);
> *lbq_desc->addr = cpu_to_le64(map);
>
> pci_dma_sync_single_for_device(qdev->pdev, map,
> @@ -1177,8 +1175,6 @@ static void ql_update_sbq(struct ql_adapter *qdev,
> struct rx_ring *rx_ring)
> return;
> }
> dma_unmap_addr_set(sbq_desc, mapaddr,
> map);
> - dma_unmap_len_set(sbq_desc, maplen,
> - rx_ring->sbq_buf_size);
> *sbq_desc->addr = cpu_to_le64(map);
> }
>
> @@ -1598,14 +1594,14 @@ static void ql_process_mac_rx_skb(struct
> ql_adapter *qdev,
>
> pci_dma_sync_single_for_cpu(qdev->pdev,
> dma_unmap_addr(sbq_desc, mapaddr),
> - dma_unmap_len(sbq_desc, maplen),
> + rx_ring->sbq_buf_size,
> PCI_DMA_FROMDEVICE);
>
> skb_put_data(new_skb, skb->data, length);
>
> pci_dma_sync_single_for_device(qdev->pdev,
> dma_unmap_addr(sbq_desc, mapaddr),
> - dma_unmap_len(sbq_desc, maplen),
> + rx_ring->sbq_buf_size,
> PCI_DMA_FROMDEVICE);
> skb = new_skb;
>
> @@ -1727,8 +1723,7 @@ static struct sk_buff *ql_build_rx_skb(struct
> ql_adapter *qdev,
> sbq_desc = ql_get_curr_sbuf(rx_ring);
> pci_unmap_single(qdev->pdev,
> dma_unmap_addr(sbq_desc, mapaddr),
> - dma_unmap_len(sbq_desc, maplen),
> - PCI_DMA_FROMDEVICE);
> + rx_ring->sbq_buf_size,
> PCI_DMA_FROMDEVICE);
> skb = sbq_desc->p.skb;
> ql_realign_skb(skb, hdr_len);
> skb_put(skb, hdr_len);
> @@ -1758,19 +1753,15 @@ static struct sk_buff *ql_build_rx_skb(struct
> ql_adapter *qdev,
> */
> sbq_desc = ql_get_curr_sbuf(rx_ring);
> pci_dma_sync_single_for_cpu(qdev->pdev,
> - dma_unmap_addr
> - (sbq_desc, mapaddr),
> - dma_unmap_len
> - (sbq_desc, maplen),
> +
> dma_unmap_addr(sbq_desc,
> + mapaddr),
> + rx_ring->sbq_buf_size,
> PCI_DMA_FROMDEVICE);
> skb_put_data(skb, sbq_desc->p.skb->data, length);
> pci_dma_sync_single_for_device(qdev->pdev,
> - dma_unmap_addr
> - (sbq_desc,
> - mapaddr),
> - dma_unmap_len
> - (sbq_desc,
> - maplen),
> +
> dma_unmap_addr(sbq_desc,
> + mapaddr),
> + rx_ring->sbq_buf_size,
> PCI_DMA_FROMDEVICE);
> } else {
> netif_printk(qdev, rx_status, KERN_DEBUG, qdev-
> >ndev, @@ -1781,10 +1772,8 @@ static struct sk_buff *ql_build_rx_skb(struct
> ql_adapter *qdev,
> ql_realign_skb(skb, length);
> skb_put(skb, length);
> pci_unmap_single(qdev->pdev,
> - dma_unmap_addr(sbq_desc,
> - mapaddr),
> - dma_unmap_len(sbq_desc,
> - maplen),
> + dma_unmap_addr(sbq_desc,
> mapaddr),
> + rx_ring->sbq_buf_size,
> PCI_DMA_FROMDEVICE);
> sbq_desc->p.skb = NULL;
> }
> @@ -1822,9 +1811,8 @@ static struct sk_buff *ql_build_rx_skb(struct
> ql_adapter *qdev,
> return NULL;
> }
> pci_unmap_page(qdev->pdev,
> - dma_unmap_addr(lbq_desc,
> - mapaddr),
> - dma_unmap_len(lbq_desc, maplen),
> + dma_unmap_addr(lbq_desc, mapaddr),
> + qdev->lbq_buf_size,
> PCI_DMA_FROMDEVICE);
> skb_reserve(skb, NET_IP_ALIGN);
> netif_printk(qdev, rx_status, KERN_DEBUG, qdev-
> >ndev, @@ -1858,8 +1846,7 @@ static struct sk_buff *ql_build_rx_skb(struct
> ql_adapter *qdev,
> sbq_desc = ql_get_curr_sbuf(rx_ring);
> pci_unmap_single(qdev->pdev,
> dma_unmap_addr(sbq_desc, mapaddr),
> - dma_unmap_len(sbq_desc, maplen),
> - PCI_DMA_FROMDEVICE);
> + rx_ring->sbq_buf_size,
> PCI_DMA_FROMDEVICE);
> if (!(ib_mac_rsp->flags4 & IB_MAC_IOCB_RSP_HS)) {
> /*
> * This is an non TCP/UDP IP frame, so @@ -2820,7
> +2807,7 @@ static void ql_free_sbq_buffers(struct ql_adapter *qdev, struct
> rx_ring *rx_ring
> if (sbq_desc->p.skb) {
> pci_unmap_single(qdev->pdev,
> dma_unmap_addr(sbq_desc,
> mapaddr),
> - dma_unmap_len(sbq_desc, maplen),
> + rx_ring->sbq_buf_size,
> PCI_DMA_FROMDEVICE);
> dev_kfree_skb(sbq_desc->p.skb);
> sbq_desc->p.skb = NULL;
> --
> 2.21.0
Acked-by: Manish Chopra <manishc@marvell.com>
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH net-next 05/16] qlge: Remove rx_ring.sbq_buf_size
2019-06-17 7:48 [PATCH net-next 01/16] qlge: Remove irq_cnt Benjamin Poirier
` (2 preceding siblings ...)
2019-06-17 7:48 ` [PATCH net-next 04/16] qlge: Remove bq_desc.maplen Benjamin Poirier
@ 2019-06-17 7:48 ` Benjamin Poirier
2019-06-26 9:36 ` [EXT] " Manish Chopra
2019-06-17 7:48 ` [PATCH net-next 06/16] qlge: Remove useless dma synchronization calls Benjamin Poirier
` (13 subsequent siblings)
17 siblings, 1 reply; 46+ messages in thread
From: Benjamin Poirier @ 2019-06-17 7:48 UTC (permalink / raw)
To: Manish Chopra, GR-Linux-NIC-Dev, netdev
Tx rings have sbq_buf_size = 0 but there's no case where the code actually
tests on that value. We can remove sbq_buf_size and use a constant instead.
Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
---
drivers/net/ethernet/qlogic/qlge/qlge.h | 1 -
drivers/net/ethernet/qlogic/qlge/qlge_dbg.c | 1 -
drivers/net/ethernet/qlogic/qlge/qlge_main.c | 24 ++++++++------------
3 files changed, 10 insertions(+), 16 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qlge/qlge.h b/drivers/net/ethernet/qlogic/qlge/qlge.h
index f32da8c7679f..a3a52bbc2821 100644
--- a/drivers/net/ethernet/qlogic/qlge/qlge.h
+++ b/drivers/net/ethernet/qlogic/qlge/qlge.h
@@ -1447,7 +1447,6 @@ struct rx_ring {
/* Small buffer queue elements. */
u32 sbq_len; /* entry count */
u32 sbq_size; /* size in bytes of queue */
- u32 sbq_buf_size;
void *sbq_base;
dma_addr_t sbq_base_dma;
void *sbq_base_indirect;
diff --git a/drivers/net/ethernet/qlogic/qlge/qlge_dbg.c b/drivers/net/ethernet/qlogic/qlge/qlge_dbg.c
index 46599d74c6fb..cff1603d121c 100644
--- a/drivers/net/ethernet/qlogic/qlge/qlge_dbg.c
+++ b/drivers/net/ethernet/qlogic/qlge/qlge_dbg.c
@@ -1792,7 +1792,6 @@ void ql_dump_rx_ring(struct rx_ring *rx_ring)
pr_err("rx_ring->sbq_curr_idx = %d\n", rx_ring->sbq_curr_idx);
pr_err("rx_ring->sbq_clean_idx = %d\n", rx_ring->sbq_clean_idx);
pr_err("rx_ring->sbq_free_cnt = %d\n", rx_ring->sbq_free_cnt);
- pr_err("rx_ring->sbq_buf_size = %d\n", rx_ring->sbq_buf_size);
pr_err("rx_ring->cq_id = %d\n", rx_ring->cq_id);
pr_err("rx_ring->irq = %d\n", rx_ring->irq);
pr_err("rx_ring->cpu = %d\n", rx_ring->cpu);
diff --git a/drivers/net/ethernet/qlogic/qlge/qlge_main.c b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
index 25dbaa9cc55d..6b932bb6ce8f 100644
--- a/drivers/net/ethernet/qlogic/qlge/qlge_main.c
+++ b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
@@ -1164,7 +1164,7 @@ static void ql_update_sbq(struct ql_adapter *qdev, struct rx_ring *rx_ring)
skb_reserve(sbq_desc->p.skb, QLGE_SB_PAD);
map = pci_map_single(qdev->pdev,
sbq_desc->p.skb->data,
- rx_ring->sbq_buf_size,
+ SMALL_BUF_MAP_SIZE,
PCI_DMA_FROMDEVICE);
if (pci_dma_mapping_error(qdev->pdev, map)) {
netif_err(qdev, ifup, qdev->ndev,
@@ -1594,14 +1594,13 @@ static void ql_process_mac_rx_skb(struct ql_adapter *qdev,
pci_dma_sync_single_for_cpu(qdev->pdev,
dma_unmap_addr(sbq_desc, mapaddr),
- rx_ring->sbq_buf_size,
- PCI_DMA_FROMDEVICE);
+ SMALL_BUF_MAP_SIZE, PCI_DMA_FROMDEVICE);
skb_put_data(new_skb, skb->data, length);
pci_dma_sync_single_for_device(qdev->pdev,
dma_unmap_addr(sbq_desc, mapaddr),
- rx_ring->sbq_buf_size,
+ SMALL_BUF_MAP_SIZE,
PCI_DMA_FROMDEVICE);
skb = new_skb;
@@ -1723,7 +1722,7 @@ static struct sk_buff *ql_build_rx_skb(struct ql_adapter *qdev,
sbq_desc = ql_get_curr_sbuf(rx_ring);
pci_unmap_single(qdev->pdev,
dma_unmap_addr(sbq_desc, mapaddr),
- rx_ring->sbq_buf_size, PCI_DMA_FROMDEVICE);
+ SMALL_BUF_MAP_SIZE, PCI_DMA_FROMDEVICE);
skb = sbq_desc->p.skb;
ql_realign_skb(skb, hdr_len);
skb_put(skb, hdr_len);
@@ -1755,13 +1754,13 @@ static struct sk_buff *ql_build_rx_skb(struct ql_adapter *qdev,
pci_dma_sync_single_for_cpu(qdev->pdev,
dma_unmap_addr(sbq_desc,
mapaddr),
- rx_ring->sbq_buf_size,
+ SMALL_BUF_MAP_SIZE,
PCI_DMA_FROMDEVICE);
skb_put_data(skb, sbq_desc->p.skb->data, length);
pci_dma_sync_single_for_device(qdev->pdev,
dma_unmap_addr(sbq_desc,
mapaddr),
- rx_ring->sbq_buf_size,
+ SMALL_BUF_MAP_SIZE,
PCI_DMA_FROMDEVICE);
} else {
netif_printk(qdev, rx_status, KERN_DEBUG, qdev->ndev,
@@ -1773,7 +1772,7 @@ static struct sk_buff *ql_build_rx_skb(struct ql_adapter *qdev,
skb_put(skb, length);
pci_unmap_single(qdev->pdev,
dma_unmap_addr(sbq_desc, mapaddr),
- rx_ring->sbq_buf_size,
+ SMALL_BUF_MAP_SIZE,
PCI_DMA_FROMDEVICE);
sbq_desc->p.skb = NULL;
}
@@ -1846,7 +1845,7 @@ static struct sk_buff *ql_build_rx_skb(struct ql_adapter *qdev,
sbq_desc = ql_get_curr_sbuf(rx_ring);
pci_unmap_single(qdev->pdev,
dma_unmap_addr(sbq_desc, mapaddr),
- rx_ring->sbq_buf_size, PCI_DMA_FROMDEVICE);
+ SMALL_BUF_MAP_SIZE, PCI_DMA_FROMDEVICE);
if (!(ib_mac_rsp->flags4 & IB_MAC_IOCB_RSP_HS)) {
/*
* This is an non TCP/UDP IP frame, so
@@ -2807,7 +2806,7 @@ static void ql_free_sbq_buffers(struct ql_adapter *qdev, struct rx_ring *rx_ring
if (sbq_desc->p.skb) {
pci_unmap_single(qdev->pdev,
dma_unmap_addr(sbq_desc, mapaddr),
- rx_ring->sbq_buf_size,
+ SMALL_BUF_MAP_SIZE,
PCI_DMA_FROMDEVICE);
dev_kfree_skb(sbq_desc->p.skb);
sbq_desc->p.skb = NULL;
@@ -3158,8 +3157,7 @@ static int ql_start_rx_ring(struct ql_adapter *qdev, struct rx_ring *rx_ring)
} while (page_entries < MAX_DB_PAGES_PER_BQ(rx_ring->sbq_len));
cqicb->sbq_addr =
cpu_to_le64(rx_ring->sbq_base_indirect_dma);
- cqicb->sbq_buf_size =
- cpu_to_le16((u16)(rx_ring->sbq_buf_size));
+ cqicb->sbq_buf_size = cpu_to_le16(SMALL_BUF_MAP_SIZE);
bq_len = (rx_ring->sbq_len == 65536) ? 0 :
(u16) rx_ring->sbq_len;
cqicb->sbq_len = cpu_to_le16(bq_len);
@@ -4098,7 +4096,6 @@ static int ql_configure_rings(struct ql_adapter *qdev)
rx_ring->sbq_len = NUM_SMALL_BUFFERS;
rx_ring->sbq_size =
rx_ring->sbq_len * sizeof(__le64);
- rx_ring->sbq_buf_size = SMALL_BUF_MAP_SIZE;
rx_ring->type = RX_Q;
} else {
/*
@@ -4112,7 +4109,6 @@ static int ql_configure_rings(struct ql_adapter *qdev)
rx_ring->lbq_size = 0;
rx_ring->sbq_len = 0;
rx_ring->sbq_size = 0;
- rx_ring->sbq_buf_size = 0;
rx_ring->type = TX_Q;
}
}
--
2.21.0
^ permalink raw reply related [flat|nested] 46+ messages in thread* RE: [EXT] [PATCH net-next 05/16] qlge: Remove rx_ring.sbq_buf_size
2019-06-17 7:48 ` [PATCH net-next 05/16] qlge: Remove rx_ring.sbq_buf_size Benjamin Poirier
@ 2019-06-26 9:36 ` Manish Chopra
2019-06-26 11:39 ` Benjamin Poirier
0 siblings, 1 reply; 46+ messages in thread
From: Manish Chopra @ 2019-06-26 9:36 UTC (permalink / raw)
To: Benjamin Poirier, GR-Linux-NIC-Dev, netdev
> -----Original Message-----
> From: Benjamin Poirier <bpoirier@suse.com>
> Sent: Monday, June 17, 2019 1:19 PM
> To: Manish Chopra <manishc@marvell.com>; GR-Linux-NIC-Dev <GR-Linux-
> NIC-Dev@marvell.com>; netdev@vger.kernel.org
> Subject: [EXT] [PATCH net-next 05/16] qlge: Remove rx_ring.sbq_buf_size
>
> External Email
>
> ----------------------------------------------------------------------
> Tx rings have sbq_buf_size = 0 but there's no case where the code actually
> tests on that value. We can remove sbq_buf_size and use a constant instead.
>
Seems relevant to RX ring, not the TX ring ?
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [EXT] [PATCH net-next 05/16] qlge: Remove rx_ring.sbq_buf_size
2019-06-26 9:36 ` [EXT] " Manish Chopra
@ 2019-06-26 11:39 ` Benjamin Poirier
2019-06-26 15:35 ` Willem de Bruijn
0 siblings, 1 reply; 46+ messages in thread
From: Benjamin Poirier @ 2019-06-26 11:39 UTC (permalink / raw)
To: Manish Chopra; +Cc: GR-Linux-NIC-Dev, netdev
On 2019/06/26 09:36, Manish Chopra wrote:
> > -----Original Message-----
> > From: Benjamin Poirier <bpoirier@suse.com>
> > Sent: Monday, June 17, 2019 1:19 PM
> > To: Manish Chopra <manishc@marvell.com>; GR-Linux-NIC-Dev <GR-Linux-
> > NIC-Dev@marvell.com>; netdev@vger.kernel.org
> > Subject: [EXT] [PATCH net-next 05/16] qlge: Remove rx_ring.sbq_buf_size
> >
> > External Email
> >
> > ----------------------------------------------------------------------
> > Tx rings have sbq_buf_size = 0 but there's no case where the code actually
> > tests on that value. We can remove sbq_buf_size and use a constant instead.
> >
>
> Seems relevant to RX ring, not the TX ring ?
qlge uses "struct rx_ring" for rx and for tx completion rings.
The driver's author is probably laughing now at the success of his plan
to confuse those who would follow in his footsteps.
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [EXT] [PATCH net-next 05/16] qlge: Remove rx_ring.sbq_buf_size
2019-06-26 11:39 ` Benjamin Poirier
@ 2019-06-26 15:35 ` Willem de Bruijn
0 siblings, 0 replies; 46+ messages in thread
From: Willem de Bruijn @ 2019-06-26 15:35 UTC (permalink / raw)
To: Benjamin Poirier; +Cc: Manish Chopra, GR-Linux-NIC-Dev, netdev
On Wed, Jun 26, 2019 at 7:40 AM Benjamin Poirier <bpoirier@suse.com> wrote:
>
> On 2019/06/26 09:36, Manish Chopra wrote:
> > > -----Original Message-----
> > > From: Benjamin Poirier <bpoirier@suse.com>
> > > Sent: Monday, June 17, 2019 1:19 PM
> > > To: Manish Chopra <manishc@marvell.com>; GR-Linux-NIC-Dev <GR-Linux-
> > > NIC-Dev@marvell.com>; netdev@vger.kernel.org
> > > Subject: [EXT] [PATCH net-next 05/16] qlge: Remove rx_ring.sbq_buf_size
> > >
> > > External Email
> > >
> > > ----------------------------------------------------------------------
> > > Tx rings have sbq_buf_size = 0 but there's no case where the code actually
> > > tests on that value. We can remove sbq_buf_size and use a constant instead.
> > >
> >
> > Seems relevant to RX ring, not the TX ring ?
>
> qlge uses "struct rx_ring" for rx and for tx completion rings.
>
> The driver's author is probably laughing now at the success of his plan
> to confuse those who would follow in his footsteps.
:-)
Reviewed-by: Willem de Bruijn <willemb@google.com>
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH net-next 06/16] qlge: Remove useless dma synchronization calls
2019-06-17 7:48 [PATCH net-next 01/16] qlge: Remove irq_cnt Benjamin Poirier
` (3 preceding siblings ...)
2019-06-17 7:48 ` [PATCH net-next 05/16] qlge: Remove rx_ring.sbq_buf_size Benjamin Poirier
@ 2019-06-17 7:48 ` Benjamin Poirier
2019-06-17 9:44 ` [EXT] " Manish Chopra
2019-06-17 7:48 ` [PATCH net-next 07/16] qlge: Deduplicate rx buffer queue management Benjamin Poirier
` (12 subsequent siblings)
17 siblings, 1 reply; 46+ messages in thread
From: Benjamin Poirier @ 2019-06-17 7:48 UTC (permalink / raw)
To: Manish Chopra, GR-Linux-NIC-Dev, netdev
This is unneeded for two reasons:
1) the mapping is not written by the cpu
2) calls like ..._sync_..._for_device(..., ..._FROMDEVICE) are
nonsensical, see commit 3f0fb4e85b38 ("Documentation/DMA-API-HOWTO.txt:
fix misleading example")
Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
---
drivers/net/ethernet/qlogic/qlge/qlge_main.c | 12 ------------
1 file changed, 12 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qlge/qlge_main.c b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
index 6b932bb6ce8f..70a284857488 100644
--- a/drivers/net/ethernet/qlogic/qlge/qlge_main.c
+++ b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
@@ -1110,9 +1110,6 @@ static void ql_update_lbq(struct ql_adapter *qdev, struct rx_ring *rx_ring)
dma_unmap_addr_set(lbq_desc, mapaddr, map);
*lbq_desc->addr = cpu_to_le64(map);
- pci_dma_sync_single_for_device(qdev->pdev, map,
- qdev->lbq_buf_size,
- PCI_DMA_FROMDEVICE);
clean_idx++;
if (clean_idx == rx_ring->lbq_len)
clean_idx = 0;
@@ -1598,10 +1595,6 @@ static void ql_process_mac_rx_skb(struct ql_adapter *qdev,
skb_put_data(new_skb, skb->data, length);
- pci_dma_sync_single_for_device(qdev->pdev,
- dma_unmap_addr(sbq_desc, mapaddr),
- SMALL_BUF_MAP_SIZE,
- PCI_DMA_FROMDEVICE);
skb = new_skb;
/* Frame error, so drop the packet. */
@@ -1757,11 +1750,6 @@ static struct sk_buff *ql_build_rx_skb(struct ql_adapter *qdev,
SMALL_BUF_MAP_SIZE,
PCI_DMA_FROMDEVICE);
skb_put_data(skb, sbq_desc->p.skb->data, length);
- pci_dma_sync_single_for_device(qdev->pdev,
- dma_unmap_addr(sbq_desc,
- mapaddr),
- SMALL_BUF_MAP_SIZE,
- PCI_DMA_FROMDEVICE);
} else {
netif_printk(qdev, rx_status, KERN_DEBUG, qdev->ndev,
"%d bytes in a single small buffer.\n",
--
2.21.0
^ permalink raw reply related [flat|nested] 46+ messages in thread* RE: [EXT] [PATCH net-next 06/16] qlge: Remove useless dma synchronization calls
2019-06-17 7:48 ` [PATCH net-next 06/16] qlge: Remove useless dma synchronization calls Benjamin Poirier
@ 2019-06-17 9:44 ` Manish Chopra
2019-06-18 2:51 ` Benjamin Poirier
0 siblings, 1 reply; 46+ messages in thread
From: Manish Chopra @ 2019-06-17 9:44 UTC (permalink / raw)
To: Benjamin Poirier, GR-Linux-NIC-Dev, netdev
> -----Original Message-----
> From: Benjamin Poirier <bpoirier@suse.com>
> Sent: Monday, June 17, 2019 1:19 PM
> To: Manish Chopra <manishc@marvell.com>; GR-Linux-NIC-Dev <GR-Linux-
> NIC-Dev@marvell.com>; netdev@vger.kernel.org
> Subject: [EXT] [PATCH net-next 06/16] qlge: Remove useless dma
> synchronization calls
>
> External Email
>
> ----------------------------------------------------------------------
> This is unneeded for two reasons:
> 1) the mapping is not written by the cpu
> 2) calls like ..._sync_..._for_device(..., ..._FROMDEVICE) are
> nonsensical, see commit 3f0fb4e85b38 ("Documentation/DMA-API-
> HOWTO.txt:
> fix misleading example")
>
> Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
> ---
> drivers/net/ethernet/qlogic/qlge/qlge_main.c | 12 ------------
> 1 file changed, 12 deletions(-)
>
> diff --git a/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> index 6b932bb6ce8f..70a284857488 100644
> --- a/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> +++ b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> @@ -1110,9 +1110,6 @@ static void ql_update_lbq(struct ql_adapter *qdev,
> struct rx_ring *rx_ring)
> dma_unmap_addr_set(lbq_desc, mapaddr, map);
> *lbq_desc->addr = cpu_to_le64(map);
>
> - pci_dma_sync_single_for_device(qdev->pdev, map,
> - qdev->lbq_buf_size,
> - PCI_DMA_FROMDEVICE);
> clean_idx++;
> if (clean_idx == rx_ring->lbq_len)
> clean_idx = 0;
> @@ -1598,10 +1595,6 @@ static void ql_process_mac_rx_skb(struct
> ql_adapter *qdev,
>
> skb_put_data(new_skb, skb->data, length);
>
> - pci_dma_sync_single_for_device(qdev->pdev,
> - dma_unmap_addr(sbq_desc, mapaddr),
> - SMALL_BUF_MAP_SIZE,
> - PCI_DMA_FROMDEVICE);
This was introduced in commit 2c9a266afefe ("qlge: Fix receive packets drop").
So hoping that it is fine, the buffer shouldn't be synced for the device back after the synced for CPU call in context of any ownership etc. ?
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [EXT] [PATCH net-next 06/16] qlge: Remove useless dma synchronization calls
2019-06-17 9:44 ` [EXT] " Manish Chopra
@ 2019-06-18 2:51 ` Benjamin Poirier
0 siblings, 0 replies; 46+ messages in thread
From: Benjamin Poirier @ 2019-06-18 2:51 UTC (permalink / raw)
To: Manish Chopra; +Cc: GR-Linux-NIC-Dev, netdev
On 2019/06/17 09:44, Manish Chopra wrote:
[...]
> > --- a/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> > +++ b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> > @@ -1110,9 +1110,6 @@ static void ql_update_lbq(struct ql_adapter *qdev,
> > struct rx_ring *rx_ring)
> > dma_unmap_addr_set(lbq_desc, mapaddr, map);
> > *lbq_desc->addr = cpu_to_le64(map);
> >
> > - pci_dma_sync_single_for_device(qdev->pdev, map,
> > - qdev->lbq_buf_size,
> > - PCI_DMA_FROMDEVICE);
> > clean_idx++;
> > if (clean_idx == rx_ring->lbq_len)
> > clean_idx = 0;
> > @@ -1598,10 +1595,6 @@ static void ql_process_mac_rx_skb(struct
> > ql_adapter *qdev,
> >
> > skb_put_data(new_skb, skb->data, length);
> >
> > - pci_dma_sync_single_for_device(qdev->pdev,
> > - dma_unmap_addr(sbq_desc, mapaddr),
> > - SMALL_BUF_MAP_SIZE,
> > - PCI_DMA_FROMDEVICE);
>
> This was introduced in commit 2c9a266afefe ("qlge: Fix receive packets drop").
> So hoping that it is fine, the buffer shouldn't be synced for the device back after the synced for CPU call in context of any ownership etc. ?
No, dma_sync_*_for_cpu() and dma_sync_*_for_device() calls don't have to
be paired; they are not like lock acquire and release calls.
In the cases the current patch is concerned with, the cpu does not write
any data for the device in the rx buffers. Therefore, there is no need
for those pci_dma_sync_single_for_device() calls.
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH net-next 07/16] qlge: Deduplicate rx buffer queue management
2019-06-17 7:48 [PATCH net-next 01/16] qlge: Remove irq_cnt Benjamin Poirier
` (4 preceding siblings ...)
2019-06-17 7:48 ` [PATCH net-next 06/16] qlge: Remove useless dma synchronization calls Benjamin Poirier
@ 2019-06-17 7:48 ` Benjamin Poirier
2019-06-27 10:02 ` [EXT] " Manish Chopra
2019-06-17 7:48 ` [PATCH net-next 08/16] qlge: Fix dma_sync_single calls Benjamin Poirier
` (11 subsequent siblings)
17 siblings, 1 reply; 46+ messages in thread
From: Benjamin Poirier @ 2019-06-17 7:48 UTC (permalink / raw)
To: Manish Chopra, GR-Linux-NIC-Dev, netdev
The qlge driver (and device) uses two kinds of buffers for reception,
so-called "small buffers" and "large buffers". The two are arranged in
rings, the sbq and lbq. These two share similar data structures and code.
Factor out data structures into a common struct qlge_bq, make required
adjustments to code and dedup the most obvious cases of copy/paste.
This patch should not introduce any functional change other than to some of
the printk format strings.
Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
---
drivers/net/ethernet/qlogic/qlge/qlge.h | 96 ++--
drivers/net/ethernet/qlogic/qlge/qlge_dbg.c | 60 +-
drivers/net/ethernet/qlogic/qlge/qlge_main.c | 570 ++++++++-----------
3 files changed, 333 insertions(+), 393 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qlge/qlge.h b/drivers/net/ethernet/qlogic/qlge/qlge.h
index a3a52bbc2821..a84aa264dfa8 100644
--- a/drivers/net/ethernet/qlogic/qlge/qlge.h
+++ b/drivers/net/ethernet/qlogic/qlge/qlge.h
@@ -1358,23 +1358,6 @@ struct tx_ring_desc {
struct tx_ring_desc *next;
};
-struct page_chunk {
- struct page *page; /* master page */
- char *va; /* virt addr for this chunk */
- u64 map; /* mapping for master */
- unsigned int offset; /* offset for this chunk */
-};
-
-struct bq_desc {
- union {
- struct page_chunk pg_chunk;
- struct sk_buff *skb;
- } p;
- __le64 *addr;
- u32 index;
- DEFINE_DMA_UNMAP_ADDR(mapaddr);
-};
-
#define QL_TXQ_IDX(qdev, skb) (smp_processor_id()%(qdev->tx_ring_count))
struct tx_ring {
@@ -1413,6 +1396,56 @@ enum {
RX_Q = 4, /* Handles inbound completions. */
};
+struct qlge_page_chunk {
+ struct page *page;
+ void *va; /* virt addr including offset */
+ unsigned int offset;
+};
+
+struct qlge_bq_desc {
+ union {
+ /* for large buffers */
+ struct qlge_page_chunk pg_chunk;
+ /* for small buffers */
+ struct sk_buff *skb;
+ } p;
+ dma_addr_t dma_addr;
+ /* address in ring where the buffer address (dma_addr) is written for
+ * the device
+ */
+ __le64 *buf_ptr;
+ u32 index;
+ DEFINE_DMA_UNMAP_ADDR(mapaddr);
+};
+
+/* buffer queue */
+struct qlge_bq {
+ __le64 *base;
+ dma_addr_t base_dma;
+ __le64 *base_indirect;
+ dma_addr_t base_indirect_dma;
+ struct qlge_bq_desc *queue;
+ void __iomem *prod_idx_db_reg;
+ u32 len; /* entry count */
+ u32 size; /* size in bytes of hw ring */
+ u32 prod_idx; /* current sw prod idx */
+ u32 curr_idx; /* next entry we expect */
+ u32 clean_idx; /* beginning of new descs */
+ u32 free_cnt; /* free buffer desc cnt */
+ enum {
+ QLGE_SB, /* small buffer */
+ QLGE_LB, /* large buffer */
+ } type;
+};
+
+#define QLGE_BQ_CONTAINER(bq) \
+({ \
+ typeof(bq) _bq = bq; \
+ (struct rx_ring *)((char *)_bq - (_bq->type == QLGE_SB ? \
+ offsetof(struct rx_ring, sbq) : \
+ offsetof(struct rx_ring, lbq))); \
+})
+
struct rx_ring {
struct cqicb cqicb; /* The chip's completion queue init control block. */
@@ -1430,33 +1463,12 @@ struct rx_ring {
void __iomem *valid_db_reg; /* PCI doorbell mem area + 0x04 */
/* Large buffer queue elements. */
- u32 lbq_len; /* entry count */
- u32 lbq_size; /* size in bytes of queue */
- void *lbq_base;
- dma_addr_t lbq_base_dma;
- void *lbq_base_indirect;
- dma_addr_t lbq_base_indirect_dma;
- struct page_chunk pg_chunk; /* current page for chunks */
- struct bq_desc *lbq; /* array of control blocks */
- void __iomem *lbq_prod_idx_db_reg; /* PCI doorbell mem area + 0x18 */
- u32 lbq_prod_idx; /* current sw prod idx */
- u32 lbq_curr_idx; /* next entry we expect */
- u32 lbq_clean_idx; /* beginning of new descs */
- u32 lbq_free_cnt; /* free buffer desc cnt */
+ struct qlge_bq lbq;
+ struct qlge_page_chunk master_chunk;
+ dma_addr_t chunk_dma_addr;
/* Small buffer queue elements. */
- u32 sbq_len; /* entry count */
- u32 sbq_size; /* size in bytes of queue */
- void *sbq_base;
- dma_addr_t sbq_base_dma;
- void *sbq_base_indirect;
- dma_addr_t sbq_base_indirect_dma;
- struct bq_desc *sbq; /* array of control blocks */
- void __iomem *sbq_prod_idx_db_reg; /* PCI doorbell mem area + 0x1c */
- u32 sbq_prod_idx; /* current sw prod idx */
- u32 sbq_curr_idx; /* next entry we expect */
- u32 sbq_clean_idx; /* beginning of new descs */
- u32 sbq_free_cnt; /* free buffer desc cnt */
+ struct qlge_bq sbq;
/* Misc. handler elements. */
u32 type; /* Type of queue, tx, rx. */
diff --git a/drivers/net/ethernet/qlogic/qlge/qlge_dbg.c b/drivers/net/ethernet/qlogic/qlge/qlge_dbg.c
index cff1603d121c..35af06dd21dd 100644
--- a/drivers/net/ethernet/qlogic/qlge/qlge_dbg.c
+++ b/drivers/net/ethernet/qlogic/qlge/qlge_dbg.c
@@ -1759,39 +1759,39 @@ void ql_dump_rx_ring(struct rx_ring *rx_ring)
pr_err("rx_ring->curr_entry = %p\n", rx_ring->curr_entry);
pr_err("rx_ring->valid_db_reg = %p\n", rx_ring->valid_db_reg);
- pr_err("rx_ring->lbq_base = %p\n", rx_ring->lbq_base);
- pr_err("rx_ring->lbq_base_dma = %llx\n",
- (unsigned long long) rx_ring->lbq_base_dma);
- pr_err("rx_ring->lbq_base_indirect = %p\n",
- rx_ring->lbq_base_indirect);
- pr_err("rx_ring->lbq_base_indirect_dma = %llx\n",
- (unsigned long long) rx_ring->lbq_base_indirect_dma);
- pr_err("rx_ring->lbq = %p\n", rx_ring->lbq);
- pr_err("rx_ring->lbq_len = %d\n", rx_ring->lbq_len);
- pr_err("rx_ring->lbq_size = %d\n", rx_ring->lbq_size);
- pr_err("rx_ring->lbq_prod_idx_db_reg = %p\n",
- rx_ring->lbq_prod_idx_db_reg);
- pr_err("rx_ring->lbq_prod_idx = %d\n", rx_ring->lbq_prod_idx);
- pr_err("rx_ring->lbq_curr_idx = %d\n", rx_ring->lbq_curr_idx);
+ pr_err("rx_ring->lbq.base = %p\n", rx_ring->lbq.base);
+ pr_err("rx_ring->lbq.base_dma = %llx\n",
+ (unsigned long long)rx_ring->lbq.base_dma);
+ pr_err("rx_ring->lbq.base_indirect = %p\n",
+ rx_ring->lbq.base_indirect);
+ pr_err("rx_ring->lbq.base_indirect_dma = %llx\n",
+ (unsigned long long)rx_ring->lbq.base_indirect_dma);
+ pr_err("rx_ring->lbq = %p\n", rx_ring->lbq.queue);
+ pr_err("rx_ring->lbq.len = %d\n", rx_ring->lbq.len);
+ pr_err("rx_ring->lbq.size = %d\n", rx_ring->lbq.size);
+ pr_err("rx_ring->lbq.prod_idx_db_reg = %p\n",
+ rx_ring->lbq.prod_idx_db_reg);
+ pr_err("rx_ring->lbq.prod_idx = %d\n", rx_ring->lbq.prod_idx);
+ pr_err("rx_ring->lbq.curr_idx = %d\n", rx_ring->lbq.curr_idx);
pr_err("rx_ring->lbq_clean_idx = %d\n", rx_ring->lbq_clean_idx);
pr_err("rx_ring->lbq_free_cnt = %d\n", rx_ring->lbq_free_cnt);
- pr_err("rx_ring->sbq_base = %p\n", rx_ring->sbq_base);
- pr_err("rx_ring->sbq_base_dma = %llx\n",
- (unsigned long long) rx_ring->sbq_base_dma);
- pr_err("rx_ring->sbq_base_indirect = %p\n",
- rx_ring->sbq_base_indirect);
- pr_err("rx_ring->sbq_base_indirect_dma = %llx\n",
- (unsigned long long) rx_ring->sbq_base_indirect_dma);
- pr_err("rx_ring->sbq = %p\n", rx_ring->sbq);
- pr_err("rx_ring->sbq_len = %d\n", rx_ring->sbq_len);
- pr_err("rx_ring->sbq_size = %d\n", rx_ring->sbq_size);
- pr_err("rx_ring->sbq_prod_idx_db_reg addr = %p\n",
- rx_ring->sbq_prod_idx_db_reg);
- pr_err("rx_ring->sbq_prod_idx = %d\n", rx_ring->sbq_prod_idx);
- pr_err("rx_ring->sbq_curr_idx = %d\n", rx_ring->sbq_curr_idx);
- pr_err("rx_ring->sbq_clean_idx = %d\n", rx_ring->sbq_clean_idx);
- pr_err("rx_ring->sbq_free_cnt = %d\n", rx_ring->sbq_free_cnt);
+ pr_err("rx_ring->sbq.base = %p\n", rx_ring->sbq.base);
+ pr_err("rx_ring->sbq.base_dma = %llx\n",
+ (unsigned long long)rx_ring->sbq.base_dma);
+ pr_err("rx_ring->sbq.base_indirect = %p\n",
+ rx_ring->sbq.base_indirect);
+ pr_err("rx_ring->sbq.base_indirect_dma = %llx\n",
+ (unsigned long long)rx_ring->sbq.base_indirect_dma);
+ pr_err("rx_ring->sbq = %p\n", rx_ring->sbq.queue);
+ pr_err("rx_ring->sbq.len = %d\n", rx_ring->sbq.len);
+ pr_err("rx_ring->sbq.size = %d\n", rx_ring->sbq.size);
+ pr_err("rx_ring->sbq.prod_idx_db_reg addr = %p\n",
+ rx_ring->sbq.prod_idx_db_reg);
+ pr_err("rx_ring->sbq.prod_idx = %d\n", rx_ring->sbq.prod_idx);
+ pr_err("rx_ring->sbq.curr_idx = %d\n", rx_ring->sbq.curr_idx);
+ pr_err("rx_ring->sbq.clean_idx = %d\n", rx_ring->sbq.clean_idx);
+ pr_err("rx_ring->sbq.free_cnt = %d\n", rx_ring->sbq.free_cnt);
pr_err("rx_ring->cq_id = %d\n", rx_ring->cq_id);
pr_err("rx_ring->irq = %d\n", rx_ring->irq);
pr_err("rx_ring->cpu = %d\n", rx_ring->cpu);
diff --git a/drivers/net/ethernet/qlogic/qlge/qlge_main.c b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
index 70a284857488..e661ee2730e5 100644
--- a/drivers/net/ethernet/qlogic/qlge/qlge_main.c
+++ b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
@@ -978,47 +978,35 @@ static inline unsigned int ql_lbq_block_size(struct ql_adapter *qdev)
return PAGE_SIZE << qdev->lbq_buf_order;
}
-/* Get the next large buffer. */
-static struct bq_desc *ql_get_curr_lbuf(struct rx_ring *rx_ring)
-{
- struct bq_desc *lbq_desc = &rx_ring->lbq[rx_ring->lbq_curr_idx];
- rx_ring->lbq_curr_idx++;
- if (rx_ring->lbq_curr_idx == rx_ring->lbq_len)
- rx_ring->lbq_curr_idx = 0;
- rx_ring->lbq_free_cnt++;
- return lbq_desc;
+static struct qlge_bq_desc *qlge_get_curr_buf(struct qlge_bq *bq)
+{
+ struct qlge_bq_desc *bq_desc;
+
+ bq_desc = &bq->queue[bq->curr_idx++];
+ if (bq->curr_idx == bq->len)
+ bq->curr_idx = 0;
+ bq->free_cnt++;
+
+ return bq_desc;
}
-static struct bq_desc *ql_get_curr_lchunk(struct ql_adapter *qdev,
- struct rx_ring *rx_ring)
+static struct qlge_bq_desc *ql_get_curr_lchunk(struct ql_adapter *qdev,
+ struct rx_ring *rx_ring)
{
- struct bq_desc *lbq_desc = ql_get_curr_lbuf(rx_ring);
+ struct qlge_bq_desc *lbq_desc = qlge_get_curr_buf(&rx_ring->lbq);
pci_dma_sync_single_for_cpu(qdev->pdev,
dma_unmap_addr(lbq_desc, mapaddr),
qdev->lbq_buf_size, PCI_DMA_FROMDEVICE);
- /* If it's the last chunk of our master page then
- * we unmap it.
- */
- if (lbq_desc->p.pg_chunk.offset + qdev->lbq_buf_size ==
- ql_lbq_block_size(qdev))
- pci_unmap_page(qdev->pdev,
- lbq_desc->p.pg_chunk.map,
- ql_lbq_block_size(qdev),
- PCI_DMA_FROMDEVICE);
- return lbq_desc;
-}
+ if ((lbq_desc->p.pg_chunk.offset + qdev->lbq_buf_size) ==
+ ql_lbq_block_size(qdev)) {
+ /* last chunk of the master page */
+ pci_unmap_page(qdev->pdev, lbq_desc->dma_addr,
+ ql_lbq_block_size(qdev), PCI_DMA_FROMDEVICE);
+ }
-/* Get the next small buffer. */
-static struct bq_desc *ql_get_curr_sbuf(struct rx_ring *rx_ring)
-{
- struct bq_desc *sbq_desc = &rx_ring->sbq[rx_ring->sbq_curr_idx];
- rx_ring->sbq_curr_idx++;
- if (rx_ring->sbq_curr_idx == rx_ring->sbq_len)
- rx_ring->sbq_curr_idx = 0;
- rx_ring->sbq_free_cnt++;
- return sbq_desc;
+ return lbq_desc;
}
/* Update an rx ring index. */
@@ -1037,169 +1025,159 @@ static void ql_write_cq_idx(struct rx_ring *rx_ring)
ql_write_db_reg(rx_ring->cnsmr_idx, rx_ring->cnsmr_idx_db_reg);
}
-static int ql_get_next_chunk(struct ql_adapter *qdev, struct rx_ring *rx_ring,
- struct bq_desc *lbq_desc)
+static const char * const bq_type_name[] = {
+ [QLGE_SB] = "sbq",
+ [QLGE_LB] = "lbq",
+};
+
+/* return size of allocated buffer (may be 0) or negative error */
+static int qlge_refill_sb(struct rx_ring *rx_ring,
+ struct qlge_bq_desc *sbq_desc)
{
- if (!rx_ring->pg_chunk.page) {
- u64 map;
- rx_ring->pg_chunk.page = alloc_pages(__GFP_COMP | GFP_ATOMIC,
- qdev->lbq_buf_order);
- if (unlikely(!rx_ring->pg_chunk.page)) {
- netif_err(qdev, drv, qdev->ndev,
- "page allocation failed.\n");
+ struct ql_adapter *qdev = rx_ring->qdev;
+ struct sk_buff *skb;
+
+ if (sbq_desc->p.skb)
+ return 0;
+
+ netif_printk(qdev, rx_status, KERN_DEBUG, qdev->ndev,
+ "ring %u sbq: getting new skb for index %d.\n",
+ rx_ring->cq_id, sbq_desc->index);
+
+ skb = netdev_alloc_skb(qdev->ndev, SMALL_BUFFER_SIZE);
+ if (!skb)
+ return -ENOMEM;
+ skb_reserve(skb, QLGE_SB_PAD);
+
+ sbq_desc->dma_addr = pci_map_single(qdev->pdev, skb->data,
+ SMALL_BUF_MAP_SIZE,
+ PCI_DMA_FROMDEVICE);
+ if (pci_dma_mapping_error(qdev->pdev, sbq_desc->dma_addr)) {
+ netif_err(qdev, ifup, qdev->ndev, "PCI mapping failed.\n");
+ dev_kfree_skb_any(skb);
+ return -EIO;
+ }
+
+ sbq_desc->p.skb = skb;
+ return SMALL_BUFFER_SIZE;
+}
+
+/* return size of allocated buffer or negative error */
+static int qlge_refill_lb(struct rx_ring *rx_ring,
+ struct qlge_bq_desc *lbq_desc)
+{
+ struct ql_adapter *qdev = rx_ring->qdev;
+ struct qlge_page_chunk *master_chunk = &rx_ring->master_chunk;
+
+ if (!master_chunk->page) {
+ struct page *page;
+ dma_addr_t dma_addr;
+
+ page = alloc_pages(__GFP_COMP | GFP_ATOMIC,
+ qdev->lbq_buf_order);
+ if (unlikely(!page))
return -ENOMEM;
- }
- rx_ring->pg_chunk.offset = 0;
- map = pci_map_page(qdev->pdev, rx_ring->pg_chunk.page,
- 0, ql_lbq_block_size(qdev),
+ dma_addr = pci_map_page(qdev->pdev, page, 0,
+ ql_lbq_block_size(qdev),
PCI_DMA_FROMDEVICE);
- if (pci_dma_mapping_error(qdev->pdev, map)) {
- __free_pages(rx_ring->pg_chunk.page,
- qdev->lbq_buf_order);
- rx_ring->pg_chunk.page = NULL;
+ if (pci_dma_mapping_error(qdev->pdev, dma_addr)) {
+ __free_pages(page, qdev->lbq_buf_order);
netif_err(qdev, drv, qdev->ndev,
"PCI mapping failed.\n");
- return -ENOMEM;
+ return -EIO;
}
- rx_ring->pg_chunk.map = map;
- rx_ring->pg_chunk.va = page_address(rx_ring->pg_chunk.page);
+ master_chunk->page = page;
+ master_chunk->va = page_address(page);
+ master_chunk->offset = 0;
+ rx_ring->chunk_dma_addr = dma_addr;
}
- /* Copy the current master pg_chunk info
- * to the current descriptor.
- */
- lbq_desc->p.pg_chunk = rx_ring->pg_chunk;
+ lbq_desc->p.pg_chunk = *master_chunk;
+ lbq_desc->dma_addr = rx_ring->chunk_dma_addr + master_chunk->offset;
/* Adjust the master page chunk for next
* buffer get.
*/
- rx_ring->pg_chunk.offset += qdev->lbq_buf_size;
- if (rx_ring->pg_chunk.offset == ql_lbq_block_size(qdev)) {
- rx_ring->pg_chunk.page = NULL;
+ master_chunk->offset += qdev->lbq_buf_size;
+ if (master_chunk->offset == ql_lbq_block_size(qdev)) {
+ master_chunk->page = NULL;
} else {
- rx_ring->pg_chunk.va += qdev->lbq_buf_size;
- get_page(rx_ring->pg_chunk.page);
+ master_chunk->va += qdev->lbq_buf_size;
+ get_page(master_chunk->page);
}
- return 0;
+
+ return qdev->lbq_buf_size;
}
-/* Process (refill) a large buffer queue. */
-static void ql_update_lbq(struct ql_adapter *qdev, struct rx_ring *rx_ring)
+
+static void qlge_refill_bq(struct qlge_bq *bq)
{
- u32 clean_idx = rx_ring->lbq_clean_idx;
+ struct rx_ring *rx_ring = QLGE_BQ_CONTAINER(bq);
+ struct ql_adapter *qdev = rx_ring->qdev;
+ u32 clean_idx = bq->clean_idx;
+ unsigned int reserved_count;
u32 start_idx = clean_idx;
- struct bq_desc *lbq_desc;
- u64 map;
int i;
- while (rx_ring->lbq_free_cnt > 32) {
- for (i = (rx_ring->lbq_clean_idx % 16); i < 16; i++) {
+ if (bq->type == QLGE_SB)
+ reserved_count = 16;
+ else
+ reserved_count = 32;
+
+ while (bq->free_cnt > reserved_count) {
+ for (i = (bq->clean_idx % 16); i < 16; i++) {
+ struct qlge_bq_desc *bq_desc = &bq->queue[clean_idx];
+ int retval;
+
netif_printk(qdev, rx_status, KERN_DEBUG, qdev->ndev,
- "lbq: try cleaning clean_idx = %d.\n",
+ "ring %u %s: try cleaning clean_idx = %d.\n",
+ rx_ring->cq_id, bq_type_name[bq->type],
clean_idx);
- lbq_desc = &rx_ring->lbq[clean_idx];
- if (ql_get_next_chunk(qdev, rx_ring, lbq_desc)) {
- rx_ring->lbq_clean_idx = clean_idx;
+
+ if (bq->type == QLGE_SB)
+ retval = qlge_refill_sb(rx_ring, bq_desc);
+ else
+ retval = qlge_refill_lb(rx_ring, bq_desc);
+
+ if (retval > 0) {
+ dma_unmap_addr_set(bq_desc, mapaddr,
+ bq_desc->dma_addr);
+ *bq_desc->buf_ptr =
+ cpu_to_le64(bq_desc->dma_addr);
+ } else if (retval < 0) {
+ bq->clean_idx = clean_idx;
netif_err(qdev, ifup, qdev->ndev,
- "Could not get a page chunk, i=%d, clean_idx =%d .\n",
- i, clean_idx);
+ "ring %u %s: Could not get a page chunk, i=%d, clean_idx =%d .\n",
+ rx_ring->cq_id,
+ bq_type_name[bq->type], i,
+ clean_idx);
return;
}
- map = lbq_desc->p.pg_chunk.map +
- lbq_desc->p.pg_chunk.offset;
- dma_unmap_addr_set(lbq_desc, mapaddr, map);
- *lbq_desc->addr = cpu_to_le64(map);
-
clean_idx++;
- if (clean_idx == rx_ring->lbq_len)
+ if (clean_idx == bq->len)
clean_idx = 0;
}
- rx_ring->lbq_clean_idx = clean_idx;
- rx_ring->lbq_prod_idx += 16;
- if (rx_ring->lbq_prod_idx == rx_ring->lbq_len)
- rx_ring->lbq_prod_idx = 0;
- rx_ring->lbq_free_cnt -= 16;
- }
-
- if (start_idx != clean_idx) {
- netif_printk(qdev, rx_status, KERN_DEBUG, qdev->ndev,
- "lbq: updating prod idx = %d.\n",
- rx_ring->lbq_prod_idx);
- ql_write_db_reg(rx_ring->lbq_prod_idx,
- rx_ring->lbq_prod_idx_db_reg);
- }
-}
-
-/* Process (refill) a small buffer queue. */
-static void ql_update_sbq(struct ql_adapter *qdev, struct rx_ring *rx_ring)
-{
- u32 clean_idx = rx_ring->sbq_clean_idx;
- u32 start_idx = clean_idx;
- struct bq_desc *sbq_desc;
- u64 map;
- int i;
-
- while (rx_ring->sbq_free_cnt > 16) {
- for (i = (rx_ring->sbq_clean_idx % 16); i < 16; i++) {
- sbq_desc = &rx_ring->sbq[clean_idx];
- netif_printk(qdev, rx_status, KERN_DEBUG, qdev->ndev,
- "sbq: try cleaning clean_idx = %d.\n",
- clean_idx);
- if (sbq_desc->p.skb == NULL) {
- netif_printk(qdev, rx_status, KERN_DEBUG,
- qdev->ndev,
- "sbq: getting new skb for index %d.\n",
- sbq_desc->index);
- sbq_desc->p.skb =
- netdev_alloc_skb(qdev->ndev,
- SMALL_BUFFER_SIZE);
- if (sbq_desc->p.skb == NULL) {
- rx_ring->sbq_clean_idx = clean_idx;
- return;
- }
- skb_reserve(sbq_desc->p.skb, QLGE_SB_PAD);
- map = pci_map_single(qdev->pdev,
- sbq_desc->p.skb->data,
- SMALL_BUF_MAP_SIZE,
- PCI_DMA_FROMDEVICE);
- if (pci_dma_mapping_error(qdev->pdev, map)) {
- netif_err(qdev, ifup, qdev->ndev,
- "PCI mapping failed.\n");
- rx_ring->sbq_clean_idx = clean_idx;
- dev_kfree_skb_any(sbq_desc->p.skb);
- sbq_desc->p.skb = NULL;
- return;
- }
- dma_unmap_addr_set(sbq_desc, mapaddr, map);
- *sbq_desc->addr = cpu_to_le64(map);
- }
-
- clean_idx++;
- if (clean_idx == rx_ring->sbq_len)
- clean_idx = 0;
- }
- rx_ring->sbq_clean_idx = clean_idx;
- rx_ring->sbq_prod_idx += 16;
- if (rx_ring->sbq_prod_idx == rx_ring->sbq_len)
- rx_ring->sbq_prod_idx = 0;
- rx_ring->sbq_free_cnt -= 16;
+ bq->clean_idx = clean_idx;
+ bq->prod_idx += 16;
+ if (bq->prod_idx == bq->len)
+ bq->prod_idx = 0;
+ bq->free_cnt -= 16;
}
if (start_idx != clean_idx) {
netif_printk(qdev, rx_status, KERN_DEBUG, qdev->ndev,
- "sbq: updating prod idx = %d.\n",
- rx_ring->sbq_prod_idx);
- ql_write_db_reg(rx_ring->sbq_prod_idx,
- rx_ring->sbq_prod_idx_db_reg);
+ "ring %u %s: updating prod idx = %d.\n",
+ rx_ring->cq_id, bq_type_name[bq->type],
+ bq->prod_idx);
+ ql_write_db_reg(bq->prod_idx, bq->prod_idx_db_reg);
}
}
-static void ql_update_buffer_queues(struct ql_adapter *qdev,
- struct rx_ring *rx_ring)
+static void ql_update_buffer_queues(struct rx_ring *rx_ring)
{
- ql_update_sbq(qdev, rx_ring);
- ql_update_lbq(qdev, rx_ring);
+ qlge_refill_bq(&rx_ring->sbq);
+ qlge_refill_bq(&rx_ring->lbq);
}
/* Unmaps tx buffers. Can be called from send() if a pci mapping
@@ -1436,7 +1414,7 @@ static void ql_process_mac_rx_gro_page(struct ql_adapter *qdev,
u16 vlan_id)
{
struct sk_buff *skb;
- struct bq_desc *lbq_desc = ql_get_curr_lchunk(qdev, rx_ring);
+ struct qlge_bq_desc *lbq_desc = ql_get_curr_lchunk(qdev, rx_ring);
struct napi_struct *napi = &rx_ring->napi;
/* Frame error, so drop the packet. */
@@ -1485,7 +1463,7 @@ static void ql_process_mac_rx_page(struct ql_adapter *qdev,
struct net_device *ndev = qdev->ndev;
struct sk_buff *skb = NULL;
void *addr;
- struct bq_desc *lbq_desc = ql_get_curr_lchunk(qdev, rx_ring);
+ struct qlge_bq_desc *lbq_desc = ql_get_curr_lchunk(qdev, rx_ring);
struct napi_struct *napi = &rx_ring->napi;
size_t hlen = ETH_HLEN;
@@ -1575,10 +1553,9 @@ static void ql_process_mac_rx_skb(struct ql_adapter *qdev,
u32 length,
u16 vlan_id)
{
+ struct qlge_bq_desc *sbq_desc = qlge_get_curr_buf(&rx_ring->sbq);
struct net_device *ndev = qdev->ndev;
- struct sk_buff *skb = NULL;
- struct sk_buff *new_skb = NULL;
- struct bq_desc *sbq_desc = ql_get_curr_sbuf(rx_ring);
+ struct sk_buff *skb, *new_skb;
skb = sbq_desc->p.skb;
/* Allocate new_skb and copy */
@@ -1695,11 +1672,10 @@ static struct sk_buff *ql_build_rx_skb(struct ql_adapter *qdev,
struct rx_ring *rx_ring,
struct ib_mac_iocb_rsp *ib_mac_rsp)
{
- struct bq_desc *lbq_desc;
- struct bq_desc *sbq_desc;
- struct sk_buff *skb = NULL;
u32 length = le32_to_cpu(ib_mac_rsp->data_len);
u32 hdr_len = le32_to_cpu(ib_mac_rsp->hdr_len);
+ struct qlge_bq_desc *lbq_desc, *sbq_desc;
+ struct sk_buff *skb = NULL;
size_t hlen = ETH_HLEN;
/*
@@ -1712,7 +1688,7 @@ static struct sk_buff *ql_build_rx_skb(struct ql_adapter *qdev,
/*
* Headers fit nicely into a small buffer.
*/
- sbq_desc = ql_get_curr_sbuf(rx_ring);
+ sbq_desc = qlge_get_curr_buf(&rx_ring->sbq);
pci_unmap_single(qdev->pdev,
dma_unmap_addr(sbq_desc, mapaddr),
SMALL_BUF_MAP_SIZE, PCI_DMA_FROMDEVICE);
@@ -1743,7 +1719,7 @@ static struct sk_buff *ql_build_rx_skb(struct ql_adapter *qdev,
* from the "data" small buffer to the "header" small
* buffer.
*/
- sbq_desc = ql_get_curr_sbuf(rx_ring);
+ sbq_desc = qlge_get_curr_buf(&rx_ring->sbq);
pci_dma_sync_single_for_cpu(qdev->pdev,
dma_unmap_addr(sbq_desc,
mapaddr),
@@ -1754,7 +1730,7 @@ static struct sk_buff *ql_build_rx_skb(struct ql_adapter *qdev,
netif_printk(qdev, rx_status, KERN_DEBUG, qdev->ndev,
"%d bytes in a single small buffer.\n",
length);
- sbq_desc = ql_get_curr_sbuf(rx_ring);
+ sbq_desc = qlge_get_curr_buf(&rx_ring->sbq);
skb = sbq_desc->p.skb;
ql_realign_skb(skb, length);
skb_put(skb, length);
@@ -1830,7 +1806,7 @@ static struct sk_buff *ql_build_rx_skb(struct ql_adapter *qdev,
* eventually be in trouble.
*/
int size, i = 0;
- sbq_desc = ql_get_curr_sbuf(rx_ring);
+ sbq_desc = qlge_get_curr_buf(&rx_ring->sbq);
pci_unmap_single(qdev->pdev,
dma_unmap_addr(sbq_desc, mapaddr),
SMALL_BUF_MAP_SIZE, PCI_DMA_FROMDEVICE);
@@ -2207,7 +2183,7 @@ static int ql_clean_inbound_rx_ring(struct rx_ring *rx_ring, int budget)
if (count == budget)
break;
}
- ql_update_buffer_queues(qdev, rx_ring);
+ ql_update_buffer_queues(rx_ring);
ql_write_cq_idx(rx_ring);
return count;
}
@@ -2749,43 +2725,42 @@ static int ql_alloc_tx_resources(struct ql_adapter *qdev,
static void ql_free_lbq_buffers(struct ql_adapter *qdev, struct rx_ring *rx_ring)
{
unsigned int last_offset;
- struct bq_desc *lbq_desc;
uint32_t curr_idx, clean_idx;
last_offset = ql_lbq_block_size(qdev) - qdev->lbq_buf_size;
- curr_idx = rx_ring->lbq_curr_idx;
- clean_idx = rx_ring->lbq_clean_idx;
+ curr_idx = rx_ring->lbq.curr_idx;
+ clean_idx = rx_ring->lbq.clean_idx;
while (curr_idx != clean_idx) {
- lbq_desc = &rx_ring->lbq[curr_idx];
+ struct qlge_bq_desc *lbq_desc = &rx_ring->lbq.queue[curr_idx];
if (lbq_desc->p.pg_chunk.offset == last_offset)
- pci_unmap_page(qdev->pdev, lbq_desc->p.pg_chunk.map,
+ pci_unmap_page(qdev->pdev, lbq_desc->dma_addr,
ql_lbq_block_size(qdev),
PCI_DMA_FROMDEVICE);
put_page(lbq_desc->p.pg_chunk.page);
lbq_desc->p.pg_chunk.page = NULL;
- if (++curr_idx == rx_ring->lbq_len)
+ if (++curr_idx == rx_ring->lbq.len)
curr_idx = 0;
-
}
- if (rx_ring->pg_chunk.page) {
- pci_unmap_page(qdev->pdev, rx_ring->pg_chunk.map,
- ql_lbq_block_size(qdev), PCI_DMA_FROMDEVICE);
- put_page(rx_ring->pg_chunk.page);
- rx_ring->pg_chunk.page = NULL;
+
+ if (rx_ring->master_chunk.page) {
+ pci_unmap_page(qdev->pdev, rx_ring->chunk_dma_addr,
+ ql_lbq_block_size(qdev), PCI_DMA_FROMDEVICE);
+ put_page(rx_ring->master_chunk.page);
+ rx_ring->master_chunk.page = NULL;
}
}
static void ql_free_sbq_buffers(struct ql_adapter *qdev, struct rx_ring *rx_ring)
{
int i;
- struct bq_desc *sbq_desc;
- for (i = 0; i < rx_ring->sbq_len; i++) {
- sbq_desc = &rx_ring->sbq[i];
+ for (i = 0; i < rx_ring->sbq.len; i++) {
+ struct qlge_bq_desc *sbq_desc = &rx_ring->sbq.queue[i];
+
if (sbq_desc == NULL) {
netif_err(qdev, ifup, qdev->ndev,
"sbq_desc %d is NULL.\n", i);
@@ -2808,13 +2783,13 @@ static void ql_free_sbq_buffers(struct ql_adapter *qdev, struct rx_ring *rx_ring
static void ql_free_rx_buffers(struct ql_adapter *qdev)
{
int i;
- struct rx_ring *rx_ring;
for (i = 0; i < qdev->rx_ring_count; i++) {
- rx_ring = &qdev->rx_ring[i];
- if (rx_ring->lbq)
+ struct rx_ring *rx_ring = &qdev->rx_ring[i];
+
+ if (rx_ring->lbq.queue)
ql_free_lbq_buffers(qdev, rx_ring);
- if (rx_ring->sbq)
+ if (rx_ring->sbq.queue)
ql_free_sbq_buffers(qdev, rx_ring);
}
}
@@ -2827,70 +2802,70 @@ static void ql_alloc_rx_buffers(struct ql_adapter *qdev)
for (i = 0; i < qdev->rx_ring_count; i++) {
rx_ring = &qdev->rx_ring[i];
if (rx_ring->type != TX_Q)
- ql_update_buffer_queues(qdev, rx_ring);
+ ql_update_buffer_queues(rx_ring);
}
}
-static void ql_init_lbq_ring(struct ql_adapter *qdev,
- struct rx_ring *rx_ring)
+static int qlge_init_bq(struct qlge_bq *bq)
{
+ struct rx_ring *rx_ring = QLGE_BQ_CONTAINER(bq);
+ struct ql_adapter *qdev = rx_ring->qdev;
+ struct qlge_bq_desc *bq_desc;
+ __le64 *buf_ptr;
int i;
- struct bq_desc *lbq_desc;
- __le64 *bq = rx_ring->lbq_base;
- memset(rx_ring->lbq, 0, rx_ring->lbq_len * sizeof(struct bq_desc));
- for (i = 0; i < rx_ring->lbq_len; i++) {
- lbq_desc = &rx_ring->lbq[i];
- memset(lbq_desc, 0, sizeof(*lbq_desc));
- lbq_desc->index = i;
- lbq_desc->addr = bq;
- bq++;
+ bq->base = pci_alloc_consistent(qdev->pdev, bq->size, &bq->base_dma);
+ if (!bq->base) {
+ netif_err(qdev, ifup, qdev->ndev,
+ "ring %u %s allocation failed.\n", rx_ring->cq_id,
+ bq_type_name[bq->type]);
+ return -ENOMEM;
}
-}
-static void ql_init_sbq_ring(struct ql_adapter *qdev,
- struct rx_ring *rx_ring)
-{
- int i;
- struct bq_desc *sbq_desc;
- __le64 *bq = rx_ring->sbq_base;
+ bq->queue = kmalloc_array(bq->len, sizeof(struct qlge_bq_desc),
+ GFP_KERNEL);
+ if (!bq->queue)
+ return -ENOMEM;
- memset(rx_ring->sbq, 0, rx_ring->sbq_len * sizeof(struct bq_desc));
- for (i = 0; i < rx_ring->sbq_len; i++) {
- sbq_desc = &rx_ring->sbq[i];
- memset(sbq_desc, 0, sizeof(*sbq_desc));
- sbq_desc->index = i;
- sbq_desc->addr = bq;
- bq++;
+ memset(bq->queue, 0, bq->len * sizeof(struct qlge_bq_desc));
+
+ buf_ptr = bq->base;
+ bq_desc = &bq->queue[0];
+ for (i = 0; i < bq->len; i++, buf_ptr++, bq_desc++) {
+ memset(bq_desc, 0, sizeof(*bq_desc));
+ bq_desc->index = i;
+ bq_desc->buf_ptr = buf_ptr;
}
+
+ return 0;
}
static void ql_free_rx_resources(struct ql_adapter *qdev,
struct rx_ring *rx_ring)
{
/* Free the small buffer queue. */
- if (rx_ring->sbq_base) {
+ if (rx_ring->sbq.base) {
pci_free_consistent(qdev->pdev,
- rx_ring->sbq_size,
- rx_ring->sbq_base, rx_ring->sbq_base_dma);
- rx_ring->sbq_base = NULL;
+ rx_ring->sbq.size,
+ rx_ring->sbq.base, rx_ring->sbq.base_dma);
+ rx_ring->sbq.base = NULL;
}
/* Free the small buffer queue control blocks. */
- kfree(rx_ring->sbq);
- rx_ring->sbq = NULL;
+ kfree(rx_ring->sbq.queue);
+ rx_ring->sbq.queue = NULL;
/* Free the large buffer queue. */
- if (rx_ring->lbq_base) {
+ if (rx_ring->lbq.base) {
pci_free_consistent(qdev->pdev,
- rx_ring->lbq_size,
- rx_ring->lbq_base, rx_ring->lbq_base_dma);
- rx_ring->lbq_base = NULL;
+ rx_ring->lbq.size,
+ rx_ring->lbq.base, rx_ring->lbq.base_dma);
+ rx_ring->lbq.base = NULL;
}
/* Free the large buffer queue control blocks. */
- kfree(rx_ring->lbq);
- rx_ring->lbq = NULL;
+ kfree(rx_ring->lbq.queue);
+ rx_ring->lbq.queue = NULL;
/* Free the rx queue. */
if (rx_ring->cq_base) {
@@ -2919,56 +2894,10 @@ static int ql_alloc_rx_resources(struct ql_adapter *qdev,
return -ENOMEM;
}
- if (rx_ring->sbq_len) {
- /*
- * Allocate small buffer queue.
- */
- rx_ring->sbq_base =
- pci_alloc_consistent(qdev->pdev, rx_ring->sbq_size,
- &rx_ring->sbq_base_dma);
-
- if (rx_ring->sbq_base == NULL) {
- netif_err(qdev, ifup, qdev->ndev,
- "Small buffer queue allocation failed.\n");
- goto err_mem;
- }
-
- /*
- * Allocate small buffer queue control blocks.
- */
- rx_ring->sbq = kmalloc_array(rx_ring->sbq_len,
- sizeof(struct bq_desc),
- GFP_KERNEL);
- if (rx_ring->sbq == NULL)
- goto err_mem;
-
- ql_init_sbq_ring(qdev, rx_ring);
- }
-
- if (rx_ring->lbq_len) {
- /*
- * Allocate large buffer queue.
- */
- rx_ring->lbq_base =
- pci_alloc_consistent(qdev->pdev, rx_ring->lbq_size,
- &rx_ring->lbq_base_dma);
-
- if (rx_ring->lbq_base == NULL) {
- netif_err(qdev, ifup, qdev->ndev,
- "Large buffer queue allocation failed.\n");
- goto err_mem;
- }
- /*
- * Allocate large buffer queue control blocks.
- */
- rx_ring->lbq = kmalloc_array(rx_ring->lbq_len,
- sizeof(struct bq_desc),
- GFP_KERNEL);
- if (rx_ring->lbq == NULL)
- goto err_mem;
-
- ql_init_lbq_ring(qdev, rx_ring);
- }
+ if (rx_ring->sbq.len && qlge_init_bq(&rx_ring->sbq))
+ goto err_mem;
+ if (rx_ring->lbq.len && qlge_init_bq(&rx_ring->lbq))
+ goto err_mem;
return 0;
@@ -3071,12 +3000,12 @@ static int ql_start_rx_ring(struct ql_adapter *qdev, struct rx_ring *rx_ring)
*rx_ring->prod_idx_sh_reg = 0;
shadow_reg += sizeof(u64);
shadow_reg_dma += sizeof(u64);
- rx_ring->lbq_base_indirect = shadow_reg;
- rx_ring->lbq_base_indirect_dma = shadow_reg_dma;
- shadow_reg += (sizeof(u64) * MAX_DB_PAGES_PER_BQ(rx_ring->lbq_len));
- shadow_reg_dma += (sizeof(u64) * MAX_DB_PAGES_PER_BQ(rx_ring->lbq_len));
- rx_ring->sbq_base_indirect = shadow_reg;
- rx_ring->sbq_base_indirect_dma = shadow_reg_dma;
+ rx_ring->lbq.base_indirect = shadow_reg;
+ rx_ring->lbq.base_indirect_dma = shadow_reg_dma;
+ shadow_reg += (sizeof(u64) * MAX_DB_PAGES_PER_BQ(rx_ring->lbq.len));
+ shadow_reg_dma += (sizeof(u64) * MAX_DB_PAGES_PER_BQ(rx_ring->lbq.len));
+ rx_ring->sbq.base_indirect = shadow_reg;
+ rx_ring->sbq.base_indirect_dma = shadow_reg_dma;
/* PCI doorbell mem area + 0x00 for consumer index register */
rx_ring->cnsmr_idx_db_reg = (u32 __iomem *) doorbell_area;
@@ -3087,10 +3016,10 @@ static int ql_start_rx_ring(struct ql_adapter *qdev, struct rx_ring *rx_ring)
rx_ring->valid_db_reg = doorbell_area + 0x04;
/* PCI doorbell mem area + 0x18 for large buffer consumer */
- rx_ring->lbq_prod_idx_db_reg = (u32 __iomem *) (doorbell_area + 0x18);
+ rx_ring->lbq.prod_idx_db_reg = (u32 __iomem *)(doorbell_area + 0x18);
/* PCI doorbell mem area + 0x1c */
- rx_ring->sbq_prod_idx_db_reg = (u32 __iomem *) (doorbell_area + 0x1c);
+ rx_ring->sbq.prod_idx_db_reg = (u32 __iomem *)(doorbell_area + 0x1c);
memset((void *)cqicb, 0, sizeof(struct cqicb));
cqicb->msix_vect = rx_ring->irq;
@@ -3108,51 +3037,50 @@ static int ql_start_rx_ring(struct ql_adapter *qdev, struct rx_ring *rx_ring)
cqicb->flags = FLAGS_LC | /* Load queue base address */
FLAGS_LV | /* Load MSI-X vector */
FLAGS_LI; /* Load irq delay values */
- if (rx_ring->lbq_len) {
+ if (rx_ring->lbq.len) {
cqicb->flags |= FLAGS_LL; /* Load lbq values */
- tmp = (u64)rx_ring->lbq_base_dma;
- base_indirect_ptr = rx_ring->lbq_base_indirect;
+ tmp = (u64)rx_ring->lbq.base_dma;
+ base_indirect_ptr = rx_ring->lbq.base_indirect;
page_entries = 0;
do {
*base_indirect_ptr = cpu_to_le64(tmp);
tmp += DB_PAGE_SIZE;
base_indirect_ptr++;
page_entries++;
- } while (page_entries < MAX_DB_PAGES_PER_BQ(rx_ring->lbq_len));
- cqicb->lbq_addr =
- cpu_to_le64(rx_ring->lbq_base_indirect_dma);
- bq_len = (qdev->lbq_buf_size == 65536) ? 0 :
+ } while (page_entries < MAX_DB_PAGES_PER_BQ(rx_ring->lbq.len));
+ cqicb->lbq_addr = cpu_to_le64(rx_ring->lbq.base_indirect_dma);
+ bq_len = qdev->lbq_buf_size == 65536 ? 0 :
(u16)qdev->lbq_buf_size;
cqicb->lbq_buf_size = cpu_to_le16(bq_len);
- bq_len = (rx_ring->lbq_len == 65536) ? 0 :
- (u16) rx_ring->lbq_len;
+ bq_len = (rx_ring->lbq.len == 65536) ? 0 :
+ (u16)rx_ring->lbq.len;
cqicb->lbq_len = cpu_to_le16(bq_len);
- rx_ring->lbq_prod_idx = 0;
- rx_ring->lbq_curr_idx = 0;
- rx_ring->lbq_clean_idx = 0;
- rx_ring->lbq_free_cnt = rx_ring->lbq_len;
+ rx_ring->lbq.prod_idx = 0;
+ rx_ring->lbq.curr_idx = 0;
+ rx_ring->lbq.clean_idx = 0;
+ rx_ring->lbq.free_cnt = rx_ring->lbq.len;
}
- if (rx_ring->sbq_len) {
+ if (rx_ring->sbq.len) {
cqicb->flags |= FLAGS_LS; /* Load sbq values */
- tmp = (u64)rx_ring->sbq_base_dma;
- base_indirect_ptr = rx_ring->sbq_base_indirect;
+ tmp = (u64)rx_ring->sbq.base_dma;
+ base_indirect_ptr = rx_ring->sbq.base_indirect;
page_entries = 0;
do {
*base_indirect_ptr = cpu_to_le64(tmp);
tmp += DB_PAGE_SIZE;
base_indirect_ptr++;
page_entries++;
- } while (page_entries < MAX_DB_PAGES_PER_BQ(rx_ring->sbq_len));
+ } while (page_entries < MAX_DB_PAGES_PER_BQ(rx_ring->sbq.len));
cqicb->sbq_addr =
- cpu_to_le64(rx_ring->sbq_base_indirect_dma);
- cqicb->sbq_buf_size = cpu_to_le16(SMALL_BUF_MAP_SIZE);
- bq_len = (rx_ring->sbq_len == 65536) ? 0 :
- (u16) rx_ring->sbq_len;
+ cpu_to_le64(rx_ring->sbq.base_indirect_dma);
+ cqicb->sbq_buf_size = cpu_to_le16(SMALL_BUFFER_SIZE);
+ bq_len = (rx_ring->sbq.len == 65536) ? 0 :
+ (u16)rx_ring->sbq.len;
cqicb->sbq_len = cpu_to_le16(bq_len);
- rx_ring->sbq_prod_idx = 0;
- rx_ring->sbq_curr_idx = 0;
- rx_ring->sbq_clean_idx = 0;
- rx_ring->sbq_free_cnt = rx_ring->sbq_len;
+ rx_ring->sbq.prod_idx = 0;
+ rx_ring->sbq.curr_idx = 0;
+ rx_ring->sbq.clean_idx = 0;
+ rx_ring->sbq.free_cnt = rx_ring->sbq.len;
}
switch (rx_ring->type) {
case TX_Q:
@@ -4078,12 +4006,12 @@ static int ql_configure_rings(struct ql_adapter *qdev)
rx_ring->cq_len = qdev->rx_ring_size;
rx_ring->cq_size =
rx_ring->cq_len * sizeof(struct ql_net_rsp_iocb);
- rx_ring->lbq_len = NUM_LARGE_BUFFERS;
- rx_ring->lbq_size =
- rx_ring->lbq_len * sizeof(__le64);
- rx_ring->sbq_len = NUM_SMALL_BUFFERS;
- rx_ring->sbq_size =
- rx_ring->sbq_len * sizeof(__le64);
+ rx_ring->lbq.type = QLGE_LB;
+ rx_ring->lbq.len = NUM_LARGE_BUFFERS;
+ rx_ring->lbq.size = rx_ring->lbq.len * sizeof(__le64);
+ rx_ring->sbq.type = QLGE_SB;
+ rx_ring->sbq.len = NUM_SMALL_BUFFERS;
+ rx_ring->sbq.size = rx_ring->sbq.len * sizeof(__le64);
rx_ring->type = RX_Q;
} else {
/*
@@ -4093,10 +4021,10 @@ static int ql_configure_rings(struct ql_adapter *qdev)
rx_ring->cq_len = qdev->tx_ring_size;
rx_ring->cq_size =
rx_ring->cq_len * sizeof(struct ql_net_rsp_iocb);
- rx_ring->lbq_len = 0;
- rx_ring->lbq_size = 0;
- rx_ring->sbq_len = 0;
- rx_ring->sbq_size = 0;
+ rx_ring->lbq.len = 0;
+ rx_ring->lbq.size = 0;
+ rx_ring->sbq.len = 0;
+ rx_ring->sbq.size = 0;
rx_ring->type = TX_Q;
}
}
--
2.21.0
^ permalink raw reply related [flat|nested] 46+ messages in thread* RE: [EXT] [PATCH net-next 07/16] qlge: Deduplicate rx buffer queue management
2019-06-17 7:48 ` [PATCH net-next 07/16] qlge: Deduplicate rx buffer queue management Benjamin Poirier
@ 2019-06-27 10:02 ` Manish Chopra
2019-07-09 2:10 ` Benjamin Poirier
0 siblings, 1 reply; 46+ messages in thread
From: Manish Chopra @ 2019-06-27 10:02 UTC (permalink / raw)
To: Benjamin Poirier, GR-Linux-NIC-Dev, netdev
> while (curr_idx != clean_idx) {
> - lbq_desc = &rx_ring->lbq[curr_idx];
> + struct qlge_bq_desc *lbq_desc = &rx_ring-
> >lbq.queue[curr_idx];
>
> if (lbq_desc->p.pg_chunk.offset == last_offset)
> - pci_unmap_page(qdev->pdev, lbq_desc-
> >p.pg_chunk.map,
> + pci_unmap_page(qdev->pdev, lbq_desc->dma_addr,
> ql_lbq_block_size(qdev),
> PCI_DMA_FROMDEVICE);
In this patch, lbq_desc->dma_addr points to offset in the page. So unmapping is broken within this patch.
Would have been nicer to fix this in the same patch although it might have been taken care in next patches probably.
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [EXT] [PATCH net-next 07/16] qlge: Deduplicate rx buffer queue management
2019-06-27 10:02 ` [EXT] " Manish Chopra
@ 2019-07-09 2:10 ` Benjamin Poirier
0 siblings, 0 replies; 46+ messages in thread
From: Benjamin Poirier @ 2019-07-09 2:10 UTC (permalink / raw)
To: Manish Chopra; +Cc: GR-Linux-NIC-Dev, netdev
On 2019/06/27 10:02, Manish Chopra wrote:
> > while (curr_idx != clean_idx) {
> > - lbq_desc = &rx_ring->lbq[curr_idx];
> > + struct qlge_bq_desc *lbq_desc = &rx_ring-
> > >lbq.queue[curr_idx];
> >
> > if (lbq_desc->p.pg_chunk.offset == last_offset)
> > - pci_unmap_page(qdev->pdev, lbq_desc-
> > >p.pg_chunk.map,
> > + pci_unmap_page(qdev->pdev, lbq_desc->dma_addr,
> > ql_lbq_block_size(qdev),
> > PCI_DMA_FROMDEVICE);
>
> In this patch, lbq_desc->dma_addr points to offset in the page. So unmapping is broken within this patch.
> Would have been nicer to fix this in the same patch although it might have been taken care in next patches probably.
>
Indeed, thanks. The same applies in ql_get_curr_lchunk().
Replaced with the following for v2:
+ pci_unmap_page(qdev->pdev, lbq_desc->dma_addr -
+ last_offset, ql_lbq_block_size(qdev),
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH net-next 08/16] qlge: Fix dma_sync_single calls
2019-06-17 7:48 [PATCH net-next 01/16] qlge: Remove irq_cnt Benjamin Poirier
` (5 preceding siblings ...)
2019-06-17 7:48 ` [PATCH net-next 07/16] qlge: Deduplicate rx buffer queue management Benjamin Poirier
@ 2019-06-17 7:48 ` Benjamin Poirier
2019-06-17 7:48 ` [PATCH net-next 09/16] qlge: Remove rx_ring.type Benjamin Poirier
` (10 subsequent siblings)
17 siblings, 0 replies; 46+ messages in thread
From: Benjamin Poirier @ 2019-06-17 7:48 UTC (permalink / raw)
To: Manish Chopra, GR-Linux-NIC-Dev, netdev
Using the unmap addr elsewhere than unmap calls is a misuse of the dma api.
In prevision of this fix, qlge kept two copies of the dma address around ;)
Fixes: c4e84bde1d59 ("qlge: New Qlogic 10Gb Ethernet Driver.")
Fixes: 7c734359d350 ("qlge: Size RX buffers based on MTU.")
Fixes: 2c9a266afefe ("qlge: Fix receive packets drop.")
Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
---
drivers/net/ethernet/qlogic/qlge/qlge.h | 5 +--
drivers/net/ethernet/qlogic/qlge/qlge_main.c | 47 ++++++++------------
2 files changed, 19 insertions(+), 33 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qlge/qlge.h b/drivers/net/ethernet/qlogic/qlge/qlge.h
index a84aa264dfa8..519fa39dd194 100644
--- a/drivers/net/ethernet/qlogic/qlge/qlge.h
+++ b/drivers/net/ethernet/qlogic/qlge/qlge.h
@@ -1410,12 +1410,9 @@ struct qlge_bq_desc {
struct sk_buff *skb;
} p;
dma_addr_t dma_addr;
- /* address in ring where the buffer address (dma_addr) is written for
- * the device
- */
+ /* address in ring where the buffer address is written for the device */
__le64 *buf_ptr;
u32 index;
- DEFINE_DMA_UNMAP_ADDR(mapaddr);
};
/* buffer queue */
diff --git a/drivers/net/ethernet/qlogic/qlge/qlge_main.c b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
index e661ee2730e5..4a3fa0861f8d 100644
--- a/drivers/net/ethernet/qlogic/qlge/qlge_main.c
+++ b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
@@ -995,8 +995,7 @@ static struct qlge_bq_desc *ql_get_curr_lchunk(struct ql_adapter *qdev,
{
struct qlge_bq_desc *lbq_desc = qlge_get_curr_buf(&rx_ring->lbq);
- pci_dma_sync_single_for_cpu(qdev->pdev,
- dma_unmap_addr(lbq_desc, mapaddr),
+ pci_dma_sync_single_for_cpu(qdev->pdev, lbq_desc->dma_addr,
qdev->lbq_buf_size, PCI_DMA_FROMDEVICE);
if ((lbq_desc->p.pg_chunk.offset + qdev->lbq_buf_size) ==
@@ -1030,7 +1029,7 @@ static const char * const bq_type_name[] = {
[QLGE_LB] = "lbq",
};
-/* return size of allocated buffer (may be 0) or negative error */
+/* return 0 or negative error */
static int qlge_refill_sb(struct rx_ring *rx_ring,
struct qlge_bq_desc *sbq_desc)
{
@@ -1057,12 +1056,13 @@ static int qlge_refill_sb(struct rx_ring *rx_ring,
dev_kfree_skb_any(skb);
return -EIO;
}
+ *sbq_desc->buf_ptr = cpu_to_le64(sbq_desc->dma_addr);
sbq_desc->p.skb = skb;
- return SMALL_BUFFER_SIZE;
+ return 0;
}
-/* return size of allocated buffer or negative error */
+/* return 0 or negative error */
static int qlge_refill_lb(struct rx_ring *rx_ring,
struct qlge_bq_desc *lbq_desc)
{
@@ -1093,7 +1093,9 @@ static int qlge_refill_lb(struct rx_ring *rx_ring,
}
lbq_desc->p.pg_chunk = *master_chunk;
- lbq_desc->dma_addr = rx_ring->chunk_dma_addr + master_chunk->offset;
+ lbq_desc->dma_addr = rx_ring->chunk_dma_addr;
+ *lbq_desc->buf_ptr = cpu_to_le64(lbq_desc->dma_addr +
+ lbq_desc->p.pg_chunk.offset);
/* Adjust the master page chunk for next
* buffer get.
@@ -1106,7 +1108,7 @@ static int qlge_refill_lb(struct rx_ring *rx_ring,
get_page(master_chunk->page);
}
- return qdev->lbq_buf_size;
+ return 0;
}
static void qlge_refill_bq(struct qlge_bq *bq)
@@ -1137,13 +1139,7 @@ static void qlge_refill_bq(struct qlge_bq *bq)
retval = qlge_refill_sb(rx_ring, bq_desc);
else
retval = qlge_refill_lb(rx_ring, bq_desc);
-
- if (retval > 0) {
- dma_unmap_addr_set(bq_desc, mapaddr,
- bq_desc->dma_addr);
- *bq_desc->buf_ptr =
- cpu_to_le64(bq_desc->dma_addr);
- } else if (retval < 0) {
+ if (retval < 0) {
bq->clean_idx = clean_idx;
netif_err(qdev, ifup, qdev->ndev,
"ring %u %s: Could not get a page chunk, i=%d, clean_idx =%d .\n",
@@ -1566,8 +1562,7 @@ static void ql_process_mac_rx_skb(struct ql_adapter *qdev,
}
skb_reserve(new_skb, NET_IP_ALIGN);
- pci_dma_sync_single_for_cpu(qdev->pdev,
- dma_unmap_addr(sbq_desc, mapaddr),
+ pci_dma_sync_single_for_cpu(qdev->pdev, sbq_desc->dma_addr,
SMALL_BUF_MAP_SIZE, PCI_DMA_FROMDEVICE);
skb_put_data(new_skb, skb->data, length);
@@ -1689,9 +1684,8 @@ static struct sk_buff *ql_build_rx_skb(struct ql_adapter *qdev,
* Headers fit nicely into a small buffer.
*/
sbq_desc = qlge_get_curr_buf(&rx_ring->sbq);
- pci_unmap_single(qdev->pdev,
- dma_unmap_addr(sbq_desc, mapaddr),
- SMALL_BUF_MAP_SIZE, PCI_DMA_FROMDEVICE);
+ pci_unmap_single(qdev->pdev, sbq_desc->dma_addr,
+ SMALL_BUF_MAP_SIZE, PCI_DMA_FROMDEVICE);
skb = sbq_desc->p.skb;
ql_realign_skb(skb, hdr_len);
skb_put(skb, hdr_len);
@@ -1721,8 +1715,7 @@ static struct sk_buff *ql_build_rx_skb(struct ql_adapter *qdev,
*/
sbq_desc = qlge_get_curr_buf(&rx_ring->sbq);
pci_dma_sync_single_for_cpu(qdev->pdev,
- dma_unmap_addr(sbq_desc,
- mapaddr),
+ sbq_desc->dma_addr,
SMALL_BUF_MAP_SIZE,
PCI_DMA_FROMDEVICE);
skb_put_data(skb, sbq_desc->p.skb->data, length);
@@ -1734,8 +1727,7 @@ static struct sk_buff *ql_build_rx_skb(struct ql_adapter *qdev,
skb = sbq_desc->p.skb;
ql_realign_skb(skb, length);
skb_put(skb, length);
- pci_unmap_single(qdev->pdev,
- dma_unmap_addr(sbq_desc, mapaddr),
+ pci_unmap_single(qdev->pdev, sbq_desc->dma_addr,
SMALL_BUF_MAP_SIZE,
PCI_DMA_FROMDEVICE);
sbq_desc->p.skb = NULL;
@@ -1773,8 +1765,7 @@ static struct sk_buff *ql_build_rx_skb(struct ql_adapter *qdev,
"No skb available, drop the packet.\n");
return NULL;
}
- pci_unmap_page(qdev->pdev,
- dma_unmap_addr(lbq_desc, mapaddr),
+ pci_unmap_page(qdev->pdev, lbq_desc->dma_addr,
qdev->lbq_buf_size,
PCI_DMA_FROMDEVICE);
skb_reserve(skb, NET_IP_ALIGN);
@@ -1807,8 +1798,7 @@ static struct sk_buff *ql_build_rx_skb(struct ql_adapter *qdev,
*/
int size, i = 0;
sbq_desc = qlge_get_curr_buf(&rx_ring->sbq);
- pci_unmap_single(qdev->pdev,
- dma_unmap_addr(sbq_desc, mapaddr),
+ pci_unmap_single(qdev->pdev, sbq_desc->dma_addr,
SMALL_BUF_MAP_SIZE, PCI_DMA_FROMDEVICE);
if (!(ib_mac_rsp->flags4 & IB_MAC_IOCB_RSP_HS)) {
/*
@@ -2767,8 +2757,7 @@ static void ql_free_sbq_buffers(struct ql_adapter *qdev, struct rx_ring *rx_ring
return;
}
if (sbq_desc->p.skb) {
- pci_unmap_single(qdev->pdev,
- dma_unmap_addr(sbq_desc, mapaddr),
+ pci_unmap_single(qdev->pdev, sbq_desc->dma_addr,
SMALL_BUF_MAP_SIZE,
PCI_DMA_FROMDEVICE);
dev_kfree_skb(sbq_desc->p.skb);
--
2.21.0
^ permalink raw reply related [flat|nested] 46+ messages in thread* [PATCH net-next 09/16] qlge: Remove rx_ring.type
2019-06-17 7:48 [PATCH net-next 01/16] qlge: Remove irq_cnt Benjamin Poirier
` (6 preceding siblings ...)
2019-06-17 7:48 ` [PATCH net-next 08/16] qlge: Fix dma_sync_single calls Benjamin Poirier
@ 2019-06-17 7:48 ` Benjamin Poirier
2019-06-17 7:48 ` [PATCH net-next 10/16] qlge: Factor out duplicated expression Benjamin Poirier
` (9 subsequent siblings)
17 siblings, 0 replies; 46+ messages in thread
From: Benjamin Poirier @ 2019-06-17 7:48 UTC (permalink / raw)
To: Manish Chopra, GR-Linux-NIC-Dev, netdev
This field is redundant, the type can be determined from the index, cq_id.
Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
---
drivers/net/ethernet/qlogic/qlge/qlge.h | 10 -------
drivers/net/ethernet/qlogic/qlge/qlge_dbg.c | 16 +++++++---
drivers/net/ethernet/qlogic/qlge/qlge_main.c | 31 +++++---------------
3 files changed, 19 insertions(+), 38 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qlge/qlge.h b/drivers/net/ethernet/qlogic/qlge/qlge.h
index 519fa39dd194..5a4b2520cd2a 100644
--- a/drivers/net/ethernet/qlogic/qlge/qlge.h
+++ b/drivers/net/ethernet/qlogic/qlge/qlge.h
@@ -1387,15 +1387,6 @@ struct tx_ring {
u64 tx_errors;
};
-/*
- * Type of inbound queue.
- */
-enum {
- DEFAULT_Q = 2, /* Handles slow queue and chip/MPI events. */
- TX_Q = 3, /* Handles outbound completions. */
- RX_Q = 4, /* Handles inbound completions. */
-};
-
struct qlge_page_chunk {
struct page *page;
void *va; /* virt addr including offset */
@@ -1468,7 +1459,6 @@ struct rx_ring {
struct qlge_bq sbq;
/* Misc. handler elements. */
- u32 type; /* Type of queue, tx, rx. */
u32 irq; /* Which vector this ring is assigned. */
u32 cpu; /* Which CPU this should run on. */
char name[IFNAMSIZ + 5];
diff --git a/drivers/net/ethernet/qlogic/qlge/qlge_dbg.c b/drivers/net/ethernet/qlogic/qlge/qlge_dbg.c
index 35af06dd21dd..a177302073db 100644
--- a/drivers/net/ethernet/qlogic/qlge/qlge_dbg.c
+++ b/drivers/net/ethernet/qlogic/qlge/qlge_dbg.c
@@ -1731,16 +1731,24 @@ void ql_dump_cqicb(struct cqicb *cqicb)
le16_to_cpu(cqicb->sbq_len));
}
+static const char *qlge_rx_ring_type_name(struct rx_ring *rx_ring)
+{
+ struct ql_adapter *qdev = rx_ring->qdev;
+
+ if (rx_ring->cq_id < qdev->rss_ring_count)
+ return "RX COMPLETION";
+ else
+ return "TX COMPLETION";
+};
+
void ql_dump_rx_ring(struct rx_ring *rx_ring)
{
if (rx_ring == NULL)
return;
pr_err("===================== Dumping rx_ring %d ===============\n",
rx_ring->cq_id);
- pr_err("Dumping rx_ring %d, type = %s%s%s\n",
- rx_ring->cq_id, rx_ring->type == DEFAULT_Q ? "DEFAULT" : "",
- rx_ring->type == TX_Q ? "OUTBOUND COMPLETIONS" : "",
- rx_ring->type == RX_Q ? "INBOUND_COMPLETIONS" : "");
+ pr_err("Dumping rx_ring %d, type = %s\n", rx_ring->cq_id,
+ qlge_rx_ring_type_name(rx_ring));
pr_err("rx_ring->cqicb = %p\n", &rx_ring->cqicb);
pr_err("rx_ring->cq_base = %p\n", rx_ring->cq_base);
pr_err("rx_ring->cq_base_dma = %llx\n",
diff --git a/drivers/net/ethernet/qlogic/qlge/qlge_main.c b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
index 4a3fa0861f8d..733f3b87c3a5 100644
--- a/drivers/net/ethernet/qlogic/qlge/qlge_main.c
+++ b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
@@ -2785,14 +2785,10 @@ static void ql_free_rx_buffers(struct ql_adapter *qdev)
static void ql_alloc_rx_buffers(struct ql_adapter *qdev)
{
- struct rx_ring *rx_ring;
int i;
- for (i = 0; i < qdev->rx_ring_count; i++) {
- rx_ring = &qdev->rx_ring[i];
- if (rx_ring->type != TX_Q)
- ql_update_buffer_queues(rx_ring);
- }
+ for (i = 0; i < qdev->rss_ring_count; i++)
+ ql_update_buffer_queues(&qdev->rx_ring[i]);
}
static int qlge_init_bq(struct qlge_bq *bq)
@@ -3071,12 +3067,7 @@ static int ql_start_rx_ring(struct ql_adapter *qdev, struct rx_ring *rx_ring)
rx_ring->sbq.clean_idx = 0;
rx_ring->sbq.free_cnt = rx_ring->sbq.len;
}
- switch (rx_ring->type) {
- case TX_Q:
- cqicb->irq_delay = cpu_to_le16(qdev->tx_coalesce_usecs);
- cqicb->pkt_delay = cpu_to_le16(qdev->tx_max_coalesced_frames);
- break;
- case RX_Q:
+ if (rx_ring->cq_id < qdev->rss_ring_count) {
/* Inbound completion handling rx_rings run in
* separate NAPI contexts.
*/
@@ -3084,10 +3075,9 @@ static int ql_start_rx_ring(struct ql_adapter *qdev, struct rx_ring *rx_ring)
64);
cqicb->irq_delay = cpu_to_le16(qdev->rx_coalesce_usecs);
cqicb->pkt_delay = cpu_to_le16(qdev->rx_max_coalesced_frames);
- break;
- default:
- netif_printk(qdev, ifup, KERN_DEBUG, qdev->ndev,
- "Invalid rx_ring->type = %d.\n", rx_ring->type);
+ } else {
+ cqicb->irq_delay = cpu_to_le16(qdev->tx_coalesce_usecs);
+ cqicb->pkt_delay = cpu_to_le16(qdev->tx_max_coalesced_frames);
}
err = ql_write_cfg(qdev, cqicb, sizeof(struct cqicb),
CFG_LCQ, rx_ring->cq_id);
@@ -3433,12 +3423,7 @@ static int ql_request_irq(struct ql_adapter *qdev)
goto err_irq;
netif_err(qdev, ifup, qdev->ndev,
- "Hooked intr %d, queue type %s, with name %s.\n",
- i,
- qdev->rx_ring[0].type == DEFAULT_Q ?
- "DEFAULT_Q" :
- qdev->rx_ring[0].type == TX_Q ? "TX_Q" :
- qdev->rx_ring[0].type == RX_Q ? "RX_Q" : "",
+ "Hooked intr 0, queue type RX_Q, with name %s.\n",
intr_context->name);
}
intr_context->hooked = 1;
@@ -4001,7 +3986,6 @@ static int ql_configure_rings(struct ql_adapter *qdev)
rx_ring->sbq.type = QLGE_SB;
rx_ring->sbq.len = NUM_SMALL_BUFFERS;
rx_ring->sbq.size = rx_ring->sbq.len * sizeof(__le64);
- rx_ring->type = RX_Q;
} else {
/*
* Outbound queue handles outbound completions only.
@@ -4014,7 +3998,6 @@ static int ql_configure_rings(struct ql_adapter *qdev)
rx_ring->lbq.size = 0;
rx_ring->sbq.len = 0;
rx_ring->sbq.size = 0;
- rx_ring->type = TX_Q;
}
}
return 0;
--
2.21.0
^ permalink raw reply related [flat|nested] 46+ messages in thread* [PATCH net-next 10/16] qlge: Factor out duplicated expression
2019-06-17 7:48 [PATCH net-next 01/16] qlge: Remove irq_cnt Benjamin Poirier
` (7 preceding siblings ...)
2019-06-17 7:48 ` [PATCH net-next 09/16] qlge: Remove rx_ring.type Benjamin Poirier
@ 2019-06-17 7:48 ` Benjamin Poirier
2019-06-23 17:59 ` David Miller
2019-06-17 7:48 ` [PATCH net-next 11/16] qlge: Remove qlge_bq.len & size Benjamin Poirier
` (8 subsequent siblings)
17 siblings, 1 reply; 46+ messages in thread
From: Benjamin Poirier @ 2019-06-17 7:48 UTC (permalink / raw)
To: Manish Chopra, GR-Linux-NIC-Dev, netdev
Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
---
drivers/net/ethernet/qlogic/qlge/qlge.h | 6 ++++++
drivers/net/ethernet/qlogic/qlge/qlge_main.c | 18 ++++++------------
2 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qlge/qlge.h b/drivers/net/ethernet/qlogic/qlge/qlge.h
index 5a4b2520cd2a..0bb7ccdca6a7 100644
--- a/drivers/net/ethernet/qlogic/qlge/qlge.h
+++ b/drivers/net/ethernet/qlogic/qlge/qlge.h
@@ -77,6 +77,12 @@
#define LSD(x) ((u32)((u64)(x)))
#define MSD(x) ((u32)((((u64)(x)) >> 32)))
+#define QLGE_FIT16(value) \
+({ \
+ typeof(value) _value = value; \
+ (_value) == 65536 ? 0 : (u16)(_value); \
+})
+
/* MPI test register definitions. This register
* is used for determining alternate NIC function's
* PCI->func number.
diff --git a/drivers/net/ethernet/qlogic/qlge/qlge_main.c b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
index 733f3b87c3a5..a21c47c4b9bd 100644
--- a/drivers/net/ethernet/qlogic/qlge/qlge_main.c
+++ b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
@@ -2974,7 +2974,6 @@ static int ql_start_rx_ring(struct ql_adapter *qdev, struct rx_ring *rx_ring)
void __iomem *doorbell_area =
qdev->doorbell_area + (DB_PAGE_SIZE * (128 + rx_ring->cq_id));
int err = 0;
- u16 bq_len;
u64 tmp;
__le64 *base_indirect_ptr;
int page_entries;
@@ -3009,8 +3008,8 @@ static int ql_start_rx_ring(struct ql_adapter *qdev, struct rx_ring *rx_ring)
memset((void *)cqicb, 0, sizeof(struct cqicb));
cqicb->msix_vect = rx_ring->irq;
- bq_len = (rx_ring->cq_len == 65536) ? 0 : (u16) rx_ring->cq_len;
- cqicb->len = cpu_to_le16(bq_len | LEN_V | LEN_CPP_CONT);
+ cqicb->len = cpu_to_le16(QLGE_FIT16(rx_ring->cq_len) | LEN_V |
+ LEN_CPP_CONT);
cqicb->addr = cpu_to_le64(rx_ring->cq_base_dma);
@@ -3034,12 +3033,9 @@ static int ql_start_rx_ring(struct ql_adapter *qdev, struct rx_ring *rx_ring)
page_entries++;
} while (page_entries < MAX_DB_PAGES_PER_BQ(rx_ring->lbq.len));
cqicb->lbq_addr = cpu_to_le64(rx_ring->lbq.base_indirect_dma);
- bq_len = qdev->lbq_buf_size == 65536 ? 0 :
- (u16)qdev->lbq_buf_size;
- cqicb->lbq_buf_size = cpu_to_le16(bq_len);
- bq_len = (rx_ring->lbq.len == 65536) ? 0 :
- (u16)rx_ring->lbq.len;
- cqicb->lbq_len = cpu_to_le16(bq_len);
+ cqicb->lbq_buf_size =
+ cpu_to_le16(QLGE_FIT16(qdev->lbq_buf_size));
+ cqicb->lbq_len = cpu_to_le16(QLGE_FIT16(rx_ring->lbq.len));
rx_ring->lbq.prod_idx = 0;
rx_ring->lbq.curr_idx = 0;
rx_ring->lbq.clean_idx = 0;
@@ -3059,9 +3055,7 @@ static int ql_start_rx_ring(struct ql_adapter *qdev, struct rx_ring *rx_ring)
cqicb->sbq_addr =
cpu_to_le64(rx_ring->sbq.base_indirect_dma);
cqicb->sbq_buf_size = cpu_to_le16(SMALL_BUFFER_SIZE);
- bq_len = (rx_ring->sbq.len == 65536) ? 0 :
- (u16)rx_ring->sbq.len;
- cqicb->sbq_len = cpu_to_le16(bq_len);
+ cqicb->sbq_len = cpu_to_le16(QLGE_FIT16(rx_ring->sbq.len));
rx_ring->sbq.prod_idx = 0;
rx_ring->sbq.curr_idx = 0;
rx_ring->sbq.clean_idx = 0;
--
2.21.0
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH net-next 10/16] qlge: Factor out duplicated expression
2019-06-17 7:48 ` [PATCH net-next 10/16] qlge: Factor out duplicated expression Benjamin Poirier
@ 2019-06-23 17:59 ` David Miller
2019-06-23 18:00 ` David Miller
2019-06-24 7:52 ` Benjamin Poirier
0 siblings, 2 replies; 46+ messages in thread
From: David Miller @ 2019-06-23 17:59 UTC (permalink / raw)
To: bpoirier; +Cc: manishc, GR-Linux-NIC-Dev, netdev
From: Benjamin Poirier <bpoirier@suse.com>
Date: Mon, 17 Jun 2019 16:48:52 +0900
> Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
> ---
> drivers/net/ethernet/qlogic/qlge/qlge.h | 6 ++++++
> drivers/net/ethernet/qlogic/qlge/qlge_main.c | 18 ++++++------------
> 2 files changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/ethernet/qlogic/qlge/qlge.h b/drivers/net/ethernet/qlogic/qlge/qlge.h
> index 5a4b2520cd2a..0bb7ccdca6a7 100644
> --- a/drivers/net/ethernet/qlogic/qlge/qlge.h
> +++ b/drivers/net/ethernet/qlogic/qlge/qlge.h
> @@ -77,6 +77,12 @@
> #define LSD(x) ((u32)((u64)(x)))
> #define MSD(x) ((u32)((((u64)(x)) >> 32)))
>
> +#define QLGE_FIT16(value) \
> +({ \
> + typeof(value) _value = value; \
> + (_value) == 65536 ? 0 : (u16)(_value); \
> +})
> +
"(u16) 65536" is zero and the range of these values is 0 -- 65536.
This whole expression is way overdone.
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH net-next 10/16] qlge: Factor out duplicated expression
2019-06-23 17:59 ` David Miller
@ 2019-06-23 18:00 ` David Miller
2019-06-24 7:52 ` Benjamin Poirier
1 sibling, 0 replies; 46+ messages in thread
From: David Miller @ 2019-06-23 18:00 UTC (permalink / raw)
To: bpoirier; +Cc: manishc, GR-Linux-NIC-Dev, netdev
From: David Miller <davem@davemloft.net>
Date: Sun, 23 Jun 2019 10:59:35 -0700 (PDT)
> "(u16) 65536" is zero and the range of these values is 0 -- 65536.
>
> This whole expression is way overdone.
Also, when you post the next revision of this patch series, please
provide a proper "[PATCH net-next 00/16]" header posting explaining
what this patch series does logically at the high level, how it is
doing it, and why it is doing it that way.
Thank you.
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH net-next 10/16] qlge: Factor out duplicated expression
2019-06-23 17:59 ` David Miller
2019-06-23 18:00 ` David Miller
@ 2019-06-24 7:52 ` Benjamin Poirier
2019-06-25 18:32 ` Manish Chopra
1 sibling, 1 reply; 46+ messages in thread
From: Benjamin Poirier @ 2019-06-24 7:52 UTC (permalink / raw)
To: David Miller; +Cc: manishc, GR-Linux-NIC-Dev, netdev
On 2019/06/23 10:59, David Miller wrote:
> From: Benjamin Poirier <bpoirier@suse.com>
> Date: Mon, 17 Jun 2019 16:48:52 +0900
>
> > Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
> > ---
> > drivers/net/ethernet/qlogic/qlge/qlge.h | 6 ++++++
> > drivers/net/ethernet/qlogic/qlge/qlge_main.c | 18 ++++++------------
> > 2 files changed, 12 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/qlogic/qlge/qlge.h b/drivers/net/ethernet/qlogic/qlge/qlge.h
> > index 5a4b2520cd2a..0bb7ccdca6a7 100644
> > --- a/drivers/net/ethernet/qlogic/qlge/qlge.h
> > +++ b/drivers/net/ethernet/qlogic/qlge/qlge.h
> > @@ -77,6 +77,12 @@
> > #define LSD(x) ((u32)((u64)(x)))
> > #define MSD(x) ((u32)((((u64)(x)) >> 32)))
> >
> > +#define QLGE_FIT16(value) \
> > +({ \
> > + typeof(value) _value = value; \
> > + (_value) == 65536 ? 0 : (u16)(_value); \
> > +})
> > +
>
> "(u16) 65536" is zero and the range of these values is 0 -- 65536.
>
> This whole expression is way overdone.
Indeed, I missed that a simple cast is enough :/
What I inferred from the presence of that expression though is that in
the places where it is used, the device interprets a value of 0 as
65536. Manish, can you confirm that? As David points out, the expression
is useless. A comment might not be however.
^ permalink raw reply [flat|nested] 46+ messages in thread* RE: [PATCH net-next 10/16] qlge: Factor out duplicated expression
2019-06-24 7:52 ` Benjamin Poirier
@ 2019-06-25 18:32 ` Manish Chopra
2019-06-28 8:52 ` Benjamin Poirier
0 siblings, 1 reply; 46+ messages in thread
From: Manish Chopra @ 2019-06-25 18:32 UTC (permalink / raw)
To: Benjamin Poirier, David Miller; +Cc: GR-Linux-NIC-Dev, netdev
> -----Original Message-----
> From: netdev-owner@vger.kernel.org <netdev-owner@vger.kernel.org> On
> Behalf Of Benjamin Poirier
> Sent: Monday, June 24, 2019 1:22 PM
> To: David Miller <davem@davemloft.net>
> Cc: Manish Chopra <manishc@marvell.com>; GR-Linux-NIC-Dev <GR-Linux-
> NIC-Dev@marvell.com>; netdev@vger.kernel.org
> Subject: Re: [PATCH net-next 10/16] qlge: Factor out duplicated expression
>
> On 2019/06/23 10:59, David Miller wrote:
> > From: Benjamin Poirier <bpoirier@suse.com>
> > Date: Mon, 17 Jun 2019 16:48:52 +0900
> >
> > > Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
> > > ---
> > > drivers/net/ethernet/qlogic/qlge/qlge.h | 6 ++++++
> > > drivers/net/ethernet/qlogic/qlge/qlge_main.c | 18
> > > ++++++------------
> > > 2 files changed, 12 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/qlogic/qlge/qlge.h
> > > b/drivers/net/ethernet/qlogic/qlge/qlge.h
> > > index 5a4b2520cd2a..0bb7ccdca6a7 100644
> > > --- a/drivers/net/ethernet/qlogic/qlge/qlge.h
> > > +++ b/drivers/net/ethernet/qlogic/qlge/qlge.h
> > > @@ -77,6 +77,12 @@
> > > #define LSD(x) ((u32)((u64)(x)))
> > > #define MSD(x) ((u32)((((u64)(x)) >> 32)))
> > >
> > > +#define QLGE_FIT16(value) \
> > > +({ \
> > > + typeof(value) _value = value; \
> > > + (_value) == 65536 ? 0 : (u16)(_value); \
> > > +})
> > > +
> >
> > "(u16) 65536" is zero and the range of these values is 0 -- 65536.
> >
> > This whole expression is way overdone.
>
> Indeed, I missed that a simple cast is enough :/
>
> What I inferred from the presence of that expression though is that in the
> places where it is used, the device interprets a value of 0 as 65536. Manish,
> can you confirm that? As David points out, the expression is useless. A
> comment might not be however.
Yes, I think it could be simplified to simple cast just.
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH net-next 10/16] qlge: Factor out duplicated expression
2019-06-25 18:32 ` Manish Chopra
@ 2019-06-28 8:52 ` Benjamin Poirier
0 siblings, 0 replies; 46+ messages in thread
From: Benjamin Poirier @ 2019-06-28 8:52 UTC (permalink / raw)
To: Manish Chopra; +Cc: David Miller, GR-Linux-NIC-Dev, netdev
On 2019/06/25 18:32, Manish Chopra wrote:
[...]
> >
> > What I inferred from the presence of that expression though is that in the
> > places where it is used, the device interprets a value of 0 as 65536. Manish,
> > can you confirm that? As David points out, the expression is useless. A
> > comment might not be however.
>
> Yes, I think it could be simplified to simple cast just.
>
I checked and it seems like the device treats cqicq->lbq_buf_size = 0
more like 65536 than like 0. That is, I saw it write up to 9596B
(running at 9000 mtu...) in a single lbq buffer.
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH net-next 11/16] qlge: Remove qlge_bq.len & size
2019-06-17 7:48 [PATCH net-next 01/16] qlge: Remove irq_cnt Benjamin Poirier
` (8 preceding siblings ...)
2019-06-17 7:48 ` [PATCH net-next 10/16] qlge: Factor out duplicated expression Benjamin Poirier
@ 2019-06-17 7:48 ` Benjamin Poirier
2019-06-27 10:47 ` Manish Chopra
2019-06-17 7:48 ` [PATCH net-next 12/16] qlge: Remove useless memset Benjamin Poirier
` (7 subsequent siblings)
17 siblings, 1 reply; 46+ messages in thread
From: Benjamin Poirier @ 2019-06-17 7:48 UTC (permalink / raw)
To: Manish Chopra, GR-Linux-NIC-Dev, netdev
Given the way the driver currently works, these values are always known
at compile time.
Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
---
drivers/net/ethernet/qlogic/qlge/qlge.h | 17 +++--
drivers/net/ethernet/qlogic/qlge/qlge_dbg.c | 4 -
drivers/net/ethernet/qlogic/qlge/qlge_main.c | 79 ++++++++------------
3 files changed, 42 insertions(+), 58 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qlge/qlge.h b/drivers/net/ethernet/qlogic/qlge/qlge.h
index 0bb7ccdca6a7..3722748265db 100644
--- a/drivers/net/ethernet/qlogic/qlge/qlge.h
+++ b/drivers/net/ethernet/qlogic/qlge/qlge.h
@@ -34,8 +34,13 @@
#define NUM_TX_RING_ENTRIES 256
#define NUM_RX_RING_ENTRIES 256
-#define NUM_SMALL_BUFFERS 512
-#define NUM_LARGE_BUFFERS 512
+/* Use the same len for sbq and lbq. Note that it seems like the device might
+ * support different sizes.
+ */
+#define QLGE_BQ_SHIFT 9
+#define QLGE_BQ_LEN BIT(QLGE_BQ_SHIFT)
+#define QLGE_BQ_SIZE (QLGE_BQ_LEN * sizeof(__le64))
+
#define DB_PAGE_SIZE 4096
/* Calculate the number of (4k) pages required to
@@ -46,8 +51,8 @@
(((x * sizeof(u64)) % DB_PAGE_SIZE) ? 1 : 0))
#define RX_RING_SHADOW_SPACE (sizeof(u64) + \
- MAX_DB_PAGES_PER_BQ(NUM_SMALL_BUFFERS) * sizeof(u64) + \
- MAX_DB_PAGES_PER_BQ(NUM_LARGE_BUFFERS) * sizeof(u64))
+ MAX_DB_PAGES_PER_BQ(QLGE_BQ_LEN) * sizeof(u64) + \
+ MAX_DB_PAGES_PER_BQ(QLGE_BQ_LEN) * sizeof(u64))
#define LARGE_BUFFER_MAX_SIZE 8192
#define LARGE_BUFFER_MIN_SIZE 2048
@@ -1420,8 +1425,6 @@ struct qlge_bq {
dma_addr_t base_indirect_dma;
struct qlge_bq_desc *queue;
void __iomem *prod_idx_db_reg;
- u32 len; /* entry count */
- u32 size; /* size in bytes of hw ring */
u32 prod_idx; /* current sw prod idx */
u32 curr_idx; /* next entry we expect */
u32 clean_idx; /* beginning of new descs */
@@ -1440,6 +1443,8 @@ struct qlge_bq {
offsetof(struct rx_ring, lbq))); \
})
+#define QLGE_BQ_WRAP(index) ((index) & (QLGE_BQ_LEN - 1))
+
struct rx_ring {
struct cqicb cqicb; /* The chip's completion queue init control block. */
diff --git a/drivers/net/ethernet/qlogic/qlge/qlge_dbg.c b/drivers/net/ethernet/qlogic/qlge/qlge_dbg.c
index a177302073db..c21d1b228bd2 100644
--- a/drivers/net/ethernet/qlogic/qlge/qlge_dbg.c
+++ b/drivers/net/ethernet/qlogic/qlge/qlge_dbg.c
@@ -1775,8 +1775,6 @@ void ql_dump_rx_ring(struct rx_ring *rx_ring)
pr_err("rx_ring->lbq.base_indirect_dma = %llx\n",
(unsigned long long)rx_ring->lbq.base_indirect_dma);
pr_err("rx_ring->lbq = %p\n", rx_ring->lbq.queue);
- pr_err("rx_ring->lbq.len = %d\n", rx_ring->lbq.len);
- pr_err("rx_ring->lbq.size = %d\n", rx_ring->lbq.size);
pr_err("rx_ring->lbq.prod_idx_db_reg = %p\n",
rx_ring->lbq.prod_idx_db_reg);
pr_err("rx_ring->lbq.prod_idx = %d\n", rx_ring->lbq.prod_idx);
@@ -1792,8 +1790,6 @@ void ql_dump_rx_ring(struct rx_ring *rx_ring)
pr_err("rx_ring->sbq.base_indirect_dma = %llx\n",
(unsigned long long)rx_ring->sbq.base_indirect_dma);
pr_err("rx_ring->sbq = %p\n", rx_ring->sbq.queue);
- pr_err("rx_ring->sbq.len = %d\n", rx_ring->sbq.len);
- pr_err("rx_ring->sbq.size = %d\n", rx_ring->sbq.size);
pr_err("rx_ring->sbq.prod_idx_db_reg addr = %p\n",
rx_ring->sbq.prod_idx_db_reg);
pr_err("rx_ring->sbq.prod_idx = %d\n", rx_ring->sbq.prod_idx);
diff --git a/drivers/net/ethernet/qlogic/qlge/qlge_main.c b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
index a21c47c4b9bd..b1fabe60f589 100644
--- a/drivers/net/ethernet/qlogic/qlge/qlge_main.c
+++ b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
@@ -982,9 +982,8 @@ static struct qlge_bq_desc *qlge_get_curr_buf(struct qlge_bq *bq)
{
struct qlge_bq_desc *bq_desc;
- bq_desc = &bq->queue[bq->curr_idx++];
- if (bq->curr_idx == bq->len)
- bq->curr_idx = 0;
+ bq_desc = &bq->queue[bq->curr_idx];
+ bq->curr_idx = QLGE_BQ_WRAP(bq->curr_idx + 1);
bq->free_cnt++;
return bq_desc;
@@ -1150,13 +1149,13 @@ static void qlge_refill_bq(struct qlge_bq *bq)
}
clean_idx++;
- if (clean_idx == bq->len)
+ if (clean_idx == QLGE_BQ_LEN)
clean_idx = 0;
}
bq->clean_idx = clean_idx;
bq->prod_idx += 16;
- if (bq->prod_idx == bq->len)
+ if (bq->prod_idx == QLGE_BQ_LEN)
bq->prod_idx = 0;
bq->free_cnt -= 16;
}
@@ -2732,8 +2731,7 @@ static void ql_free_lbq_buffers(struct ql_adapter *qdev, struct rx_ring *rx_ring
put_page(lbq_desc->p.pg_chunk.page);
lbq_desc->p.pg_chunk.page = NULL;
- if (++curr_idx == rx_ring->lbq.len)
- curr_idx = 0;
+ curr_idx = QLGE_BQ_WRAP(curr_idx + 1);
}
if (rx_ring->master_chunk.page) {
@@ -2748,7 +2746,7 @@ static void ql_free_sbq_buffers(struct ql_adapter *qdev, struct rx_ring *rx_ring
{
int i;
- for (i = 0; i < rx_ring->sbq.len; i++) {
+ for (i = 0; i < QLGE_BQ_LEN; i++) {
struct qlge_bq_desc *sbq_desc = &rx_ring->sbq.queue[i];
if (sbq_desc == NULL) {
@@ -2773,13 +2771,11 @@ static void ql_free_rx_buffers(struct ql_adapter *qdev)
{
int i;
- for (i = 0; i < qdev->rx_ring_count; i++) {
+ for (i = 0; i < qdev->rss_ring_count; i++) {
struct rx_ring *rx_ring = &qdev->rx_ring[i];
- if (rx_ring->lbq.queue)
- ql_free_lbq_buffers(qdev, rx_ring);
- if (rx_ring->sbq.queue)
- ql_free_sbq_buffers(qdev, rx_ring);
+ ql_free_lbq_buffers(qdev, rx_ring);
+ ql_free_sbq_buffers(qdev, rx_ring);
}
}
@@ -2799,7 +2795,8 @@ static int qlge_init_bq(struct qlge_bq *bq)
__le64 *buf_ptr;
int i;
- bq->base = pci_alloc_consistent(qdev->pdev, bq->size, &bq->base_dma);
+ bq->base = pci_alloc_consistent(qdev->pdev, QLGE_BQ_SIZE,
+ &bq->base_dma);
if (!bq->base) {
netif_err(qdev, ifup, qdev->ndev,
"ring %u %s allocation failed.\n", rx_ring->cq_id,
@@ -2807,16 +2804,16 @@ static int qlge_init_bq(struct qlge_bq *bq)
return -ENOMEM;
}
- bq->queue = kmalloc_array(bq->len, sizeof(struct qlge_bq_desc),
+ bq->queue = kmalloc_array(QLGE_BQ_LEN, sizeof(struct qlge_bq_desc),
GFP_KERNEL);
if (!bq->queue)
return -ENOMEM;
- memset(bq->queue, 0, bq->len * sizeof(struct qlge_bq_desc));
+ memset(bq->queue, 0, QLGE_BQ_LEN * sizeof(struct qlge_bq_desc));
buf_ptr = bq->base;
bq_desc = &bq->queue[0];
- for (i = 0; i < bq->len; i++, buf_ptr++, bq_desc++) {
+ for (i = 0; i < QLGE_BQ_LEN; i++, buf_ptr++, bq_desc++) {
memset(bq_desc, 0, sizeof(*bq_desc));
bq_desc->index = i;
bq_desc->buf_ptr = buf_ptr;
@@ -2830,8 +2827,7 @@ static void ql_free_rx_resources(struct ql_adapter *qdev,
{
/* Free the small buffer queue. */
if (rx_ring->sbq.base) {
- pci_free_consistent(qdev->pdev,
- rx_ring->sbq.size,
+ pci_free_consistent(qdev->pdev, QLGE_BQ_SIZE,
rx_ring->sbq.base, rx_ring->sbq.base_dma);
rx_ring->sbq.base = NULL;
}
@@ -2842,8 +2838,7 @@ static void ql_free_rx_resources(struct ql_adapter *qdev,
/* Free the large buffer queue. */
if (rx_ring->lbq.base) {
- pci_free_consistent(qdev->pdev,
- rx_ring->lbq.size,
+ pci_free_consistent(qdev->pdev, QLGE_BQ_SIZE,
rx_ring->lbq.base, rx_ring->lbq.base_dma);
rx_ring->lbq.base = NULL;
}
@@ -2879,16 +2874,13 @@ static int ql_alloc_rx_resources(struct ql_adapter *qdev,
return -ENOMEM;
}
- if (rx_ring->sbq.len && qlge_init_bq(&rx_ring->sbq))
- goto err_mem;
- if (rx_ring->lbq.len && qlge_init_bq(&rx_ring->lbq))
- goto err_mem;
+ if (rx_ring->cq_id < qdev->rss_ring_count &&
+ (qlge_init_bq(&rx_ring->sbq) || qlge_init_bq(&rx_ring->lbq))) {
+ ql_free_rx_resources(qdev, rx_ring);
+ return -ENOMEM;
+ }
return 0;
-
-err_mem:
- ql_free_rx_resources(qdev, rx_ring);
- return -ENOMEM;
}
static void ql_tx_ring_clean(struct ql_adapter *qdev)
@@ -2986,8 +2978,8 @@ static int ql_start_rx_ring(struct ql_adapter *qdev, struct rx_ring *rx_ring)
shadow_reg_dma += sizeof(u64);
rx_ring->lbq.base_indirect = shadow_reg;
rx_ring->lbq.base_indirect_dma = shadow_reg_dma;
- shadow_reg += (sizeof(u64) * MAX_DB_PAGES_PER_BQ(rx_ring->lbq.len));
- shadow_reg_dma += (sizeof(u64) * MAX_DB_PAGES_PER_BQ(rx_ring->lbq.len));
+ shadow_reg += (sizeof(u64) * MAX_DB_PAGES_PER_BQ(QLGE_BQ_LEN));
+ shadow_reg_dma += (sizeof(u64) * MAX_DB_PAGES_PER_BQ(QLGE_BQ_LEN));
rx_ring->sbq.base_indirect = shadow_reg;
rx_ring->sbq.base_indirect_dma = shadow_reg_dma;
@@ -3021,7 +3013,7 @@ static int ql_start_rx_ring(struct ql_adapter *qdev, struct rx_ring *rx_ring)
cqicb->flags = FLAGS_LC | /* Load queue base address */
FLAGS_LV | /* Load MSI-X vector */
FLAGS_LI; /* Load irq delay values */
- if (rx_ring->lbq.len) {
+ if (rx_ring->cq_id < qdev->rss_ring_count) {
cqicb->flags |= FLAGS_LL; /* Load lbq values */
tmp = (u64)rx_ring->lbq.base_dma;
base_indirect_ptr = rx_ring->lbq.base_indirect;
@@ -3031,17 +3023,16 @@ static int ql_start_rx_ring(struct ql_adapter *qdev, struct rx_ring *rx_ring)
tmp += DB_PAGE_SIZE;
base_indirect_ptr++;
page_entries++;
- } while (page_entries < MAX_DB_PAGES_PER_BQ(rx_ring->lbq.len));
+ } while (page_entries < MAX_DB_PAGES_PER_BQ(QLGE_BQ_LEN));
cqicb->lbq_addr = cpu_to_le64(rx_ring->lbq.base_indirect_dma);
cqicb->lbq_buf_size =
cpu_to_le16(QLGE_FIT16(qdev->lbq_buf_size));
- cqicb->lbq_len = cpu_to_le16(QLGE_FIT16(rx_ring->lbq.len));
+ cqicb->lbq_len = cpu_to_le16(QLGE_FIT16(QLGE_BQ_LEN));
rx_ring->lbq.prod_idx = 0;
rx_ring->lbq.curr_idx = 0;
rx_ring->lbq.clean_idx = 0;
- rx_ring->lbq.free_cnt = rx_ring->lbq.len;
- }
- if (rx_ring->sbq.len) {
+ rx_ring->lbq.free_cnt = QLGE_BQ_LEN;
+
cqicb->flags |= FLAGS_LS; /* Load sbq values */
tmp = (u64)rx_ring->sbq.base_dma;
base_indirect_ptr = rx_ring->sbq.base_indirect;
@@ -3051,15 +3042,15 @@ static int ql_start_rx_ring(struct ql_adapter *qdev, struct rx_ring *rx_ring)
tmp += DB_PAGE_SIZE;
base_indirect_ptr++;
page_entries++;
- } while (page_entries < MAX_DB_PAGES_PER_BQ(rx_ring->sbq.len));
+ } while (page_entries < MAX_DB_PAGES_PER_BQ(QLGE_BQ_LEN));
cqicb->sbq_addr =
cpu_to_le64(rx_ring->sbq.base_indirect_dma);
cqicb->sbq_buf_size = cpu_to_le16(SMALL_BUFFER_SIZE);
- cqicb->sbq_len = cpu_to_le16(QLGE_FIT16(rx_ring->sbq.len));
+ cqicb->sbq_len = cpu_to_le16(QLGE_FIT16(QLGE_BQ_LEN));
rx_ring->sbq.prod_idx = 0;
rx_ring->sbq.curr_idx = 0;
rx_ring->sbq.clean_idx = 0;
- rx_ring->sbq.free_cnt = rx_ring->sbq.len;
+ rx_ring->sbq.free_cnt = QLGE_BQ_LEN;
}
if (rx_ring->cq_id < qdev->rss_ring_count) {
/* Inbound completion handling rx_rings run in
@@ -3975,11 +3966,7 @@ static int ql_configure_rings(struct ql_adapter *qdev)
rx_ring->cq_size =
rx_ring->cq_len * sizeof(struct ql_net_rsp_iocb);
rx_ring->lbq.type = QLGE_LB;
- rx_ring->lbq.len = NUM_LARGE_BUFFERS;
- rx_ring->lbq.size = rx_ring->lbq.len * sizeof(__le64);
rx_ring->sbq.type = QLGE_SB;
- rx_ring->sbq.len = NUM_SMALL_BUFFERS;
- rx_ring->sbq.size = rx_ring->sbq.len * sizeof(__le64);
} else {
/*
* Outbound queue handles outbound completions only.
@@ -3988,10 +3975,6 @@ static int ql_configure_rings(struct ql_adapter *qdev)
rx_ring->cq_len = qdev->tx_ring_size;
rx_ring->cq_size =
rx_ring->cq_len * sizeof(struct ql_net_rsp_iocb);
- rx_ring->lbq.len = 0;
- rx_ring->lbq.size = 0;
- rx_ring->sbq.len = 0;
- rx_ring->sbq.size = 0;
}
}
return 0;
--
2.21.0
^ permalink raw reply related [flat|nested] 46+ messages in thread* RE: [PATCH net-next 11/16] qlge: Remove qlge_bq.len & size
2019-06-17 7:48 ` [PATCH net-next 11/16] qlge: Remove qlge_bq.len & size Benjamin Poirier
@ 2019-06-27 10:47 ` Manish Chopra
2019-07-09 6:52 ` Benjamin Poirier
0 siblings, 1 reply; 46+ messages in thread
From: Manish Chopra @ 2019-06-27 10:47 UTC (permalink / raw)
To: Benjamin Poirier, GR-Linux-NIC-Dev, netdev
>
> - for (i = 0; i < qdev->rx_ring_count; i++) {
> + for (i = 0; i < qdev->rss_ring_count; i++) {
> struct rx_ring *rx_ring = &qdev->rx_ring[i];
>
> - if (rx_ring->lbq.queue)
> - ql_free_lbq_buffers(qdev, rx_ring);
> - if (rx_ring->sbq.queue)
> - ql_free_sbq_buffers(qdev, rx_ring);
> + ql_free_lbq_buffers(qdev, rx_ring);
> + ql_free_sbq_buffers(qdev, rx_ring);
> }
> }
>
Seems irrelevant change as per what this patch is supposed to do exactly.
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH net-next 11/16] qlge: Remove qlge_bq.len & size
2019-06-27 10:47 ` Manish Chopra
@ 2019-07-09 6:52 ` Benjamin Poirier
0 siblings, 0 replies; 46+ messages in thread
From: Benjamin Poirier @ 2019-07-09 6:52 UTC (permalink / raw)
To: Manish Chopra; +Cc: GR-Linux-NIC-Dev, netdev
On 2019/06/27 10:47, Manish Chopra wrote:
> >
> > - for (i = 0; i < qdev->rx_ring_count; i++) {
> > + for (i = 0; i < qdev->rss_ring_count; i++) {
> > struct rx_ring *rx_ring = &qdev->rx_ring[i];
> >
> > - if (rx_ring->lbq.queue)
> > - ql_free_lbq_buffers(qdev, rx_ring);
> > - if (rx_ring->sbq.queue)
> > - ql_free_sbq_buffers(qdev, rx_ring);
> > + ql_free_lbq_buffers(qdev, rx_ring);
> > + ql_free_sbq_buffers(qdev, rx_ring);
> > }
> > }
> >
>
> Seems irrelevant change as per what this patch is supposed to do exactly.
Sure, I've removed that hunk.
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH net-next 12/16] qlge: Remove useless memset
2019-06-17 7:48 [PATCH net-next 01/16] qlge: Remove irq_cnt Benjamin Poirier
` (9 preceding siblings ...)
2019-06-17 7:48 ` [PATCH net-next 11/16] qlge: Remove qlge_bq.len & size Benjamin Poirier
@ 2019-06-17 7:48 ` Benjamin Poirier
2019-06-17 7:48 ` [PATCH net-next 13/16] qlge: Replace memset with assignment Benjamin Poirier
` (6 subsequent siblings)
17 siblings, 0 replies; 46+ messages in thread
From: Benjamin Poirier @ 2019-06-17 7:48 UTC (permalink / raw)
To: Manish Chopra, GR-Linux-NIC-Dev, netdev
This just repeats what the other memset a few lines above did.
Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
---
drivers/net/ethernet/qlogic/qlge/qlge_main.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/net/ethernet/qlogic/qlge/qlge_main.c b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
index b1fabe60f589..b6e948caf2aa 100644
--- a/drivers/net/ethernet/qlogic/qlge/qlge_main.c
+++ b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
@@ -2814,7 +2814,6 @@ static int qlge_init_bq(struct qlge_bq *bq)
buf_ptr = bq->base;
bq_desc = &bq->queue[0];
for (i = 0; i < QLGE_BQ_LEN; i++, buf_ptr++, bq_desc++) {
- memset(bq_desc, 0, sizeof(*bq_desc));
bq_desc->index = i;
bq_desc->buf_ptr = buf_ptr;
}
--
2.21.0
^ permalink raw reply related [flat|nested] 46+ messages in thread* [PATCH net-next 13/16] qlge: Replace memset with assignment
2019-06-17 7:48 [PATCH net-next 01/16] qlge: Remove irq_cnt Benjamin Poirier
` (10 preceding siblings ...)
2019-06-17 7:48 ` [PATCH net-next 12/16] qlge: Remove useless memset Benjamin Poirier
@ 2019-06-17 7:48 ` Benjamin Poirier
2019-06-17 7:48 ` [PATCH net-next 14/16] qlge: Update buffer queue prod index despite oom Benjamin Poirier
` (5 subsequent siblings)
17 siblings, 0 replies; 46+ messages in thread
From: Benjamin Poirier @ 2019-06-17 7:48 UTC (permalink / raw)
To: Manish Chopra, GR-Linux-NIC-Dev, netdev
Instead of clearing the structure wholesale, it is sufficient to initialize
the skb member which is used to manage sbq instances. lbq instances are
managed according to curr_idx and clean_idx.
Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
---
drivers/net/ethernet/qlogic/qlge/qlge_main.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qlge/qlge_main.c b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
index b6e948caf2aa..4502bb4758b4 100644
--- a/drivers/net/ethernet/qlogic/qlge/qlge_main.c
+++ b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
@@ -2809,11 +2809,10 @@ static int qlge_init_bq(struct qlge_bq *bq)
if (!bq->queue)
return -ENOMEM;
- memset(bq->queue, 0, QLGE_BQ_LEN * sizeof(struct qlge_bq_desc));
-
buf_ptr = bq->base;
bq_desc = &bq->queue[0];
for (i = 0; i < QLGE_BQ_LEN; i++, buf_ptr++, bq_desc++) {
+ bq_desc->p.skb = NULL;
bq_desc->index = i;
bq_desc->buf_ptr = buf_ptr;
}
--
2.21.0
^ permalink raw reply related [flat|nested] 46+ messages in thread* [PATCH net-next 14/16] qlge: Update buffer queue prod index despite oom
2019-06-17 7:48 [PATCH net-next 01/16] qlge: Remove irq_cnt Benjamin Poirier
` (11 preceding siblings ...)
2019-06-17 7:48 ` [PATCH net-next 13/16] qlge: Replace memset with assignment Benjamin Poirier
@ 2019-06-17 7:48 ` Benjamin Poirier
2019-06-17 7:48 ` [PATCH net-next 15/16] qlge: Refill rx buffers up to multiple of 16 Benjamin Poirier
` (4 subsequent siblings)
17 siblings, 0 replies; 46+ messages in thread
From: Benjamin Poirier @ 2019-06-17 7:48 UTC (permalink / raw)
To: Manish Chopra, GR-Linux-NIC-Dev, netdev
Currently, if we repeatedly fail to allocate all of the buffers from the
desired batching budget, we will never update the prod_idx register.
Restructure code to always update prod_idx if new buffers could be
allocated. This eliminates the current two stage process (clean_idx ->
prod_idx) and some associated bookkeeping variables.
Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
---
drivers/net/ethernet/qlogic/qlge/qlge.h | 8 +-
drivers/net/ethernet/qlogic/qlge/qlge_dbg.c | 10 +-
drivers/net/ethernet/qlogic/qlge/qlge_main.c | 109 +++++++++----------
3 files changed, 60 insertions(+), 67 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qlge/qlge.h b/drivers/net/ethernet/qlogic/qlge/qlge.h
index 3722748265db..c1b71af1d351 100644
--- a/drivers/net/ethernet/qlogic/qlge/qlge.h
+++ b/drivers/net/ethernet/qlogic/qlge/qlge.h
@@ -1425,10 +1425,10 @@ struct qlge_bq {
dma_addr_t base_indirect_dma;
struct qlge_bq_desc *queue;
void __iomem *prod_idx_db_reg;
- u32 prod_idx; /* current sw prod idx */
- u32 curr_idx; /* next entry we expect */
- u32 clean_idx; /* beginning of new descs */
- u32 free_cnt; /* free buffer desc cnt */
+ /* next index where sw should refill a buffer for hw */
+ u16 next_to_use;
+ /* next index where sw expects to find a buffer filled by hw */
+ u16 next_to_clean;
enum {
QLGE_SB, /* small buffer */
QLGE_LB, /* large buffer */
diff --git a/drivers/net/ethernet/qlogic/qlge/qlge_dbg.c b/drivers/net/ethernet/qlogic/qlge/qlge_dbg.c
index c21d1b228bd2..08d9223956c2 100644
--- a/drivers/net/ethernet/qlogic/qlge/qlge_dbg.c
+++ b/drivers/net/ethernet/qlogic/qlge/qlge_dbg.c
@@ -1777,8 +1777,8 @@ void ql_dump_rx_ring(struct rx_ring *rx_ring)
pr_err("rx_ring->lbq = %p\n", rx_ring->lbq.queue);
pr_err("rx_ring->lbq.prod_idx_db_reg = %p\n",
rx_ring->lbq.prod_idx_db_reg);
- pr_err("rx_ring->lbq.prod_idx = %d\n", rx_ring->lbq.prod_idx);
- pr_err("rx_ring->lbq.curr_idx = %d\n", rx_ring->lbq.curr_idx);
+ pr_err("rx_ring->lbq.next_to_use = %d\n", rx_ring->lbq.next_to_use);
+ pr_err("rx_ring->lbq.next_to_clean = %d\n", rx_ring->lbq.next_to_clean);
pr_err("rx_ring->lbq_clean_idx = %d\n", rx_ring->lbq_clean_idx);
pr_err("rx_ring->lbq_free_cnt = %d\n", rx_ring->lbq_free_cnt);
@@ -1792,10 +1792,8 @@ void ql_dump_rx_ring(struct rx_ring *rx_ring)
pr_err("rx_ring->sbq = %p\n", rx_ring->sbq.queue);
pr_err("rx_ring->sbq.prod_idx_db_reg addr = %p\n",
rx_ring->sbq.prod_idx_db_reg);
- pr_err("rx_ring->sbq.prod_idx = %d\n", rx_ring->sbq.prod_idx);
- pr_err("rx_ring->sbq.curr_idx = %d\n", rx_ring->sbq.curr_idx);
- pr_err("rx_ring->sbq.clean_idx = %d\n", rx_ring->sbq.clean_idx);
- pr_err("rx_ring->sbq.free_cnt = %d\n", rx_ring->sbq.free_cnt);
+ pr_err("rx_ring->sbq.next_to_use = %d\n", rx_ring->sbq.next_to_use);
+ pr_err("rx_ring->sbq.next_to_clean = %d\n", rx_ring->sbq.next_to_clean);
pr_err("rx_ring->cq_id = %d\n", rx_ring->cq_id);
pr_err("rx_ring->irq = %d\n", rx_ring->irq);
pr_err("rx_ring->cpu = %d\n", rx_ring->cpu);
diff --git a/drivers/net/ethernet/qlogic/qlge/qlge_main.c b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
index 4502bb4758b4..949dd48b3930 100644
--- a/drivers/net/ethernet/qlogic/qlge/qlge_main.c
+++ b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
@@ -982,9 +982,8 @@ static struct qlge_bq_desc *qlge_get_curr_buf(struct qlge_bq *bq)
{
struct qlge_bq_desc *bq_desc;
- bq_desc = &bq->queue[bq->curr_idx];
- bq->curr_idx = QLGE_BQ_WRAP(bq->curr_idx + 1);
- bq->free_cnt++;
+ bq_desc = &bq->queue[bq->next_to_clean];
+ bq->next_to_clean = QLGE_BQ_WRAP(bq->next_to_clean + 1);
return bq_desc;
}
@@ -1114,9 +1113,9 @@ static void qlge_refill_bq(struct qlge_bq *bq)
{
struct rx_ring *rx_ring = QLGE_BQ_CONTAINER(bq);
struct ql_adapter *qdev = rx_ring->qdev;
- u32 clean_idx = bq->clean_idx;
+ struct qlge_bq_desc *bq_desc;
+ int free_count, refill_count;
unsigned int reserved_count;
- u32 start_idx = clean_idx;
int i;
if (bq->type == QLGE_SB)
@@ -1124,48 +1123,52 @@ static void qlge_refill_bq(struct qlge_bq *bq)
else
reserved_count = 32;
- while (bq->free_cnt > reserved_count) {
- for (i = (bq->clean_idx % 16); i < 16; i++) {
- struct qlge_bq_desc *bq_desc = &bq->queue[clean_idx];
- int retval;
+ free_count = bq->next_to_clean - bq->next_to_use;
+ if (free_count <= 0)
+ free_count += QLGE_BQ_LEN;
- netif_printk(qdev, rx_status, KERN_DEBUG, qdev->ndev,
- "ring %u %s: try cleaning clean_idx = %d.\n",
- rx_ring->cq_id, bq_type_name[bq->type],
- clean_idx);
-
- if (bq->type == QLGE_SB)
- retval = qlge_refill_sb(rx_ring, bq_desc);
- else
- retval = qlge_refill_lb(rx_ring, bq_desc);
- if (retval < 0) {
- bq->clean_idx = clean_idx;
- netif_err(qdev, ifup, qdev->ndev,
- "ring %u %s: Could not get a page chunk, i=%d, clean_idx =%d .\n",
- rx_ring->cq_id,
- bq_type_name[bq->type], i,
- clean_idx);
- return;
- }
+ refill_count = free_count - reserved_count;
+ /* refill batch size */
+ if (refill_count < 16)
+ return;
+
+ i = bq->next_to_use;
+ bq_desc = &bq->queue[i];
+ i -= QLGE_BQ_LEN;
+ do {
+ int retval;
+
+ netif_printk(qdev, rx_status, KERN_DEBUG, qdev->ndev,
+ "ring %u %s: try cleaning idx %d\n",
+ rx_ring->cq_id, bq_type_name[bq->type], i);
- clean_idx++;
- if (clean_idx == QLGE_BQ_LEN)
- clean_idx = 0;
+ if (bq->type == QLGE_SB)
+ retval = qlge_refill_sb(rx_ring, bq_desc);
+ else
+ retval = qlge_refill_lb(rx_ring, bq_desc);
+ if (retval < 0) {
+ netif_err(qdev, ifup, qdev->ndev,
+ "ring %u %s: Could not get a page chunk, idx %d\n",
+ rx_ring->cq_id, bq_type_name[bq->type], i);
+ break;
}
- bq->clean_idx = clean_idx;
- bq->prod_idx += 16;
- if (bq->prod_idx == QLGE_BQ_LEN)
- bq->prod_idx = 0;
- bq->free_cnt -= 16;
- }
+ bq_desc++;
+ i++;
+ if (unlikely(!i)) {
+ bq_desc = &bq->queue[0];
+ i -= QLGE_BQ_LEN;
+ }
+ refill_count--;
+ } while (refill_count);
+ i += QLGE_BQ_LEN;
- if (start_idx != clean_idx) {
+ if (bq->next_to_use != i) {
netif_printk(qdev, rx_status, KERN_DEBUG, qdev->ndev,
"ring %u %s: updating prod idx = %d.\n",
- rx_ring->cq_id, bq_type_name[bq->type],
- bq->prod_idx);
- ql_write_db_reg(bq->prod_idx, bq->prod_idx_db_reg);
+ rx_ring->cq_id, bq_type_name[bq->type], i);
+ bq->next_to_use = i;
+ ql_write_db_reg(bq->next_to_use, bq->prod_idx_db_reg);
}
}
@@ -2713,25 +2716,21 @@ static int ql_alloc_tx_resources(struct ql_adapter *qdev,
static void ql_free_lbq_buffers(struct ql_adapter *qdev, struct rx_ring *rx_ring)
{
+ struct qlge_bq *lbq = &rx_ring->lbq;
unsigned int last_offset;
- uint32_t curr_idx, clean_idx;
-
last_offset = ql_lbq_block_size(qdev) - qdev->lbq_buf_size;
- curr_idx = rx_ring->lbq.curr_idx;
- clean_idx = rx_ring->lbq.clean_idx;
- while (curr_idx != clean_idx) {
- struct qlge_bq_desc *lbq_desc = &rx_ring->lbq.queue[curr_idx];
+ while (lbq->next_to_clean != lbq->next_to_use) {
+ struct qlge_bq_desc *lbq_desc =
+ &lbq->queue[lbq->next_to_clean];
if (lbq_desc->p.pg_chunk.offset == last_offset)
pci_unmap_page(qdev->pdev, lbq_desc->dma_addr,
ql_lbq_block_size(qdev),
PCI_DMA_FROMDEVICE);
-
put_page(lbq_desc->p.pg_chunk.page);
- lbq_desc->p.pg_chunk.page = NULL;
- curr_idx = QLGE_BQ_WRAP(curr_idx + 1);
+ lbq->next_to_clean = QLGE_BQ_WRAP(lbq->next_to_clean + 1);
}
if (rx_ring->master_chunk.page) {
@@ -3026,10 +3025,8 @@ static int ql_start_rx_ring(struct ql_adapter *qdev, struct rx_ring *rx_ring)
cqicb->lbq_buf_size =
cpu_to_le16(QLGE_FIT16(qdev->lbq_buf_size));
cqicb->lbq_len = cpu_to_le16(QLGE_FIT16(QLGE_BQ_LEN));
- rx_ring->lbq.prod_idx = 0;
- rx_ring->lbq.curr_idx = 0;
- rx_ring->lbq.clean_idx = 0;
- rx_ring->lbq.free_cnt = QLGE_BQ_LEN;
+ rx_ring->lbq.next_to_use = 0;
+ rx_ring->lbq.next_to_clean = 0;
cqicb->flags |= FLAGS_LS; /* Load sbq values */
tmp = (u64)rx_ring->sbq.base_dma;
@@ -3045,10 +3042,8 @@ static int ql_start_rx_ring(struct ql_adapter *qdev, struct rx_ring *rx_ring)
cpu_to_le64(rx_ring->sbq.base_indirect_dma);
cqicb->sbq_buf_size = cpu_to_le16(SMALL_BUFFER_SIZE);
cqicb->sbq_len = cpu_to_le16(QLGE_FIT16(QLGE_BQ_LEN));
- rx_ring->sbq.prod_idx = 0;
- rx_ring->sbq.curr_idx = 0;
- rx_ring->sbq.clean_idx = 0;
- rx_ring->sbq.free_cnt = QLGE_BQ_LEN;
+ rx_ring->sbq.next_to_use = 0;
+ rx_ring->sbq.next_to_clean = 0;
}
if (rx_ring->cq_id < qdev->rss_ring_count) {
/* Inbound completion handling rx_rings run in
--
2.21.0
^ permalink raw reply related [flat|nested] 46+ messages in thread* [PATCH net-next 15/16] qlge: Refill rx buffers up to multiple of 16
2019-06-17 7:48 [PATCH net-next 01/16] qlge: Remove irq_cnt Benjamin Poirier
` (12 preceding siblings ...)
2019-06-17 7:48 ` [PATCH net-next 14/16] qlge: Update buffer queue prod index despite oom Benjamin Poirier
@ 2019-06-17 7:48 ` Benjamin Poirier
2019-06-17 7:48 ` [PATCH net-next 16/16] qlge: Refill empty buffer queues from wq Benjamin Poirier
` (3 subsequent siblings)
17 siblings, 0 replies; 46+ messages in thread
From: Benjamin Poirier @ 2019-06-17 7:48 UTC (permalink / raw)
To: Manish Chopra, GR-Linux-NIC-Dev, netdev
Reading the {s,l}bq_prod_idx registers on a running device, it appears that
the adapter will only use buffers up to prod_idx & 0xfff0. The driver
currently uses fixed-size guard zones (16 for sbq, 32 for lbq - don't know
why this difference). After the previous patch, this approach no longer
guarantees prod_idx values aligned on multiples of 16. While it appears
that we can write unaligned values to prod_idx without ill effects on
device operation, it makes more sense to change qlge_refill_bq() to refill
up to a limit that corresponds with the device's behavior.
Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
---
drivers/net/ethernet/qlogic/qlge/qlge.h | 8 ++++++
drivers/net/ethernet/qlogic/qlge/qlge_main.c | 29 ++++++++------------
2 files changed, 19 insertions(+), 18 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qlge/qlge.h b/drivers/net/ethernet/qlogic/qlge/qlge.h
index c1b71af1d351..1d90b32f6285 100644
--- a/drivers/net/ethernet/qlogic/qlge/qlge.h
+++ b/drivers/net/ethernet/qlogic/qlge/qlge.h
@@ -1424,6 +1424,9 @@ struct qlge_bq {
__le64 *base_indirect;
dma_addr_t base_indirect_dma;
struct qlge_bq_desc *queue;
+ /* prod_idx is the index of the first buffer that may NOT be used by
+ * hw, ie. one after the last. Advanced by sw.
+ */
void __iomem *prod_idx_db_reg;
/* next index where sw should refill a buffer for hw */
u16 next_to_use;
@@ -1443,6 +1446,11 @@ struct qlge_bq {
offsetof(struct rx_ring, lbq))); \
})
+/* Experience shows that the device ignores the low 4 bits of the tail index.
+ * Refill up to a x16 multiple.
+ */
+#define QLGE_BQ_ALIGN(index) ALIGN_DOWN(index, 16)
+
#define QLGE_BQ_WRAP(index) ((index) & (QLGE_BQ_LEN - 1))
struct rx_ring {
diff --git a/drivers/net/ethernet/qlogic/qlge/qlge_main.c b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
index 949dd48b3930..7db4c31c9cc4 100644
--- a/drivers/net/ethernet/qlogic/qlge/qlge_main.c
+++ b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
@@ -1114,22 +1114,12 @@ static void qlge_refill_bq(struct qlge_bq *bq)
struct rx_ring *rx_ring = QLGE_BQ_CONTAINER(bq);
struct ql_adapter *qdev = rx_ring->qdev;
struct qlge_bq_desc *bq_desc;
- int free_count, refill_count;
- unsigned int reserved_count;
+ int refill_count;
int i;
- if (bq->type == QLGE_SB)
- reserved_count = 16;
- else
- reserved_count = 32;
-
- free_count = bq->next_to_clean - bq->next_to_use;
- if (free_count <= 0)
- free_count += QLGE_BQ_LEN;
-
- refill_count = free_count - reserved_count;
- /* refill batch size */
- if (refill_count < 16)
+ refill_count = QLGE_BQ_WRAP(QLGE_BQ_ALIGN(bq->next_to_clean - 1) -
+ bq->next_to_use);
+ if (!refill_count)
return;
i = bq->next_to_use;
@@ -1164,11 +1154,14 @@ static void qlge_refill_bq(struct qlge_bq *bq)
i += QLGE_BQ_LEN;
if (bq->next_to_use != i) {
- netif_printk(qdev, rx_status, KERN_DEBUG, qdev->ndev,
- "ring %u %s: updating prod idx = %d.\n",
- rx_ring->cq_id, bq_type_name[bq->type], i);
+ if (QLGE_BQ_ALIGN(bq->next_to_use) != QLGE_BQ_ALIGN(i)) {
+ netif_printk(qdev, rx_status, KERN_DEBUG, qdev->ndev,
+ "ring %u %s: updating prod idx = %d.\n",
+ rx_ring->cq_id, bq_type_name[bq->type],
+ i);
+ ql_write_db_reg(i, bq->prod_idx_db_reg);
+ }
bq->next_to_use = i;
- ql_write_db_reg(bq->next_to_use, bq->prod_idx_db_reg);
}
}
--
2.21.0
^ permalink raw reply related [flat|nested] 46+ messages in thread* [PATCH net-next 16/16] qlge: Refill empty buffer queues from wq
2019-06-17 7:48 [PATCH net-next 01/16] qlge: Remove irq_cnt Benjamin Poirier
` (13 preceding siblings ...)
2019-06-17 7:48 ` [PATCH net-next 15/16] qlge: Refill rx buffers up to multiple of 16 Benjamin Poirier
@ 2019-06-17 7:48 ` Benjamin Poirier
2019-06-27 14:18 ` [EXT] " Manish Chopra
2019-06-17 16:49 ` [PATCH net-next 01/16] qlge: Remove irq_cnt Manish Chopra
` (2 subsequent siblings)
17 siblings, 1 reply; 46+ messages in thread
From: Benjamin Poirier @ 2019-06-17 7:48 UTC (permalink / raw)
To: Manish Chopra, GR-Linux-NIC-Dev, netdev
When operating at mtu 9000, qlge does order-1 allocations for rx buffers in
atomic context. This is especially unreliable when free memory is low or
fragmented. Add an approach similar to commit 3161e453e496 ("virtio: net
refill on out-of-memory") to qlge so that the device doesn't lock up if
there are allocation failures.
Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
---
drivers/net/ethernet/qlogic/qlge/qlge.h | 8 ++
drivers/net/ethernet/qlogic/qlge/qlge_main.c | 80 ++++++++++++++++----
2 files changed, 72 insertions(+), 16 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qlge/qlge.h b/drivers/net/ethernet/qlogic/qlge/qlge.h
index 1d90b32f6285..9c4d933c1ff7 100644
--- a/drivers/net/ethernet/qlogic/qlge/qlge.h
+++ b/drivers/net/ethernet/qlogic/qlge/qlge.h
@@ -1453,6 +1453,13 @@ struct qlge_bq {
#define QLGE_BQ_WRAP(index) ((index) & (QLGE_BQ_LEN - 1))
+#define QLGE_BQ_HW_OWNED(bq) \
+({ \
+ typeof(bq) _bq = bq; \
+ QLGE_BQ_WRAP(QLGE_BQ_ALIGN((_bq)->next_to_use) - \
+ (_bq)->next_to_clean); \
+})
+
struct rx_ring {
struct cqicb cqicb; /* The chip's completion queue init control block. */
@@ -1480,6 +1487,7 @@ struct rx_ring {
/* Misc. handler elements. */
u32 irq; /* Which vector this ring is assigned. */
u32 cpu; /* Which CPU this should run on. */
+ struct delayed_work refill_work;
char name[IFNAMSIZ + 5];
struct napi_struct napi;
u8 reserved;
diff --git a/drivers/net/ethernet/qlogic/qlge/qlge_main.c b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
index 7db4c31c9cc4..a13bda566187 100644
--- a/drivers/net/ethernet/qlogic/qlge/qlge_main.c
+++ b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
@@ -1029,7 +1029,7 @@ static const char * const bq_type_name[] = {
/* return 0 or negative error */
static int qlge_refill_sb(struct rx_ring *rx_ring,
- struct qlge_bq_desc *sbq_desc)
+ struct qlge_bq_desc *sbq_desc, gfp_t gfp)
{
struct ql_adapter *qdev = rx_ring->qdev;
struct sk_buff *skb;
@@ -1041,7 +1041,7 @@ static int qlge_refill_sb(struct rx_ring *rx_ring,
"ring %u sbq: getting new skb for index %d.\n",
rx_ring->cq_id, sbq_desc->index);
- skb = netdev_alloc_skb(qdev->ndev, SMALL_BUFFER_SIZE);
+ skb = __netdev_alloc_skb(qdev->ndev, SMALL_BUFFER_SIZE, gfp);
if (!skb)
return -ENOMEM;
skb_reserve(skb, QLGE_SB_PAD);
@@ -1062,7 +1062,7 @@ static int qlge_refill_sb(struct rx_ring *rx_ring,
/* return 0 or negative error */
static int qlge_refill_lb(struct rx_ring *rx_ring,
- struct qlge_bq_desc *lbq_desc)
+ struct qlge_bq_desc *lbq_desc, gfp_t gfp)
{
struct ql_adapter *qdev = rx_ring->qdev;
struct qlge_page_chunk *master_chunk = &rx_ring->master_chunk;
@@ -1071,8 +1071,7 @@ static int qlge_refill_lb(struct rx_ring *rx_ring,
struct page *page;
dma_addr_t dma_addr;
- page = alloc_pages(__GFP_COMP | GFP_ATOMIC,
- qdev->lbq_buf_order);
+ page = alloc_pages(gfp | __GFP_COMP, qdev->lbq_buf_order);
if (unlikely(!page))
return -ENOMEM;
dma_addr = pci_map_page(qdev->pdev, page, 0,
@@ -1109,33 +1108,33 @@ static int qlge_refill_lb(struct rx_ring *rx_ring,
return 0;
}
-static void qlge_refill_bq(struct qlge_bq *bq)
+/* return 0 or negative error */
+static int qlge_refill_bq(struct qlge_bq *bq, gfp_t gfp)
{
struct rx_ring *rx_ring = QLGE_BQ_CONTAINER(bq);
struct ql_adapter *qdev = rx_ring->qdev;
struct qlge_bq_desc *bq_desc;
int refill_count;
+ int retval;
int i;
refill_count = QLGE_BQ_WRAP(QLGE_BQ_ALIGN(bq->next_to_clean - 1) -
bq->next_to_use);
if (!refill_count)
- return;
+ return 0;
i = bq->next_to_use;
bq_desc = &bq->queue[i];
i -= QLGE_BQ_LEN;
do {
- int retval;
-
netif_printk(qdev, rx_status, KERN_DEBUG, qdev->ndev,
"ring %u %s: try cleaning idx %d\n",
rx_ring->cq_id, bq_type_name[bq->type], i);
if (bq->type == QLGE_SB)
- retval = qlge_refill_sb(rx_ring, bq_desc);
+ retval = qlge_refill_sb(rx_ring, bq_desc, gfp);
else
- retval = qlge_refill_lb(rx_ring, bq_desc);
+ retval = qlge_refill_lb(rx_ring, bq_desc, gfp);
if (retval < 0) {
netif_err(qdev, ifup, qdev->ndev,
"ring %u %s: Could not get a page chunk, idx %d\n",
@@ -1163,12 +1162,52 @@ static void qlge_refill_bq(struct qlge_bq *bq)
}
bq->next_to_use = i;
}
+
+ return retval;
+}
+
+static void ql_update_buffer_queues(struct rx_ring *rx_ring, gfp_t gfp,
+ unsigned long delay)
+{
+ bool sbq_fail, lbq_fail;
+
+ sbq_fail = !!qlge_refill_bq(&rx_ring->sbq, gfp);
+ lbq_fail = !!qlge_refill_bq(&rx_ring->lbq, gfp);
+
+ /* Minimum number of buffers needed to be able to receive at least one
+ * frame of any format:
+ * sbq: 1 for header + 1 for data
+ * lbq: mtu 9000 / lb size
+ * Below this, the queue might stall.
+ */
+ if ((sbq_fail && QLGE_BQ_HW_OWNED(&rx_ring->sbq) < 2) ||
+ (lbq_fail && QLGE_BQ_HW_OWNED(&rx_ring->lbq) <
+ DIV_ROUND_UP(9000, LARGE_BUFFER_MAX_SIZE)))
+ /* Allocations can take a long time in certain cases (ex.
+ * reclaim). Therefore, use a workqueue for long-running
+ * work items.
+ */
+ queue_delayed_work_on(smp_processor_id(), system_long_wq,
+ &rx_ring->refill_work, delay);
}
-static void ql_update_buffer_queues(struct rx_ring *rx_ring)
+static void qlge_slow_refill(struct work_struct *work)
{
- qlge_refill_bq(&rx_ring->sbq);
- qlge_refill_bq(&rx_ring->lbq);
+ struct rx_ring *rx_ring = container_of(work, struct rx_ring,
+ refill_work.work);
+ struct napi_struct *napi = &rx_ring->napi;
+
+ napi_disable(napi);
+ ql_update_buffer_queues(rx_ring, GFP_KERNEL, HZ / 2);
+ napi_enable(napi);
+
+ local_bh_disable();
+ /* napi_disable() might have prevented incomplete napi work from being
+ * rescheduled.
+ */
+ napi_schedule(napi);
+ /* trigger softirq processing */
+ local_bh_enable();
}
/* Unmaps tx buffers. Can be called from send() if a pci mapping
@@ -2168,7 +2207,7 @@ static int ql_clean_inbound_rx_ring(struct rx_ring *rx_ring, int budget)
if (count == budget)
break;
}
- ql_update_buffer_queues(rx_ring);
+ ql_update_buffer_queues(rx_ring, GFP_ATOMIC, 0);
ql_write_cq_idx(rx_ring);
return count;
}
@@ -2776,7 +2815,8 @@ static void ql_alloc_rx_buffers(struct ql_adapter *qdev)
int i;
for (i = 0; i < qdev->rss_ring_count; i++)
- ql_update_buffer_queues(&qdev->rx_ring[i]);
+ ql_update_buffer_queues(&qdev->rx_ring[i], GFP_KERNEL,
+ HZ / 2);
}
static int qlge_init_bq(struct qlge_bq *bq)
@@ -3870,6 +3910,7 @@ static int ql_get_adapter_resources(struct ql_adapter *qdev)
static int qlge_close(struct net_device *ndev)
{
struct ql_adapter *qdev = netdev_priv(ndev);
+ int i;
/* If we hit pci_channel_io_perm_failure
* failure condition, then we already
@@ -3887,6 +3928,11 @@ static int qlge_close(struct net_device *ndev)
*/
while (!test_bit(QL_ADAPTER_UP, &qdev->flags))
msleep(1);
+
+ /* Make sure refill_work doesn't re-enable napi */
+ for (i = 0; i < qdev->rss_ring_count; i++)
+ cancel_delayed_work_sync(&qdev->rx_ring[i].refill_work);
+
ql_adapter_down(qdev);
ql_release_adapter_resources(qdev);
return 0;
@@ -3953,6 +3999,8 @@ static int ql_configure_rings(struct ql_adapter *qdev)
rx_ring->cq_len * sizeof(struct ql_net_rsp_iocb);
rx_ring->lbq.type = QLGE_LB;
rx_ring->sbq.type = QLGE_SB;
+ INIT_DELAYED_WORK(&rx_ring->refill_work,
+ &qlge_slow_refill);
} else {
/*
* Outbound queue handles outbound completions only.
--
2.21.0
^ permalink raw reply related [flat|nested] 46+ messages in thread* RE: [EXT] [PATCH net-next 16/16] qlge: Refill empty buffer queues from wq
2019-06-17 7:48 ` [PATCH net-next 16/16] qlge: Refill empty buffer queues from wq Benjamin Poirier
@ 2019-06-27 14:18 ` Manish Chopra
2019-07-10 1:18 ` Benjamin Poirier
0 siblings, 1 reply; 46+ messages in thread
From: Manish Chopra @ 2019-06-27 14:18 UTC (permalink / raw)
To: Benjamin Poirier, GR-Linux-NIC-Dev, netdev
> -----Original Message-----
> From: Benjamin Poirier <bpoirier@suse.com>
> Sent: Monday, June 17, 2019 1:19 PM
> To: Manish Chopra <manishc@marvell.com>; GR-Linux-NIC-Dev <GR-Linux-
> NIC-Dev@marvell.com>; netdev@vger.kernel.org
> Subject: [EXT] [PATCH net-next 16/16] qlge: Refill empty buffer queues from
> wq
>
> External Email
>
> ----------------------------------------------------------------------
> When operating at mtu 9000, qlge does order-1 allocations for rx buffers in
> atomic context. This is especially unreliable when free memory is low or
> fragmented. Add an approach similar to commit 3161e453e496 ("virtio: net
> refill on out-of-memory") to qlge so that the device doesn't lock up if there
> are allocation failures.
>
> Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
> ---
> drivers/net/ethernet/qlogic/qlge/qlge.h | 8 ++
> drivers/net/ethernet/qlogic/qlge/qlge_main.c | 80 ++++++++++++++++----
> 2 files changed, 72 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/net/ethernet/qlogic/qlge/qlge.h
> b/drivers/net/ethernet/qlogic/qlge/qlge.h
> index 1d90b32f6285..9c4d933c1ff7 100644
> --- a/drivers/net/ethernet/qlogic/qlge/qlge.h
> +++ b/drivers/net/ethernet/qlogic/qlge/qlge.h
> @@ -1453,6 +1453,13 @@ struct qlge_bq {
>
> #define QLGE_BQ_WRAP(index) ((index) & (QLGE_BQ_LEN - 1))
>
> +#define QLGE_BQ_HW_OWNED(bq) \
> +({ \
> + typeof(bq) _bq = bq; \
> + QLGE_BQ_WRAP(QLGE_BQ_ALIGN((_bq)->next_to_use) - \
> + (_bq)->next_to_clean); \
> +})
> +
> struct rx_ring {
> struct cqicb cqicb; /* The chip's completion queue init control
> block. */
>
> @@ -1480,6 +1487,7 @@ struct rx_ring {
> /* Misc. handler elements. */
> u32 irq; /* Which vector this ring is assigned. */
> u32 cpu; /* Which CPU this should run on. */
> + struct delayed_work refill_work;
> char name[IFNAMSIZ + 5];
> struct napi_struct napi;
> u8 reserved;
> diff --git a/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> index 7db4c31c9cc4..a13bda566187 100644
> --- a/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> +++ b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> @@ -1029,7 +1029,7 @@ static const char * const bq_type_name[] = {
>
> /* return 0 or negative error */
> static int qlge_refill_sb(struct rx_ring *rx_ring,
> - struct qlge_bq_desc *sbq_desc)
> + struct qlge_bq_desc *sbq_desc, gfp_t gfp)
> {
> struct ql_adapter *qdev = rx_ring->qdev;
> struct sk_buff *skb;
> @@ -1041,7 +1041,7 @@ static int qlge_refill_sb(struct rx_ring *rx_ring,
> "ring %u sbq: getting new skb for index %d.\n",
> rx_ring->cq_id, sbq_desc->index);
>
> - skb = netdev_alloc_skb(qdev->ndev, SMALL_BUFFER_SIZE);
> + skb = __netdev_alloc_skb(qdev->ndev, SMALL_BUFFER_SIZE, gfp);
> if (!skb)
> return -ENOMEM;
> skb_reserve(skb, QLGE_SB_PAD);
> @@ -1062,7 +1062,7 @@ static int qlge_refill_sb(struct rx_ring *rx_ring,
>
> /* return 0 or negative error */
> static int qlge_refill_lb(struct rx_ring *rx_ring,
> - struct qlge_bq_desc *lbq_desc)
> + struct qlge_bq_desc *lbq_desc, gfp_t gfp)
> {
> struct ql_adapter *qdev = rx_ring->qdev;
> struct qlge_page_chunk *master_chunk = &rx_ring->master_chunk;
> @@ -1071,8 +1071,7 @@ static int qlge_refill_lb(struct rx_ring *rx_ring,
> struct page *page;
> dma_addr_t dma_addr;
>
> - page = alloc_pages(__GFP_COMP | GFP_ATOMIC,
> - qdev->lbq_buf_order);
> + page = alloc_pages(gfp | __GFP_COMP, qdev-
> >lbq_buf_order);
> if (unlikely(!page))
> return -ENOMEM;
> dma_addr = pci_map_page(qdev->pdev, page, 0, @@ -
> 1109,33 +1108,33 @@ static int qlge_refill_lb(struct rx_ring *rx_ring,
> return 0;
> }
>
> -static void qlge_refill_bq(struct qlge_bq *bq)
> +/* return 0 or negative error */
> +static int qlge_refill_bq(struct qlge_bq *bq, gfp_t gfp)
> {
> struct rx_ring *rx_ring = QLGE_BQ_CONTAINER(bq);
> struct ql_adapter *qdev = rx_ring->qdev;
> struct qlge_bq_desc *bq_desc;
> int refill_count;
> + int retval;
> int i;
>
> refill_count = QLGE_BQ_WRAP(QLGE_BQ_ALIGN(bq->next_to_clean -
> 1) -
> bq->next_to_use);
> if (!refill_count)
> - return;
> + return 0;
>
> i = bq->next_to_use;
> bq_desc = &bq->queue[i];
> i -= QLGE_BQ_LEN;
> do {
> - int retval;
> -
> netif_printk(qdev, rx_status, KERN_DEBUG, qdev->ndev,
> "ring %u %s: try cleaning idx %d\n",
> rx_ring->cq_id, bq_type_name[bq->type], i);
>
> if (bq->type == QLGE_SB)
> - retval = qlge_refill_sb(rx_ring, bq_desc);
> + retval = qlge_refill_sb(rx_ring, bq_desc, gfp);
> else
> - retval = qlge_refill_lb(rx_ring, bq_desc);
> + retval = qlge_refill_lb(rx_ring, bq_desc, gfp);
> if (retval < 0) {
> netif_err(qdev, ifup, qdev->ndev,
> "ring %u %s: Could not get a page chunk, idx
> %d\n", @@ -1163,12 +1162,52 @@ static void qlge_refill_bq(struct qlge_bq
> *bq)
> }
> bq->next_to_use = i;
> }
> +
> + return retval;
> +}
> +
> +static void ql_update_buffer_queues(struct rx_ring *rx_ring, gfp_t gfp,
> + unsigned long delay)
> +{
> + bool sbq_fail, lbq_fail;
> +
> + sbq_fail = !!qlge_refill_bq(&rx_ring->sbq, gfp);
> + lbq_fail = !!qlge_refill_bq(&rx_ring->lbq, gfp);
> +
> + /* Minimum number of buffers needed to be able to receive at least
> one
> + * frame of any format:
> + * sbq: 1 for header + 1 for data
> + * lbq: mtu 9000 / lb size
> + * Below this, the queue might stall.
> + */
> + if ((sbq_fail && QLGE_BQ_HW_OWNED(&rx_ring->sbq) < 2) ||
> + (lbq_fail && QLGE_BQ_HW_OWNED(&rx_ring->lbq) <
> + DIV_ROUND_UP(9000, LARGE_BUFFER_MAX_SIZE)))
> + /* Allocations can take a long time in certain cases (ex.
> + * reclaim). Therefore, use a workqueue for long-running
> + * work items.
> + */
> + queue_delayed_work_on(smp_processor_id(),
> system_long_wq,
> + &rx_ring->refill_work, delay);
> }
>
This is probably going to mess up when at the interface load time (qlge_open()) allocation failure occurs, in such cases we don't really want to re-try allocations
using refill_work but rather simply fail the interface load. Just to make sure here in such cases it shouldn't lead to kernel panic etc. while completing qlge_open() and
leaving refill_work executing in background. Or probably handle such allocation failures from the napi context and schedule refill_work from there.
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [EXT] [PATCH net-next 16/16] qlge: Refill empty buffer queues from wq
2019-06-27 14:18 ` [EXT] " Manish Chopra
@ 2019-07-10 1:18 ` Benjamin Poirier
0 siblings, 0 replies; 46+ messages in thread
From: Benjamin Poirier @ 2019-07-10 1:18 UTC (permalink / raw)
To: Manish Chopra; +Cc: GR-Linux-NIC-Dev, netdev
On 2019/06/27 14:18, Manish Chopra wrote:
> > -----Original Message-----
> > From: Benjamin Poirier <bpoirier@suse.com>
> > Sent: Monday, June 17, 2019 1:19 PM
> > To: Manish Chopra <manishc@marvell.com>; GR-Linux-NIC-Dev <GR-Linux-
> > NIC-Dev@marvell.com>; netdev@vger.kernel.org
> > Subject: [EXT] [PATCH net-next 16/16] qlge: Refill empty buffer queues from
> > wq
> >
> > External Email
> >
> > ----------------------------------------------------------------------
> > When operating at mtu 9000, qlge does order-1 allocations for rx buffers in
> > atomic context. This is especially unreliable when free memory is low or
> > fragmented. Add an approach similar to commit 3161e453e496 ("virtio: net
> > refill on out-of-memory") to qlge so that the device doesn't lock up if there
> > are allocation failures.
> >
[...]
> > +
> > +static void ql_update_buffer_queues(struct rx_ring *rx_ring, gfp_t gfp,
> > + unsigned long delay)
> > +{
> > + bool sbq_fail, lbq_fail;
> > +
> > + sbq_fail = !!qlge_refill_bq(&rx_ring->sbq, gfp);
> > + lbq_fail = !!qlge_refill_bq(&rx_ring->lbq, gfp);
> > +
> > + /* Minimum number of buffers needed to be able to receive at least
> > one
> > + * frame of any format:
> > + * sbq: 1 for header + 1 for data
> > + * lbq: mtu 9000 / lb size
> > + * Below this, the queue might stall.
> > + */
> > + if ((sbq_fail && QLGE_BQ_HW_OWNED(&rx_ring->sbq) < 2) ||
> > + (lbq_fail && QLGE_BQ_HW_OWNED(&rx_ring->lbq) <
> > + DIV_ROUND_UP(9000, LARGE_BUFFER_MAX_SIZE)))
> > + /* Allocations can take a long time in certain cases (ex.
> > + * reclaim). Therefore, use a workqueue for long-running
> > + * work items.
> > + */
> > + queue_delayed_work_on(smp_processor_id(),
> > system_long_wq,
> > + &rx_ring->refill_work, delay);
> > }
> >
>
> This is probably going to mess up when at the interface load time (qlge_open()) allocation failure occurs, in such cases we don't really want to re-try allocations
> using refill_work but rather simply fail the interface load.
Why would you want to turn a recoverable failure into a fatal failure?
In case of allocation failure at ndo_open time, allocations are retried
later from a workqueue. Meanwhile, the device can use the available rx
buffers (if any could be allocated at all).
> Just to make sure here in such cases it shouldn't lead to kernel panic etc. while completing qlge_open() and
> leaving refill_work executing in background. Or probably handle such allocation failures from the napi context and schedule refill_work from there.
>
I've just tested allocation failures at open time and didn't find
problems; with mtu 9000, using bcc, for example:
tools/inject.py -P 0.5 -c 100 alloc_page "should_fail_alloc_page(gfp_t gfp_mask, unsigned int order) (order == 1) => qlge_refill_bq()"
What exact scenario do you have in mind that's going to lead to
problems? Please try it out and describe it precisely.
^ permalink raw reply [flat|nested] 46+ messages in thread
* RE: [PATCH net-next 01/16] qlge: Remove irq_cnt
2019-06-17 7:48 [PATCH net-next 01/16] qlge: Remove irq_cnt Benjamin Poirier
` (14 preceding siblings ...)
2019-06-17 7:48 ` [PATCH net-next 16/16] qlge: Refill empty buffer queues from wq Benjamin Poirier
@ 2019-06-17 16:49 ` Manish Chopra
2019-06-26 8:59 ` Manish Chopra
2019-07-15 1:40 ` Benjamin Poirier
17 siblings, 0 replies; 46+ messages in thread
From: Manish Chopra @ 2019-06-17 16:49 UTC (permalink / raw)
To: Benjamin Poirier, GR-Linux-NIC-Dev, netdev
> -----Original Message-----
> From: Benjamin Poirier <bpoirier@suse.com>
> Sent: Monday, June 17, 2019 1:19 PM
> To: Manish Chopra <manishc@marvell.com>; GR-Linux-NIC-Dev <GR-Linux-
> NIC-Dev@marvell.com>; netdev@vger.kernel.org
> Subject: [PATCH net-next 01/16] qlge: Remove irq_cnt
>
> qlge uses an irq enable/disable refcounting scheme that is:
> * poorly implemented
> Uses a spin_lock to protect accesses to the irq_cnt atomic variable
> * buggy
> Breaks when there is not a 1:1 sequence of irq - napi_poll, such as
> when using SO_BUSY_POLL.
> * unnecessary
> The purpose or irq_cnt is to reduce irq control writes when
> multiple work items result from one irq: the irq is re-enabled
> after all work is done.
> Analysis of the irq handler shows that there is only one case where
> there might be two workers scheduled at once, and those have
> separate irq masking bits.
>
> Therefore, remove irq_cnt.
>
> Additionally, we get a performance improvement:
> perf stat -e cycles -a -r5 super_netperf 100 -H 192.168.33.1 -t TCP_RR
>
> Before:
> 628560
> 628056
> 622103
> 622744
> 627202
> [...]
> 268,803,947,669 cycles ( +- 0.09% )
>
> After:
> 636300
> 634106
> 634984
> 638555
> 634188
> [...]
> 259,237,291,449 cycles ( +- 0.19% )
>
> Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
> ---
> drivers/net/ethernet/qlogic/qlge/qlge.h | 7 --
> drivers/net/ethernet/qlogic/qlge/qlge_main.c | 98 ++++++--------------
> drivers/net/ethernet/qlogic/qlge/qlge_mpi.c | 1 -
> 3 files changed, 27 insertions(+), 79 deletions(-)
>
> diff --git a/drivers/net/ethernet/qlogic/qlge/qlge.h
> b/drivers/net/ethernet/qlogic/qlge/qlge.h
> index ad7c5eb8a3b6..5d9a36deda08 100644
> --- a/drivers/net/ethernet/qlogic/qlge/qlge.h
> +++ b/drivers/net/ethernet/qlogic/qlge/qlge.h
> @@ -1982,11 +1982,6 @@ struct intr_context {
> u32 intr_dis_mask; /* value/mask used to disable this intr */
> u32 intr_read_mask; /* value/mask used to read this intr */
> char name[IFNAMSIZ * 2];
> - atomic_t irq_cnt; /* irq_cnt is used in single vector
> - * environment. It's incremented for each
> - * irq handler that is scheduled. When each
> - * handler finishes it decrements irq_cnt and
> - * enables interrupts if it's zero. */
> irq_handler_t handler;
> };
>
> @@ -2074,7 +2069,6 @@ struct ql_adapter {
> u32 port; /* Port number this adapter */
>
> spinlock_t adapter_lock;
> - spinlock_t hw_lock;
> spinlock_t stats_lock;
>
> /* PCI Bus Relative Register Addresses */ @@ -2235,7 +2229,6 @@
> void ql_mpi_reset_work(struct work_struct *work); void
> ql_mpi_core_to_log(struct work_struct *work); int ql_wait_reg_rdy(struct
> ql_adapter *qdev, u32 reg, u32 bit, u32 ebit); void
> ql_queue_asic_error(struct ql_adapter *qdev);
> -u32 ql_enable_completion_interrupt(struct ql_adapter *qdev, u32 intr);
> void ql_set_ethtool_ops(struct net_device *ndev); int
> ql_read_xgmac_reg64(struct ql_adapter *qdev, u32 reg, u64 *data); void
> ql_mpi_idc_work(struct work_struct *work); diff --git
> a/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> index 6cae33072496..0bfbe11db795 100644
> --- a/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> +++ b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> @@ -625,75 +625,26 @@ static void ql_disable_interrupts(struct ql_adapter
> *qdev)
> ql_write32(qdev, INTR_EN, (INTR_EN_EI << 16)); }
>
> -/* If we're running with multiple MSI-X vectors then we enable on the fly.
> - * Otherwise, we may have multiple outstanding workers and don't want to
> - * enable until the last one finishes. In this case, the irq_cnt gets
> - * incremented every time we queue a worker and decremented every time
> - * a worker finishes. Once it hits zero we enable the interrupt.
> - */
> -u32 ql_enable_completion_interrupt(struct ql_adapter *qdev, u32 intr)
> +static void ql_enable_completion_interrupt(struct ql_adapter *qdev, u32
> +intr)
> {
> - u32 var = 0;
> - unsigned long hw_flags = 0;
> - struct intr_context *ctx = qdev->intr_context + intr;
> -
> - if (likely(test_bit(QL_MSIX_ENABLED, &qdev->flags) && intr)) {
> - /* Always enable if we're MSIX multi interrupts and
> - * it's not the default (zeroeth) interrupt.
> - */
> - ql_write32(qdev, INTR_EN,
> - ctx->intr_en_mask);
> - var = ql_read32(qdev, STS);
> - return var;
> - }
> + struct intr_context *ctx = &qdev->intr_context[intr];
>
> - spin_lock_irqsave(&qdev->hw_lock, hw_flags);
> - if (atomic_dec_and_test(&ctx->irq_cnt)) {
> - ql_write32(qdev, INTR_EN,
> - ctx->intr_en_mask);
> - var = ql_read32(qdev, STS);
> - }
> - spin_unlock_irqrestore(&qdev->hw_lock, hw_flags);
> - return var;
> + ql_write32(qdev, INTR_EN, ctx->intr_en_mask);
> }
>
> -static u32 ql_disable_completion_interrupt(struct ql_adapter *qdev, u32
> intr)
> +static void ql_disable_completion_interrupt(struct ql_adapter *qdev,
> +u32 intr)
> {
> - u32 var = 0;
> - struct intr_context *ctx;
> + struct intr_context *ctx = &qdev->intr_context[intr];
>
> - /* HW disables for us if we're MSIX multi interrupts and
> - * it's not the default (zeroeth) interrupt.
> - */
> - if (likely(test_bit(QL_MSIX_ENABLED, &qdev->flags) && intr))
> - return 0;
> -
> - ctx = qdev->intr_context + intr;
> - spin_lock(&qdev->hw_lock);
> - if (!atomic_read(&ctx->irq_cnt)) {
> - ql_write32(qdev, INTR_EN,
> - ctx->intr_dis_mask);
> - var = ql_read32(qdev, STS);
> - }
> - atomic_inc(&ctx->irq_cnt);
> - spin_unlock(&qdev->hw_lock);
> - return var;
> + ql_write32(qdev, INTR_EN, ctx->intr_dis_mask);
> }
>
> static void ql_enable_all_completion_interrupts(struct ql_adapter *qdev) {
> int i;
> - for (i = 0; i < qdev->intr_count; i++) {
> - /* The enable call does a atomic_dec_and_test
> - * and enables only if the result is zero.
> - * So we precharge it here.
> - */
> - if (unlikely(!test_bit(QL_MSIX_ENABLED, &qdev->flags) ||
> - i == 0))
> - atomic_set(&qdev->intr_context[i].irq_cnt, 1);
> - ql_enable_completion_interrupt(qdev, i);
> - }
>
> + for (i = 0; i < qdev->intr_count; i++)
> + ql_enable_completion_interrupt(qdev, i);
> }
>
> static int ql_validate_flash(struct ql_adapter *qdev, u32 size, const char
> *str) @@ -2500,21 +2451,22 @@ static irqreturn_t qlge_isr(int irq, void
> *dev_id)
> u32 var;
> int work_done = 0;
>
> - spin_lock(&qdev->hw_lock);
> - if (atomic_read(&qdev->intr_context[0].irq_cnt)) {
> - netif_printk(qdev, intr, KERN_DEBUG, qdev->ndev,
> - "Shared Interrupt, Not ours!\n");
> - spin_unlock(&qdev->hw_lock);
> - return IRQ_NONE;
> - }
> - spin_unlock(&qdev->hw_lock);
> + /* Experience shows that when using INTx interrupts, the device
> does
> + * not always auto-mask the interrupt.
> + * When using MSI mode, the interrupt must be explicitly disabled
> + * (even though it is auto-masked), otherwise a later command to
> + * enable it is not effective.
> + */
> + if (!test_bit(QL_MSIX_ENABLED, &qdev->flags))
> + ql_disable_completion_interrupt(qdev, 0);
>
> - var = ql_disable_completion_interrupt(qdev, intr_context->intr);
> + var = ql_read32(qdev, STS);
>
> /*
> * Check for fatal error.
> */
> if (var & STS_FE) {
> + ql_disable_completion_interrupt(qdev, 0);
> ql_queue_asic_error(qdev);
> netdev_err(qdev->ndev, "Got fatal error, STS = %x.\n", var);
> var = ql_read32(qdev, ERR_STS);
> @@ -2534,7 +2486,6 @@ static irqreturn_t qlge_isr(int irq, void *dev_id)
> */
> netif_err(qdev, intr, qdev->ndev,
> "Got MPI processor interrupt.\n");
> - ql_disable_completion_interrupt(qdev, intr_context->intr);
> ql_write32(qdev, INTR_MASK, (INTR_MASK_PI << 16));
> queue_delayed_work_on(smp_processor_id(),
> qdev->workqueue, &qdev->mpi_work, 0);
> @@ -2550,11 +2501,18 @@ static irqreturn_t qlge_isr(int irq, void *dev_id)
> if (var & intr_context->irq_mask) {
> netif_info(qdev, intr, qdev->ndev,
> "Waking handler for rx_ring[0].\n");
> - ql_disable_completion_interrupt(qdev, intr_context->intr);
> napi_schedule(&rx_ring->napi);
> work_done++;
> + } else {
> + /* Experience shows that the device sometimes signals an
> + * interrupt but no work is scheduled from this function.
> + * Nevertheless, the interrupt is auto-masked. Therefore, we
> + * systematically re-enable the interrupt if we didn't
> + * schedule napi.
> + */
> + ql_enable_completion_interrupt(qdev, 0);
> }
> - ql_enable_completion_interrupt(qdev, intr_context->intr);
> +
> return work_done ? IRQ_HANDLED : IRQ_NONE; }
>
> @@ -3557,7 +3515,6 @@ static int ql_request_irq(struct ql_adapter *qdev)
> ql_resolve_queues_to_irqs(qdev);
>
> for (i = 0; i < qdev->intr_count; i++, intr_context++) {
> - atomic_set(&intr_context->irq_cnt, 0);
> if (test_bit(QL_MSIX_ENABLED, &qdev->flags)) {
> status = request_irq(qdev->msi_x_entry[i].vector,
> intr_context->handler,
> @@ -4642,7 +4599,6 @@ static int ql_init_device(struct pci_dev *pdev,
> struct net_device *ndev,
> goto err_out2;
> }
> qdev->msg_enable = netif_msg_init(debug, default_msg);
> - spin_lock_init(&qdev->hw_lock);
> spin_lock_init(&qdev->stats_lock);
>
> if (qlge_mpi_coredump) {
> diff --git a/drivers/net/ethernet/qlogic/qlge/qlge_mpi.c
> b/drivers/net/ethernet/qlogic/qlge/qlge_mpi.c
> index 957c72985a06..9e422bbbb6ab 100644
> --- a/drivers/net/ethernet/qlogic/qlge/qlge_mpi.c
> +++ b/drivers/net/ethernet/qlogic/qlge/qlge_mpi.c
> @@ -1257,7 +1257,6 @@ void ql_mpi_work(struct work_struct *work)
> /* End polled mode for MPI */
> ql_write32(qdev, INTR_MASK, (INTR_MASK_PI << 16) |
> INTR_MASK_PI);
> mutex_unlock(&qdev->mpi_mutex);
> - ql_enable_completion_interrupt(qdev, 0);
> }
>
> void ql_mpi_reset_work(struct work_struct *work)
> --
> 2.21.0
Hello Benjamin,
Just FYI. I am OOO for a week, so reviewing and testing these patches will take time.
Thanks,
Manish
^ permalink raw reply [flat|nested] 46+ messages in thread* RE: [PATCH net-next 01/16] qlge: Remove irq_cnt
2019-06-17 7:48 [PATCH net-next 01/16] qlge: Remove irq_cnt Benjamin Poirier
` (15 preceding siblings ...)
2019-06-17 16:49 ` [PATCH net-next 01/16] qlge: Remove irq_cnt Manish Chopra
@ 2019-06-26 8:59 ` Manish Chopra
2019-06-26 11:36 ` Benjamin Poirier
2019-07-15 1:40 ` Benjamin Poirier
17 siblings, 1 reply; 46+ messages in thread
From: Manish Chopra @ 2019-06-26 8:59 UTC (permalink / raw)
To: Benjamin Poirier, GR-Linux-NIC-Dev, netdev
> -----Original Message-----
> From: Benjamin Poirier <bpoirier@suse.com>
> Sent: Monday, June 17, 2019 1:19 PM
> To: Manish Chopra <manishc@marvell.com>; GR-Linux-NIC-Dev <GR-Linux-
> NIC-Dev@marvell.com>; netdev@vger.kernel.org
> Subject: [PATCH net-next 01/16] qlge: Remove irq_cnt
>
> qlge uses an irq enable/disable refcounting scheme that is:
> * poorly implemented
> Uses a spin_lock to protect accesses to the irq_cnt atomic variable
> * buggy
> Breaks when there is not a 1:1 sequence of irq - napi_poll, such as
> when using SO_BUSY_POLL.
> * unnecessary
> The purpose or irq_cnt is to reduce irq control writes when
> multiple work items result from one irq: the irq is re-enabled
> after all work is done.
> Analysis of the irq handler shows that there is only one case where
> there might be two workers scheduled at once, and those have
> separate irq masking bits.
I believe you are talking about here for asic error recovery worker and MPI worker.
Which separate IRQ masking bits are you referring here ?
> static int ql_validate_flash(struct ql_adapter *qdev, u32 size, const char *str)
> @@ -2500,21 +2451,22 @@ static irqreturn_t qlge_isr(int irq, void *dev_id)
> u32 var;
> int work_done = 0;
>
> - spin_lock(&qdev->hw_lock);
> - if (atomic_read(&qdev->intr_context[0].irq_cnt)) {
> - netif_printk(qdev, intr, KERN_DEBUG, qdev->ndev,
> - "Shared Interrupt, Not ours!\n");
> - spin_unlock(&qdev->hw_lock);
> - return IRQ_NONE;
> - }
> - spin_unlock(&qdev->hw_lock);
> + /* Experience shows that when using INTx interrupts, the device does
> + * not always auto-mask the interrupt.
> + * When using MSI mode, the interrupt must be explicitly disabled
> + * (even though it is auto-masked), otherwise a later command to
> + * enable it is not effective.
> + */
> + if (!test_bit(QL_MSIX_ENABLED, &qdev->flags))
> + ql_disable_completion_interrupt(qdev, 0);
Current code is disabling completion interrupt in case of MSI-X zeroth vector.
This change will break that behavior. Shouldn't zeroth vector in case of MSI-X also disable completion interrupt ?
If not, please explain why ?
>
> - var = ql_disable_completion_interrupt(qdev, intr_context->intr);
> + var = ql_read32(qdev, STS);
>
> /*
> * Check for fatal error.
> */
> if (var & STS_FE) {
> + ql_disable_completion_interrupt(qdev, 0);
Why need to do it again here ? if before this it can disable completion interrupt for INT-X case and MSI-X zeroth vector case ?
> ql_queue_asic_error(qdev);
> netdev_err(qdev->ndev, "Got fatal error, STS = %x.\n", var);
> var = ql_read32(qdev, ERR_STS);
> @@ -2534,7 +2486,6 @@ static irqreturn_t qlge_isr(int irq, void *dev_id)
> */
> netif_err(qdev, intr, qdev->ndev,
> "Got MPI processor interrupt.\n");
> - ql_disable_completion_interrupt(qdev, intr_context->intr);
Why disable interrupt is not required here ? While in case of Fatal error case above ql_disable_completion_interrupt() is being called ?
Also, in case of MSI-X zeroth vector it will not disable completion interrupt but at the end, it will end of qlge_isr() enabling completion interrupt.
Seems like disabling and enabling might not be in sync in case of MSI-X zeroth vector.
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH net-next 01/16] qlge: Remove irq_cnt
2019-06-26 8:59 ` Manish Chopra
@ 2019-06-26 11:36 ` Benjamin Poirier
2019-06-26 13:21 ` Manish Chopra
0 siblings, 1 reply; 46+ messages in thread
From: Benjamin Poirier @ 2019-06-26 11:36 UTC (permalink / raw)
To: Manish Chopra; +Cc: GR-Linux-NIC-Dev, netdev
On 2019/06/26 08:59, Manish Chopra wrote:
> > -----Original Message-----
> > From: Benjamin Poirier <bpoirier@suse.com>
> > Sent: Monday, June 17, 2019 1:19 PM
> > To: Manish Chopra <manishc@marvell.com>; GR-Linux-NIC-Dev <GR-Linux-
> > NIC-Dev@marvell.com>; netdev@vger.kernel.org
> > Subject: [PATCH net-next 01/16] qlge: Remove irq_cnt
> >
> > qlge uses an irq enable/disable refcounting scheme that is:
> > * poorly implemented
> > Uses a spin_lock to protect accesses to the irq_cnt atomic variable
> > * buggy
> > Breaks when there is not a 1:1 sequence of irq - napi_poll, such as
> > when using SO_BUSY_POLL.
> > * unnecessary
> > The purpose or irq_cnt is to reduce irq control writes when
> > multiple work items result from one irq: the irq is re-enabled
> > after all work is done.
> > Analysis of the irq handler shows that there is only one case where
> > there might be two workers scheduled at once, and those have
> > separate irq masking bits.
>
> I believe you are talking about here for asic error recovery worker and MPI worker.
> Which separate IRQ masking bits are you referring here ?
INTR_EN with intr_dis_mask for completion interrupts
INTR_MASK bit INTR_MASK_PI for mpi interrupts
> > static int ql_validate_flash(struct ql_adapter *qdev, u32 size, const char *str)
> > @@ -2500,21 +2451,22 @@ static irqreturn_t qlge_isr(int irq, void *dev_id)
> > u32 var;
> > int work_done = 0;
> >
> > - spin_lock(&qdev->hw_lock);
> > - if (atomic_read(&qdev->intr_context[0].irq_cnt)) {
> > - netif_printk(qdev, intr, KERN_DEBUG, qdev->ndev,
> > - "Shared Interrupt, Not ours!\n");
> > - spin_unlock(&qdev->hw_lock);
> > - return IRQ_NONE;
> > - }
> > - spin_unlock(&qdev->hw_lock);
> > + /* Experience shows that when using INTx interrupts, the device does
> > + * not always auto-mask the interrupt.
> > + * When using MSI mode, the interrupt must be explicitly disabled
> > + * (even though it is auto-masked), otherwise a later command to
> > + * enable it is not effective.
> > + */
> > + if (!test_bit(QL_MSIX_ENABLED, &qdev->flags))
> > + ql_disable_completion_interrupt(qdev, 0);
>
> Current code is disabling completion interrupt in case of MSI-X zeroth vector.
> This change will break that behavior. Shouldn't zeroth vector in case of MSI-X also disable completion interrupt ?
> If not, please explain why ?
In msix mode there's no need to explicitly disable completion
interrupts, they are reliably auto-masked, according to my observations.
I tested this on two QLE8142 adapters.
Do you have reason to believe this might not always be the case?
>
> >
> > - var = ql_disable_completion_interrupt(qdev, intr_context->intr);
> > + var = ql_read32(qdev, STS);
> >
> > /*
> > * Check for fatal error.
> > */
> > if (var & STS_FE) {
> > + ql_disable_completion_interrupt(qdev, 0);
>
> Why need to do it again here ? if before this it can disable completion interrupt for INT-X case and MSI-X zeroth vector case ?
I couldn't test this code path, so I preserved the original behavior.
>
> > ql_queue_asic_error(qdev);
> > netdev_err(qdev->ndev, "Got fatal error, STS = %x.\n", var);
> > var = ql_read32(qdev, ERR_STS);
> > @@ -2534,7 +2486,6 @@ static irqreturn_t qlge_isr(int irq, void *dev_id)
> > */
> > netif_err(qdev, intr, qdev->ndev,
> > "Got MPI processor interrupt.\n");
> > - ql_disable_completion_interrupt(qdev, intr_context->intr);
>
> Why disable interrupt is not required here ?
The interrupt source _is_ masked:
ql_write32(qdev, INTR_MASK, (INTR_MASK_PI << 16));
> While in case of Fatal error case above ql_disable_completion_interrupt() is being called ?
> Also, in case of MSI-X zeroth vector it will not disable completion interrupt but at the end, it will end of qlge_isr() enabling completion interrupt.
> Seems like disabling and enabling might not be in sync in case of MSI-X zeroth vector.
I guess you forgot to consider that completion interrupts are
auto-masked in msix mode.
^ permalink raw reply [flat|nested] 46+ messages in thread* RE: [PATCH net-next 01/16] qlge: Remove irq_cnt
2019-06-26 11:36 ` Benjamin Poirier
@ 2019-06-26 13:21 ` Manish Chopra
2019-07-05 8:33 ` Benjamin Poirier
0 siblings, 1 reply; 46+ messages in thread
From: Manish Chopra @ 2019-06-26 13:21 UTC (permalink / raw)
To: Benjamin Poirier; +Cc: GR-Linux-NIC-Dev, netdev
> In msix mode there's no need to explicitly disable completion interrupts, they
> are reliably auto-masked, according to my observations.
> I tested this on two QLE8142 adapters.
>
> Do you have reason to believe this might not always be the case?
How did you check auto-masking of MSI-X interrupts ?
I was just wondering about the below comment in ql_disable_completion_interrupt(), where for MSI-X it does disable completion intr for zeroth intr.
Seems special case for zeroth intr in MSI-X particular to this device.
/* HW disables for us if we're MSIX multi interrupts and
* it's not the default (zeroeth) interrupt.
*/
if (likely(test_bit(QL_MSIX_ENABLED, &qdev->flags) && intr))
return 0;
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH net-next 01/16] qlge: Remove irq_cnt
2019-06-26 13:21 ` Manish Chopra
@ 2019-07-05 8:33 ` Benjamin Poirier
0 siblings, 0 replies; 46+ messages in thread
From: Benjamin Poirier @ 2019-07-05 8:33 UTC (permalink / raw)
To: Manish Chopra; +Cc: GR-Linux-NIC-Dev, netdev
On 2019/06/26 13:21, Manish Chopra wrote:
> > In msix mode there's no need to explicitly disable completion interrupts, they
> > are reliably auto-masked, according to my observations.
> > I tested this on two QLE8142 adapters.
> >
> > Do you have reason to believe this might not always be the case?
>
> How did you check auto-masking of MSI-X interrupts ?
> I was just wondering about the below comment in ql_disable_completion_interrupt(), where for MSI-X it does disable completion intr for zeroth intr.
> Seems special case for zeroth intr in MSI-X particular to this device.
>
> /* HW disables for us if we're MSIX multi interrupts and
> * it's not the default (zeroeth) interrupt.
> */
> if (likely(test_bit(QL_MSIX_ENABLED, &qdev->flags) && intr))
> return 0;
>
I checked again and arrived at the same conclusion: in msix mode,
completion interrupts are masked automatically and the adapter does not
raise interrupts until they are enabled at the end of napi polling. That
includes queue 0.
I checked by adding some tracepoints and sending traffic using pktgen.
All udp traffic goes to queue 0 with qlge. Over a 100s interval I got
2970339 q0 interrupts. In all cases, INTR_EN_EN was unset for q0.
Moreover, there were no interrupts that were raised while we were sure
that interrupts were expected to be disabled. I also tested with icmp
and multiple streams of tcp traffic and got similar results.
The driver patch for tracing as well as the analysis script are at the
bottom of this mail. I use them like so:
root@dtest:~# trace-cmd record -C global -b 1000000 -s 1000000 -e qlge:compirq_* -f "intr == 0" -e qlge:q0_intr sleep 100
[...]
root@dtest:~# trace-cmd report -l | ./report.awk | awk '{print $1}' | sort | uniq -c
It took me a few days to reply because while doing that testing I
actually found another problem. It is present before this patch set. In
INTx mode, ql_disable_completion_interrupt() does not immediately
prevent the adapter from raising interrupts. Redoing a similar test as
the previous one while forcing INTx mode via qlge_irq_type=2, I get
something like this:
4966280 0x00004300
6565 0x0000c300
137749 def_bad
7094 ISR1_0
First, we can see what I already wrote in this patch:
+ /* Experience shows that when using INTx interrupts, the device does
+ * not always auto-mask the interrupt.
(The 0x0000c300 values include INTR_EN_EN)
Second, we can see 137749 instances of interrupts while we were
expecting interrupt generation to be disabled.
If I disable interrupts using INTR_EN_EI instead, I get something like
this:
4672919 0x00004300
75 0x0000c300
2 ISR1_0
I'll be including a patch for this in the next iteration of this
patchset.
==== report.awk ====
#!/usr/bin/awk -f
BEGIN {
enabled = -1;
}
/compirq_enable_b/ {
enabled = 1;
next;
}
/compirq_enable_a/ {
enabled = 2;
next;
}
/q0_intr/ {
# INTR_EN
print $10;
if ($14 == "0x00000000") {
print "ISR1_0";
}
if (enabled == 0) {
printf "def_bad "
print $3;
} else if (enabled == 1) {
printf "maybe_bad "
print $3;
}
# at this point we expect the irq to be masked, either automatically
# or explicitely
enabled = 0;
next;
}
==== driver patch ====
diff --git a/drivers/net/ethernet/qlogic/qlge/qlge_main.c b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
index 9a99e0938f08..ab306963eef1 100644
--- a/drivers/net/ethernet/qlogic/qlge/qlge_main.c
+++ b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
@@ -43,6 +43,9 @@
#include "qlge.h"
+#define CREATE_TRACE_POINTS
+#include "qlge_trace.h"
+
char qlge_driver_name[] = DRV_NAME;
const char qlge_driver_version[] = DRV_VERSION;
@@ -641,16 +644,20 @@ u32 ql_enable_completion_interrupt(struct ql_adapter *qdev, u32 intr)
/* Always enable if we're MSIX multi interrupts and
* it's not the default (zeroeth) interrupt.
*/
+ trace_compirq_enable_b(qdev, intr);
ql_write32(qdev, INTR_EN,
ctx->intr_en_mask);
+ trace_compirq_enable_a(qdev, intr);
var = ql_read32(qdev, STS);
return var;
}
spin_lock_irqsave(&qdev->hw_lock, hw_flags);
if (atomic_dec_and_test(&ctx->irq_cnt)) {
+ trace_compirq_enable_b(qdev, intr);
ql_write32(qdev, INTR_EN,
ctx->intr_en_mask);
+ trace_compirq_enable_a(qdev, intr);
var = ql_read32(qdev, STS);
}
spin_unlock_irqrestore(&qdev->hw_lock, hw_flags);
@@ -671,8 +678,10 @@ static u32 ql_disable_completion_interrupt(struct ql_adapter *qdev, u32 intr)
ctx = qdev->intr_context + intr;
spin_lock(&qdev->hw_lock);
if (!atomic_read(&ctx->irq_cnt)) {
+ trace_compirq_disable_b(qdev, intr);
ql_write32(qdev, INTR_EN,
ctx->intr_dis_mask);
+ trace_compirq_disable_a(qdev, intr);
var = ql_read32(qdev, STS);
}
atomic_inc(&ctx->irq_cnt);
@@ -2484,6 +2493,7 @@ static irqreturn_t qlge_msix_rx_isr(int irq, void *dev_id)
{
struct rx_ring *rx_ring = dev_id;
napi_schedule(&rx_ring->napi);
+ trace_napi_schedule(&rx_ring->napi);
return IRQ_HANDLED;
}
@@ -2500,6 +2510,8 @@ static irqreturn_t qlge_isr(int irq, void *dev_id)
u32 var;
int work_done = 0;
+ trace_q0_intr(qdev, ql_read32(qdev, STS), ql_read32(qdev, ISR1));
+
spin_lock(&qdev->hw_lock);
if (atomic_read(&qdev->intr_context[0].irq_cnt)) {
netif_printk(qdev, intr, KERN_DEBUG, qdev->ndev,
@@ -2552,6 +2564,7 @@ static irqreturn_t qlge_isr(int irq, void *dev_id)
"Waking handler for rx_ring[0].\n");
ql_disable_completion_interrupt(qdev, intr_context->intr);
napi_schedule(&rx_ring->napi);
+ trace_napi_schedule(&rx_ring->napi);
work_done++;
}
ql_enable_completion_interrupt(qdev, intr_context->intr);
diff --git a/drivers/net/ethernet/qlogic/qlge/qlge_trace.h b/drivers/net/ethernet/qlogic/qlge/qlge_trace.h
new file mode 100644
index 000000000000..f199c6eb785c
--- /dev/null
+++ b/drivers/net/ethernet/qlogic/qlge/qlge_trace.h
@@ -0,0 +1,98 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#if !defined(_QLGE_TRACE_H_) || defined(TRACE_HEADER_MULTI_READ)
+#define _QLGE_TRACE_H_
+
+#include <linux/tracepoint.h>
+
+#include "qlge.h"
+
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM qlge
+#define TRACE_INCLUDE_FILE qlge_trace
+
+#define NO_DEV "(no_device)"
+
+TRACE_EVENT(napi_schedule,
+ TP_PROTO(struct napi_struct *napi),
+ TP_ARGS(napi),
+
+ TP_STRUCT__entry(
+ __field(struct napi_struct *, napi)
+ __string(dev_name,
+ napi->dev ? napi->dev->name : NO_DEV)
+ __field(unsigned long, napi_state)
+ ),
+
+ TP_fast_assign(
+ __entry->napi = napi;
+ __assign_str(dev_name,
+ napi->dev ? napi->dev->name : NO_DEV);
+ __entry->napi_state = READ_ONCE(napi->state);
+ ),
+
+ TP_printk("napi schedule on napi struct %p for device %s napi 0x%02lx\n",
+ __entry->napi, __get_str(dev_name), __entry->napi_state)
+);
+
+DECLARE_EVENT_CLASS(compirq_template,
+ TP_PROTO(struct ql_adapter *qdev, u32 intr),
+ TP_ARGS(qdev, intr),
+
+ TP_STRUCT__entry(
+ __string(dev_name, qdev->ndev->name)
+ __field(unsigned int, intr)
+ ),
+
+ TP_fast_assign(
+ __assign_str(dev_name, qdev->ndev->name);
+ __entry->intr = intr;
+ ),
+
+ TP_printk("completion irq toggle device %s intr %d\n",
+ __get_str(dev_name), __entry->intr)
+);
+
+DEFINE_EVENT(compirq_template, compirq_enable_b,
+ TP_PROTO(struct ql_adapter *qdev, u32 intr),
+ TP_ARGS(qdev, intr));
+DEFINE_EVENT(compirq_template, compirq_enable_a,
+ TP_PROTO(struct ql_adapter *qdev, u32 intr),
+ TP_ARGS(qdev, intr));
+
+DEFINE_EVENT(compirq_template, compirq_disable_b,
+ TP_PROTO(struct ql_adapter *qdev, u32 intr),
+ TP_ARGS(qdev, intr));
+DEFINE_EVENT(compirq_template, compirq_disable_a,
+ TP_PROTO(struct ql_adapter *qdev, u32 intr),
+ TP_ARGS(qdev, intr));
+
+TRACE_EVENT(q0_intr,
+ TP_PROTO(struct ql_adapter *qdev, u32 sts, u32 isr1),
+ TP_ARGS(qdev, sts, isr1),
+
+ TP_STRUCT__entry(
+ __string(dev_name, qdev->ndev->name)
+ __field(unsigned int, intr_en)
+ __field(unsigned int, sts)
+ __field(unsigned int, isr1)
+ ),
+
+ TP_fast_assign(
+ __assign_str(dev_name, qdev->ndev->name);
+ ql_write32(qdev, INTR_EN, qdev->intr_context[0].intr_read_mask);
+ __entry->intr_en = ql_read32(qdev, INTR_EN);
+ __entry->sts = sts;
+ __entry->isr1 = isr1;
+ ),
+
+ TP_printk("interrupt for dev %s INTR_EN 0x%08x STS 0x%08x ISR1 0x%08x\n",
+ __get_str(dev_name), __entry->intr_en, __entry->sts,
+ __entry->isr1)
+);
+
+#endif /* _QLGE_TRACE_H_ */
+
+/* This part must be outside protection */
+#undef TRACE_INCLUDE_PATH
+#define TRACE_INCLUDE_PATH ../../drivers/net/ethernet/qlogic/qlge
+#include <trace/define_trace.h>
^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [PATCH net-next 01/16] qlge: Remove irq_cnt
2019-06-17 7:48 [PATCH net-next 01/16] qlge: Remove irq_cnt Benjamin Poirier
` (16 preceding siblings ...)
2019-06-26 8:59 ` Manish Chopra
@ 2019-07-15 1:40 ` Benjamin Poirier
2019-07-15 9:48 ` Greg Kroah-Hartman
17 siblings, 1 reply; 46+ messages in thread
From: Benjamin Poirier @ 2019-07-15 1:40 UTC (permalink / raw)
To: David Miller, Greg Kroah-Hartman; +Cc: Manish Chopra, GR-Linux-NIC-Dev, netdev
On 2019/06/17 16:48, Benjamin Poirier wrote:
> qlge uses an irq enable/disable refcounting scheme that is:
> * poorly implemented
> Uses a spin_lock to protect accesses to the irq_cnt atomic variable
> * buggy
> Breaks when there is not a 1:1 sequence of irq - napi_poll, such as
> when using SO_BUSY_POLL.
> * unnecessary
> The purpose or irq_cnt is to reduce irq control writes when
> multiple work items result from one irq: the irq is re-enabled
> after all work is done.
> Analysis of the irq handler shows that there is only one case where
> there might be two workers scheduled at once, and those have
> separate irq masking bits.
>
> Therefore, remove irq_cnt.
>
> Additionally, we get a performance improvement:
> perf stat -e cycles -a -r5 super_netperf 100 -H 192.168.33.1 -t TCP_RR
>
> Before:
> 628560
> 628056
> 622103
> 622744
> 627202
> [...]
> 268,803,947,669 cycles ( +- 0.09% )
>
> After:
> 636300
> 634106
> 634984
> 638555
> 634188
> [...]
> 259,237,291,449 cycles ( +- 0.19% )
>
> Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
David, Greg,
Before I send v2 of this patchset, how about moving this driver out to
staging? The hardware has been declared EOL by the vendor but what's
more relevant to the Linux kernel is that the quality of this driver is
not on par with many other mainline drivers, IMO. Over in staging, the
code might benefit from the attention of interested parties (drive-by
fixers) or fade away into obscurity.
Now that I check, the code has >500 basic checkpatch issues. While
working on the rx processing code for the current patchset, I noticed
the following more structural issues:
* commit 7c734359d350 ("qlge: Size RX buffers based on MTU.",
v2.6.33-rc1) introduced dead code in the receive routines, which
should be rewritten anyways by the admission of the author himself
(see the comment above ql_build_rx_skb())
* truesize accounting is incorrect (ex: a 9000B frame has truesize 10280
while containing two frags of order-1 allocations, ie. >16K)
* in some cases the driver allocates skbs with head room but only puts
data in the frags
* there is an inordinate amount of disparate debugging code, most of
which is of questionable value. In particular, qlge_dbg.c has hundreds
of lines of code bitrotting away in ifdef land (doesn't compile since
commit 18c49b91777c ("qlge: do vlan cleanup", v3.1-rc1), 8 years ago).
* triggering an ethtool regdump will instead hexdump a 176k struct to
dmesg depending on some module parameters
* many structures are defined full of holes, as we found in this
thread already; are used inefficiently (struct rx_ring is used for rx
and tx completions, with some members relevant to one case only); are
initialized redundantly (ex. memset 0 after alloc_etherdev). The
driver also has a habit of using runtime checks where compile time
checks are possible.
* in terms of namespace, the driver uses either qlge_, ql_ (used by
other qlogic drivers, with clashes, ex: ql_sem_spinlock) or nothing (with
clashes, ex: struct ob_mac_iocb_req)
I'm guessing other parts of the driver have more issues of the same
nature. The driver also has many smaller issues like deprecated apis,
duplicate or useless comments, weird code structure (some "while" loops
that could be rewritten with simple "for", ex. ql_wait_reg_rdy()) and
weird line wrapping (all over, ex. the ql_set_routing_reg() calls in
qlge_set_multicast_list()).
I'm aware of some precedents for code moving from mainline to staging:
1bb8155080c6 ncpfs: move net/ncpfs to drivers/staging/ncpfs (v4.16-rc1)
6c391ff758eb irda: move drivers/net/irda to drivers/staging/irda/drivers
(v4.14-rc1)
What do you think is the best course of action in this case?
Thanks,
-Benjamin
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH net-next 01/16] qlge: Remove irq_cnt
2019-07-15 1:40 ` Benjamin Poirier
@ 2019-07-15 9:48 ` Greg Kroah-Hartman
0 siblings, 0 replies; 46+ messages in thread
From: Greg Kroah-Hartman @ 2019-07-15 9:48 UTC (permalink / raw)
To: Benjamin Poirier; +Cc: David Miller, Manish Chopra, GR-Linux-NIC-Dev, netdev
On Mon, Jul 15, 2019 at 10:40:16AM +0900, Benjamin Poirier wrote:
> On 2019/06/17 16:48, Benjamin Poirier wrote:
> > qlge uses an irq enable/disable refcounting scheme that is:
> > * poorly implemented
> > Uses a spin_lock to protect accesses to the irq_cnt atomic variable
> > * buggy
> > Breaks when there is not a 1:1 sequence of irq - napi_poll, such as
> > when using SO_BUSY_POLL.
> > * unnecessary
> > The purpose or irq_cnt is to reduce irq control writes when
> > multiple work items result from one irq: the irq is re-enabled
> > after all work is done.
> > Analysis of the irq handler shows that there is only one case where
> > there might be two workers scheduled at once, and those have
> > separate irq masking bits.
> >
> > Therefore, remove irq_cnt.
> >
> > Additionally, we get a performance improvement:
> > perf stat -e cycles -a -r5 super_netperf 100 -H 192.168.33.1 -t TCP_RR
> >
> > Before:
> > 628560
> > 628056
> > 622103
> > 622744
> > 627202
> > [...]
> > 268,803,947,669 cycles ( +- 0.09% )
> >
> > After:
> > 636300
> > 634106
> > 634984
> > 638555
> > 634188
> > [...]
> > 259,237,291,449 cycles ( +- 0.19% )
> >
> > Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
>
> David, Greg,
>
> Before I send v2 of this patchset, how about moving this driver out to
> staging? The hardware has been declared EOL by the vendor but what's
> more relevant to the Linux kernel is that the quality of this driver is
> not on par with many other mainline drivers, IMO. Over in staging, the
> code might benefit from the attention of interested parties (drive-by
> fixers) or fade away into obscurity.
>
> Now that I check, the code has >500 basic checkpatch issues. While
> working on the rx processing code for the current patchset, I noticed
> the following more structural issues:
> * commit 7c734359d350 ("qlge: Size RX buffers based on MTU.",
> v2.6.33-rc1) introduced dead code in the receive routines, which
> should be rewritten anyways by the admission of the author himself
> (see the comment above ql_build_rx_skb())
> * truesize accounting is incorrect (ex: a 9000B frame has truesize 10280
> while containing two frags of order-1 allocations, ie. >16K)
> * in some cases the driver allocates skbs with head room but only puts
> data in the frags
> * there is an inordinate amount of disparate debugging code, most of
> which is of questionable value. In particular, qlge_dbg.c has hundreds
> of lines of code bitrotting away in ifdef land (doesn't compile since
> commit 18c49b91777c ("qlge: do vlan cleanup", v3.1-rc1), 8 years ago).
> * triggering an ethtool regdump will instead hexdump a 176k struct to
> dmesg depending on some module parameters
> * many structures are defined full of holes, as we found in this
> thread already; are used inefficiently (struct rx_ring is used for rx
> and tx completions, with some members relevant to one case only); are
> initialized redundantly (ex. memset 0 after alloc_etherdev). The
> driver also has a habit of using runtime checks where compile time
> checks are possible.
> * in terms of namespace, the driver uses either qlge_, ql_ (used by
> other qlogic drivers, with clashes, ex: ql_sem_spinlock) or nothing (with
> clashes, ex: struct ob_mac_iocb_req)
>
> I'm guessing other parts of the driver have more issues of the same
> nature. The driver also has many smaller issues like deprecated apis,
> duplicate or useless comments, weird code structure (some "while" loops
> that could be rewritten with simple "for", ex. ql_wait_reg_rdy()) and
> weird line wrapping (all over, ex. the ql_set_routing_reg() calls in
> qlge_set_multicast_list()).
>
> I'm aware of some precedents for code moving from mainline to staging:
> 1bb8155080c6 ncpfs: move net/ncpfs to drivers/staging/ncpfs (v4.16-rc1)
> 6c391ff758eb irda: move drivers/net/irda to drivers/staging/irda/drivers
> (v4.14-rc1)
>
> What do you think is the best course of action in this case?
Feel free to move it to staging if you want it removed from the tree in
a few releases if no one is willing to do the work to keep it alive. If
someone comes along later, it's a trivial revert to add it back.
So send a patch moving it, with all of the information you listed above
in a TODO file for the directory, and I'll be glad to take it.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 46+ messages in thread