[dpdk-dev] [PATCH 1/4] Link Bonding Library

Doherty, Declan declan.doherty at intel.com
Thu May 29 15:32:38 CEST 2014



> -----Original Message-----
> From: Shaw, Jeffrey B
> Sent: Wednesday, May 28, 2014 5:55 PM
> To: Doherty, Declan; dev at dpdk.org
> Subject: RE: [dpdk-dev] [PATCH 1/4] Link Bonding Library
> 
> Hi Declan,
> I'm worried about one thing in "bond_ethdev_tx_broadcast()" related to
> freeing of the broadcasted packets.
> 
> > +static uint16_t
> > +bond_ethdev_tx_broadcast(void *queue, struct rte_mbuf **bufs,
> > +uint16_t nb_pkts) {
> > +	struct bond_dev_private *internals;
> > +	struct bond_tx_queue *bd_tx_q;
> > +
> > +	uint8_t num_of_slaves;
> > +	uint8_t slaves[RTE_MAX_ETHPORTS];
> > +
> > +	uint16_t num_tx_total = 0;
> > +
> > +	int i;
> > +
> > +	bd_tx_q = (struct bond_tx_queue *)queue;
> > +	internals = bd_tx_q->dev_private;
> > +
> > +	/* Copy slave list to protect against slave up/down changes during tx
> > +	 * bursting */
> > +	num_of_slaves = internals->active_slave_count;
> > +	memcpy(slaves, internals->active_slaves,
> > +			sizeof(internals->active_slaves[0]) * num_of_slaves);
> > +
> > +	if (num_of_slaves < 1)
> > +		return 0;
> > +
> > +
> > +	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);
> > +	}
> > +
> > +	return num_tx_total;
> > +}
> > +
> 
> Transmitting the same buffers on all slaves will cause problems when the PMD
> frees the mbufs for previously transmitted packets.
> So if you broadcast 1 packet on 4 slaves, each of the 4 slaves will (eventually)
> have to free the same mbuf.  Without updating the refcnt, you will end up
> incorrectly freeing the same mbuf 3 extra times.
> 
> Your test application does not catch this case since the max packets that are
> tested is 320, which is less than the size of the Tx descriptor rings (512).  Try
> testing with more packets and you should see some latent segmentation
> faults.  You should also see this with testpmd, if you try broadcasting enough
> packets.
> 
> 
> Thanks,
> Jeff

Hey Jeff, I'm not sure why I didn't see this in testing using on the test-pmd app, but it's obvious that this will cause issues. I'll add code to increment the refcnt in each mbuf by (num_of_slaves -1) in the bond_ethdev_tx_broadcast() function and also add a unit test to validate this in the next version of the patch.

Thanks,
Declan
--------------------------------------------------------------
Intel Shannon Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263
Business address: Dromore House, East Park, Shannon, Co. Clare

This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.




More information about the dev mailing list