Re: [PATCH v2 2/2] ixgbe: enable l2 forwarding acceleration for macvlans

[John Fastabend <john.r.fastabend@intel.com>]

On 11/5/2013 6:26 AM, Neil Horman wrote:
> On Mon, Nov 04, 2013 at 11:01:55AM -0800, John Fastabend wrote:
>> Now that l2 acceleration ops are in place from the prior patch,
>> enable ixgbe to take advantage of these operations.  Allow it to
>> allocate queues for a macvlan so that when we transmit a frame,
>> we can do the switching in hardware inside the ixgbe card, rather
>> than in software.
>>
>> For now this patch limits the hardware to 8 offloaded macvlan ports.
>> A follow on patch will remove this limitation but to simplify
>> review/validation of the new macvlan offload ops we leave it at 8
>> for now.
>>
>> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
>> CC: Andy Gospodarek <andy@greyhouse.net>
>> CC: "David S. Miller" <davem@davemloft.net>
>
> A few Nits, since you're going to send a V3 anyway.  Comments inline.
>
>> ---


[...]

> Nit: You're using macvlan here, and vsi above.  Might be nice to give everything
> a consistent suffix/prefix for clarity.  Perhaps dfwd since thats what the new
> ndo_ops uses?
>

using dfwd now.

>> <snip>
>
>>   static inline bool ixgbe_is_sfp(struct ixgbe_hw *hw)
>> @@ -4317,6 +4521,8 @@ static void ixgbe_setup_gpie(struct ixgbe_adapter *adapter)
>>   static void ixgbe_up_complete(struct ixgbe_adapter *adapter)
>>   {
>>   	struct ixgbe_hw *hw = &adapter->hw;
>> +	struct net_device *upper;
>> +	struct list_head *iter;
>>   	int err;
>>   	u32 ctrl_ext;
>>
>> @@ -4360,6 +4566,16 @@ static void ixgbe_up_complete(struct ixgbe_adapter *adapter)
>>   	/* enable transmits */
>>   	netif_tx_start_all_queues(adapter->netdev);
>>
>> +	/* enable any upper devices */
>> +	netdev_for_each_all_upper_dev_rcu(adapter->netdev, upper, iter) {
>> +		if (netif_is_macvlan(upper)) {
>> +			struct macvlan_dev *vlan = netdev_priv(upper);
>> +
>> +			if (vlan->fwd_priv)
>> +				netif_tx_start_all_queues(upper);
>> +		}
>> +	}
>> +
> I don't think it matters much, but do we want to start the upper devs queues
> here unilaterally?  Nominally a network interface starts its queues on dev_open.
> Would it be better to only start those devices if (vlan->fwd_priv &&
> (upper->flags & IFF_UP)?  We would then have to listen for NETDEV_UP events as
> well to handle accelerated macvlans comming up after ixgbe does, which is more
> work, but I wanted to mention this in case theres some disadvantage to starting
> the queue early.
>

vlan->fwd_priv is set in macvlan_open() and cleared in macvlan_close()
and serialized with the rtnl lock. So I think anytime you get here and
have vlan->fwd_priv non-null its the same as IFF_UP being set.

The only call sites I see changing the IFF_UP flag are dev_open() and
dev_close() and derivatives.  Each calls ndo_open and ndo_close. So
assuming I didn't miss some call sites the above seems correct.

>
> Other than that I think it looks good though.  Nice work!
>

Great thanks for looking it over. I should have v3 out here shortly
doing a couple more tests for feature interop.

> Regards
> Neil
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>