Re: [PATCH] 802.1Q VLAN

[Tommy Christensen <tommy.christensen@tpack.net>]

Ben Greear wrote:
> How does this look?  I think it should fix the problem with
> having a new skb created in the __vlan_put_tag() method.

Yes, this seems to be handled correctly now.

Two nitpickings:
  - veth should be reassigned after calling __vlan_put_tag
  - sample skb->len before calling dev_queue_xmit and use that to
    update stats->tx_bytes (it can be different from skb2->len)

And then there's the return values ...

This is a hard_start_xmit() method, so we should try to be
consistent with real device drivers. The only defined return
values are: NETDEV_TX_OK and NETDEV_TX_BUSY.
(There is also Andi's NETDEV_TX_LOCKED, which we can ignore here).

Furthermore transmission shouldn't be retried in case of failure,
only on congestion does this make sense.

So my suggestion for the last part of the function is:

	len = skb->len;
	rv = dev_queue_xmit(skb);
	if (rv < 0) {
		stats->tx_dropped++;
		kfree_skb(skb2);
		ret = NETDEV_TX_OK;
	} else if (rv == NET_XMIT_SUCCESS || rv == NET_XMIT_CN) {
		stats->tx_packets++;
		stats->tx_bytes += len;
		kfree_skb(skb2);
		ret = NETDEV_TX_OK;
	} else {
		/* The device below us is congested */
		ret = NETDEV_TX_BUSY;
	}

	return ret;


-Tommy