[dpdk-dev,1/2] net/sfc: free mbufs in bulks on EF10 native Tx datapath reap
Checks
Commit Message
From: Ivan Malov <ivan.malov@oktetlabs.ru>
Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
---
drivers/net/sfc/sfc_ef10_tx.c | 48 ++++++++++++++++++++++++++++++++++++-------
drivers/net/sfc/sfc_tweak.h | 3 +++
2 files changed, 44 insertions(+), 7 deletions(-)
Comments
On 9/8/2017 3:15 PM, Andrew Rybchenko wrote:
> From: Ivan Malov <ivan.malov@oktetlabs.ru>
>
> Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru>
> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
> ---
> drivers/net/sfc/sfc_ef10_tx.c | 48 ++++++++++++++++++++++++++++++++++++-------
> drivers/net/sfc/sfc_tweak.h | 3 +++
> 2 files changed, 44 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/sfc/sfc_ef10_tx.c b/drivers/net/sfc/sfc_ef10_tx.c
> index 182fc23..5127a7a 100644
> --- a/drivers/net/sfc/sfc_ef10_tx.c
> +++ b/drivers/net/sfc/sfc_ef10_tx.c
> @@ -158,17 +158,35 @@ struct sfc_ef10_txq {
> pending += sfc_ef10_tx_process_events(txq);
>
> if (pending != completed) {
> + struct rte_mbuf *bulk[SFC_TX_REAP_BULK_SIZE];
> + unsigned int nb = 0;
> +
> do {
> struct sfc_ef10_tx_sw_desc *txd;
> + struct rte_mbuf *m;
>
> txd = &txq->sw_ring[completed & ptr_mask];
> + if (txd->mbuf == NULL)
> + continue;
>
> - if (txd->mbuf != NULL) {
> - rte_pktmbuf_free(txd->mbuf);
> - txd->mbuf = NULL;
> + m = rte_pktmbuf_prefree_seg(txd->mbuf);
> + txd->mbuf = NULL;
> + if (m == NULL)
> + continue;
> +
> + if ((nb == RTE_DIM(bulk)) ||
> + ((nb != 0) && (m->pool != bulk[0]->pool))) {
ICC is giving warning [1] here, as far as I can see this is false
positive but can you please double check in case I am missing something?
And unless if you don't see a way to convince icc without effecting
performance, would you mind updating patch to ignore warning [2] ?
[1]
error #3656: variable "bulk" may be used before its value is set
[2]
diff --git a/drivers/net/sfc/Makefile b/drivers/net/sfc/Makefile
index 57aa963ba..359314809 100644
--- a/drivers/net/sfc/Makefile
+++ b/drivers/net/sfc/Makefile
@@ -65,6 +65,7 @@ CFLAGS += -Wbad-function-cast
CFLAGS_BASE_DRIVER += -Wno-empty-body
else ifeq ($(CONFIG_RTE_TOOLCHAIN_ICC),y)
CFLAGS_BASE_DRIVER += -Wno-unused-but-set-variable
+CFLAGS_sfc_ef10_tx.o += -wd3656
endif
#
> + rte_mempool_put_bulk(bulk[0]->pool,
> + (void *)bulk, nb);
> + nb = 0;
> }
> +
> + bulk[nb++] = m;
> } while (++completed != pending);
>
> + if (nb != 0)
> + rte_mempool_put_bulk(bulk[0]->pool, (void *)bulk, nb);
> +
> txq->completed = completed;
> }
<...>
On Fri, 8 Sep 2017 15:15:50 +0100
Andrew Rybchenko <arybchenko@solarflare.com> wrote:
> + txd->mbuf = NULL;
> + if (m == NULL)
> + continue;
> +
> + if ((nb == RTE_DIM(bulk)) ||
> + ((nb != 0) && (m->pool != bulk[0]->pool))) {
> + rte_mempool_put_bulk(bulk[0]->pool,
> + (void *)bulk, nb);
> + nb = 0;
> }
> +
Why not add rte_mbuf_free_bulk (inline) to base code, rather than recoding
everywhere?
On 09/13/2017 12:51 AM, Stephen Hemminger wrote:
> On Fri, 8 Sep 2017 15:15:50 +0100
> Andrew Rybchenko <arybchenko@solarflare.com> wrote:
>
>> + txd->mbuf = NULL;
>> + if (m == NULL)
>> + continue;
>> +
>> + if ((nb == RTE_DIM(bulk)) ||
>> + ((nb != 0) && (m->pool != bulk[0]->pool))) {
>> + rte_mempool_put_bulk(bulk[0]->pool,
>> + (void *)bulk, nb);
>> + nb = 0;
>> }
>> +
> Why not add rte_mbuf_free_bulk (inline) to base code, rather than recoding
> everywhere?
I'm not 100% sure that I understand the question in a right way, but if
you're
talking about base driver code, it is not used in native datapath
implementations
at all (just header files with HW/SW interface definition). In fact
patches 1 and 2
of the series are slightly different and the difference is proved by
performance
measurements.
On 09/12/2017 09:26 PM, Ferruh Yigit wrote:
> On 9/8/2017 3:15 PM, Andrew Rybchenko wrote:
>> From: Ivan Malov <ivan.malov@oktetlabs.ru>
>>
>> Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru>
>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
>> ---
>> drivers/net/sfc/sfc_ef10_tx.c | 48 ++++++++++++++++++++++++++++++++++++-------
>> drivers/net/sfc/sfc_tweak.h | 3 +++
>> 2 files changed, 44 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/net/sfc/sfc_ef10_tx.c b/drivers/net/sfc/sfc_ef10_tx.c
>> index 182fc23..5127a7a 100644
>> --- a/drivers/net/sfc/sfc_ef10_tx.c
>> +++ b/drivers/net/sfc/sfc_ef10_tx.c
>> @@ -158,17 +158,35 @@ struct sfc_ef10_txq {
>> pending += sfc_ef10_tx_process_events(txq);
>>
>> if (pending != completed) {
>> + struct rte_mbuf *bulk[SFC_TX_REAP_BULK_SIZE];
>> + unsigned int nb = 0;
>> +
>> do {
>> struct sfc_ef10_tx_sw_desc *txd;
>> + struct rte_mbuf *m;
>>
>> txd = &txq->sw_ring[completed & ptr_mask];
>> + if (txd->mbuf == NULL)
>> + continue;
>>
>> - if (txd->mbuf != NULL) {
>> - rte_pktmbuf_free(txd->mbuf);
>> - txd->mbuf = NULL;
>> + m = rte_pktmbuf_prefree_seg(txd->mbuf);
>> + txd->mbuf = NULL;
>> + if (m == NULL)
>> + continue;
>> +
>> + if ((nb == RTE_DIM(bulk)) ||
>> + ((nb != 0) && (m->pool != bulk[0]->pool))) {
> ICC is giving warning [1] here, as far as I can see this is false
> positive but can you please double check in case I am missing something?
Yes, I think it is false positive.
> And unless if you don't see a way to convince icc without effecting
> performance, would you mind updating patch to ignore warning [2] ?
Thanks, done.
>
>
> [1]
> error #3656: variable "bulk" may be used before its value is set
>
> [2]
> diff --git a/drivers/net/sfc/Makefile b/drivers/net/sfc/Makefile
> index 57aa963ba..359314809 100644
> --- a/drivers/net/sfc/Makefile
> +++ b/drivers/net/sfc/Makefile
> @@ -65,6 +65,7 @@ CFLAGS += -Wbad-function-cast
> CFLAGS_BASE_DRIVER += -Wno-empty-body
> else ifeq ($(CONFIG_RTE_TOOLCHAIN_ICC),y)
> CFLAGS_BASE_DRIVER += -Wno-unused-but-set-variable
> +CFLAGS_sfc_ef10_tx.o += -wd3656
> endif
>
> #
<...>
@@ -158,17 +158,35 @@ struct sfc_ef10_txq {
pending += sfc_ef10_tx_process_events(txq);
if (pending != completed) {
+ struct rte_mbuf *bulk[SFC_TX_REAP_BULK_SIZE];
+ unsigned int nb = 0;
+
do {
struct sfc_ef10_tx_sw_desc *txd;
+ struct rte_mbuf *m;
txd = &txq->sw_ring[completed & ptr_mask];
+ if (txd->mbuf == NULL)
+ continue;
- if (txd->mbuf != NULL) {
- rte_pktmbuf_free(txd->mbuf);
- txd->mbuf = NULL;
+ m = rte_pktmbuf_prefree_seg(txd->mbuf);
+ txd->mbuf = NULL;
+ if (m == NULL)
+ continue;
+
+ if ((nb == RTE_DIM(bulk)) ||
+ ((nb != 0) && (m->pool != bulk[0]->pool))) {
+ rte_mempool_put_bulk(bulk[0]->pool,
+ (void *)bulk, nb);
+ nb = 0;
}
+
+ bulk[nb++] = m;
} while (++completed != pending);
+ if (nb != 0)
+ rte_mempool_put_bulk(bulk[0]->pool, (void *)bulk, nb);
+
txq->completed = completed;
}
@@ -325,6 +343,7 @@ struct sfc_ef10_txq {
do {
phys_addr_t seg_addr = rte_mbuf_data_dma_addr(m_seg);
unsigned int seg_len = rte_pktmbuf_data_len(m_seg);
+ unsigned int id = added & ptr_mask;
SFC_ASSERT(seg_len <= SFC_EF10_TX_DMA_DESC_LEN_MAX);
@@ -332,15 +351,30 @@ struct sfc_ef10_txq {
sfc_ef10_tx_qdesc_dma_create(seg_addr,
seg_len, (pkt_len == 0),
- &txq->txq_hw_ring[added & ptr_mask]);
+ &txq->txq_hw_ring[id]);
+
+ /*
+ * rte_pktmbuf_free() is commonly used in DPDK for
+ * recycling packets - the function checks every
+ * segment's reference counter and returns the
+ * buffer to its pool whenever possible;
+ * nevertheless, freeing mbuf segments one by one
+ * may entail some performance decline;
+ * from this point, sfc_efx_tx_reap() does the same job
+ * on its own and frees buffers in bulks (all mbufs
+ * within a bulk belong to the same pool);
+ * from this perspective, individual segment pointers
+ * must be associated with the corresponding SW
+ * descriptors independently so that only one loop
+ * is sufficient on reap to inspect all the buffers
+ */
+ txq->sw_ring[id].mbuf = m_seg;
+
++added;
} while ((m_seg = m_seg->next) != 0);
dma_desc_space -= (added - pkt_start);
-
- /* Assign mbuf to the last used desc */
- txq->sw_ring[(added - 1) & ptr_mask].mbuf = *pktp;
}
if (likely(added != txq->added)) {
@@ -53,4 +53,7 @@
/** Default free threshold follows recommendations from DPDK documentation */
#define SFC_TX_DEFAULT_FREE_THRESH 32
+/** Number of mbufs to be freed in bulk in a single call */
+#define SFC_TX_REAP_BULK_SIZE 32
+
#endif /* _SFC_TWEAK_H_ */