DPDK patches and discussions
 help / color / mirror / Atom feed
* Re: [dpdk-dev] [PATCH 4/6] bond: free mbufs if transmission fails in bonding tx_burst functions
@ 2014-08-20 20:54 Sanford, Robert
  2014-08-29 15:36 ` Thomas Monjalon
  0 siblings, 1 reply; 3+ messages in thread
From: Sanford, Robert @ 2014-08-20 20:54 UTC (permalink / raw)
  To: Declan Doherty, dev

Hi Declan,

I have a problem with the TX-burst logic of this patch. I believe that for
packets that we *don't* enqueue to the device, we should *NOT* free them.
The API expects that the caller will free them or try again to send them.

Here is one way to accomplish selective freeing: Move mbuf pointers of
packets successfully enqueued, to the beginning of the caller's mbuf
vector; move mbuf pointers of packets not enqueued, to the end of the
caller's mbuf list. Although possibly re-ordered, the caller will be able
to free/resend all (and only) mbufs that we failed to enqueue.

Here is a code snippet to do this:

  uint16_t failed_index = nb_pkts;

  /* Send packet burst on each slave device */
  for (i = 0; i < num_of_slaves; i++) {
    if (slave_nb_pkts[i] > 0) {
      num_tx_slave = rte_eth_tx_burst(slaves[i],
          bd_tx_q->queue_id, slave_bufs[i], slave_nb_pkts[i]);

    
      /* Move sent packets to the beginning of the caller's array. */
      if (likely(num_tx_slave)) {
        memcpy(&bufs[num_tx_total], slave_bufs[i],
            num_tx_slave * sizeof(void *));
        num_tx_total += num_tx_slave;
      }

    
      /* Move failed packets to the end of the caller's array,
       * so that the caller can free or resend the correct ones.
       */
      if (unlikely(num_tx_slave < slave_nb_pkts[i])) {
        uint16_t num_failed = slave_nb_pkts[i] - num_tx_slave;
        failed_index -= num_failed;
        memcpy(&bufs[failed_index], &slave_bufs[i][num_tx_slave],
            num_failed * sizeof(void *));
      }
    }
  }       
 


--
Regards,
Robert


>Fixing a number of corner cases that if transmission failed on slave
>devices then this
>could lead to leaked mbufs
>
>
>Signed-off-by: Declan Doherty <declan.doherty@intel.com>
>---
> app/test/test_link_bonding.c           |    4 +-
> lib/librte_pmd_bond/rte_eth_bond_pmd.c |   46
>+++++++++++++++++++++++++-------
> 2 files changed, 38 insertions(+), 12 deletions(-)
>
>diff --git a/app/test/test_link_bonding.c b/app/test/test_link_bonding.c
>index 02823b6..3c265ee 100644
>--- a/app/test/test_link_bonding.c
>+++ b/app/test/test_link_bonding.c
>@@ -3415,7 +3415,7 @@ test_broadcast_tx_burst(void)
> 	/* Send burst on bonded port */
> 	nb_tx = rte_eth_tx_burst(test_params->bonded_port_id, 0, pkts_burst,
> 			burst_size);
>-	if (nb_tx != burst_size * test_params->bonded_slave_count) {
>+	if (nb_tx != burst_size) {
> 		printf("Bonded Port (%d) rx burst failed, packets transmitted value
>(%u) not as expected (%d)\n",
> 				test_params->bonded_port_id,
> 				nb_tx, burst_size);
>@@ -3770,7 +3770,7 @@
>test_broadcast_verify_slave_link_status_change_behaviour(void)
> 	}
> 
> 	if (rte_eth_tx_burst(test_params->bonded_port_id, 0, &pkt_burst[0][0],
>-			burst_size) != (burst_size * slave_count)) {
>+			burst_size) != burst_size) {
> 		printf("rte_eth_tx_burst failed\n");
> 		return -1;
> 	}
>diff --git a/lib/librte_pmd_bond/rte_eth_bond_pmd.c
>b/lib/librte_pmd_bond/rte_eth_bond_pmd.c
>index 70123fc..ae9726e 100644
>--- a/lib/librte_pmd_bond/rte_eth_bond_pmd.c
>+++ b/lib/librte_pmd_bond/rte_eth_bond_pmd.c
>@@ -101,7 +101,7 @@ bond_ethdev_tx_burst_round_robin(void *queue, struct
>rte_mbuf **bufs,
> 	uint8_t num_of_slaves;
> 	uint8_t slaves[RTE_MAX_ETHPORTS];
> 
>-	uint16_t num_tx_total = 0;
>+	uint16_t num_tx_total = 0, num_tx_slave;
> 
> 	static int slave_idx = 0;
> 	int i, cs_idx = 0;
>@@ -130,9 +130,17 @@ bond_ethdev_tx_burst_round_robin(void *queue, struct
>rte_mbuf **bufs,
> 
> 	/* Send packet burst on each slave device */
> 	for (i = 0; i < num_of_slaves; i++)
>-		if (slave_nb_pkts[i] > 0)
>-			num_tx_total += rte_eth_tx_burst(slaves[i],
>+		if (slave_nb_pkts[i] > 0) {
>+			num_tx_slave = rte_eth_tx_burst(slaves[i],
> 					bd_tx_q->queue_id, slave_bufs[i], slave_nb_pkts[i]);
>+			num_tx_total += num_tx_slave;
>+
>+			/* if tx burst fails, free unsent mbufs */
>+			while (unlikely(num_tx_slave < slave_nb_pkts[i])) {
>+				rte_pktmbuf_free(slave_bufs[i][num_tx_slave]);
>+				num_tx_slave++;
>+			}
>+		}
> 
> 	return num_tx_total;
> }
>@@ -283,7 +291,7 @@ bond_ethdev_tx_burst_balance(void *queue, struct
>rte_mbuf **bufs,
> 	uint8_t num_of_slaves;
> 	uint8_t slaves[RTE_MAX_ETHPORTS];
> 
>-	uint16_t num_tx_total = 0;
>+	uint16_t num_tx_total = 0, num_tx_slave = 0;
> 
> 	int i, op_slave_id;
> 
>@@ -315,11 +323,19 @@ bond_ethdev_tx_burst_balance(void *queue, struct
>rte_mbuf **bufs,
> 	/* Send packet burst on each slave device */
> 	for (i = 0; i < num_of_slaves; i++) {
> 		if (slave_nb_pkts[i] > 0) {
>-			num_tx_total += rte_eth_tx_burst(slaves[i], bd_tx_q->queue_id,
>+			num_tx_slave = rte_eth_tx_burst(slaves[i], bd_tx_q->queue_id,
> 					slave_bufs[i], slave_nb_pkts[i]);
>+			num_tx_total += num_tx_slave;
>+
>+			/* if tx burst fails, free unsent mbufs */
>+			while (unlikely(num_tx_slave < slave_nb_pkts[i])) {
>+				rte_pktmbuf_free(slave_bufs[i][num_tx_slave]);
>+				num_tx_slave++;
>+			}
> 		}
> 	}
> 
>+
> 	return num_tx_total;
> }
> 
>@@ -333,7 +349,7 @@ bond_ethdev_tx_burst_broadcast(void *queue, struct
>rte_mbuf **bufs,
> 	uint8_t num_of_slaves;
> 	uint8_t slaves[RTE_MAX_ETHPORTS];
> 
>-	uint16_t num_tx_total = 0;
>+	uint16_t num_tx_slave = 0, max_tx_pkts = 0;
> 
> 	int i;
> 
>@@ -354,11 +370,21 @@ bond_ethdev_tx_burst_broadcast(void *queue, struct
>rte_mbuf **bufs,
> 		rte_mbuf_refcnt_update(bufs[i], num_of_slaves - 1);
> 
> 	/* Transmit burst on each active slave */
>-	for (i = 0; i < num_of_slaves; i++)
>-		num_tx_total += rte_eth_tx_burst(slaves[i], bd_tx_q->queue_id,
>-				bufs, nb_pkts);
>+	for (i = 0; i < num_of_slaves; i++) {
>+		num_tx_slave = rte_eth_tx_burst(slaves[i], bd_tx_q->queue_id,
>+					bufs, nb_pkts);
> 
>-	return num_tx_total;
>+		if (max_tx_pkts < num_tx_slave)
>+			max_tx_pkts = num_tx_slave;
>+
>+		/* if tx burst fails, free unsent mbufs */
>+		while (unlikely(num_tx_slave < nb_pkts)) {
>+			rte_pktmbuf_free(bufs[num_tx_slave]);
>+			num_tx_slave++;
>+		}
>+	}
>+
>+	return max_tx_pkts;
> }
> 
> void
>-- 
>1.7.0.7
>

^ permalink raw reply	[flat|nested] 3+ messages in thread
* [dpdk-dev] [PATCH 0/6] link bonding
@ 2014-08-19 13:51 Declan Doherty
  2014-08-19 13:51 ` [dpdk-dev] [PATCH 4/6] bond: free mbufs if transmission fails in bonding tx_burst functions Declan Doherty
  0 siblings, 1 reply; 3+ messages in thread
From: Declan Doherty @ 2014-08-19 13:51 UTC (permalink / raw)
  To: dev

This patch set adds support for link status interrupt in the link bonding
pmd. It also contains some patches to tidy up the code structure and to
of the link bonding code and to fix bugs relating to transmission 
failures in the under lying slave pmd which could lead to leaked mbufs.  


Declan Doherty (6):
  bond: link status interrupt support
  bond: removing switch statement from rx burst method
  bond: fix naming inconsistency in tx_burst_round_robin
  bond: free mbufs if transmission fails in bonding tx_burst functions
  test app: adding support for generating variable sized packets
  testpmd: adding parameter to reconfig method to set socket_id when
    adding new port to portlist

 app/test-pmd/cmdline.c                 |    2 +-
 app/test-pmd/testpmd.c                 |    3 +-
 app/test-pmd/testpmd.h                 |    2 +-
 app/test/packet_burst_generator.c      |   22 +--
 app/test/packet_burst_generator.h      |    6 +-
 app/test/test_link_bonding.c           |  234 ++++++++++++++++++++++++++------
 lib/librte_pmd_bond/rte_eth_bond_api.c |    4 +
 lib/librte_pmd_bond/rte_eth_bond_pmd.c |  124 +++++++++++------
 8 files changed, 295 insertions(+), 102 deletions(-)

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-08-29 15:32 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-08-20 20:54 [dpdk-dev] [PATCH 4/6] bond: free mbufs if transmission fails in bonding tx_burst functions Sanford, Robert
2014-08-29 15:36 ` Thomas Monjalon
  -- strict thread matches above, loose matches on Subject: below --
2014-08-19 13:51 [dpdk-dev] [PATCH 0/6] link bonding Declan Doherty
2014-08-19 13:51 ` [dpdk-dev] [PATCH 4/6] bond: free mbufs if transmission fails in bonding tx_burst functions Declan Doherty

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).