Re: [REPOST patch net-next V6] net: introduce ethernet teaming device

[Eric Dumazet <eric.dumazet@gmail.com>]

Le jeudi 10 novembre 2011 à 00:12 +0100, Eric Dumazet a écrit :
> Le mercredi 09 novembre 2011 à 23:13 +0100, Jiri Pirko a écrit :
> > This patch introduces new network device called team. It supposes to be
> > very fast, simple, userspace-driven alternative to existing bonding
> > driver.
> 
> ...
> 
> > +/*
> > + * note: already called with rcu_read_lock
> > + */
> > +static netdev_tx_t team_xmit(struct sk_buff *skb, struct net_device *dev)
> > +{
> > +	struct team *team = netdev_priv(dev);
> > +	bool tx_success = false;
> > +	unsigned int len = skb->len;
> > +
> > +	/*
> > +	 * Ensure transmit function is called only in case there is at least
> > +	 * one port present.
> > +	 */
> > +	if (likely(!list_empty(&team->port_list) && team->mode_ops.transmit))
> > +		tx_success = team->mode_ops.transmit(team, skb);

Not clear why its so complex here.

When you manipulate team->port_list and make it empty, why dont you set
team->mode_ops.transmit to a helper function, freeing the skb and
returning false.

Also, instead of setting .transmit to NULL, you also could set it to
same helper function.

This way you could just do in fast path :

	tx_succcess = team->mode_ops.transmit(team, skb);

Avoiding two tests

> > +	if (tx_success) {
> > +		struct team_pcpu_stats *pcpu_stats;
> > +
> > +		pcpu_stats = this_cpu_ptr(team->pcpu_stats);
> > +		u64_stats_update_begin(&pcpu_stats->syncp);
> > +		pcpu_stats->tx_packets++;
> > +		pcpu_stats->tx_bytes += len;
> > +		u64_stats_update_end(&pcpu_stats->syncp);
> > +	} else {
> > +		this_cpu_inc(team->pcpu_stats->tx_dropped);
> > +	}
> > +
> > +	return NETDEV_TX_OK;
> > +}
> > +
>