Re: [RFC PATCH 0/2 v2] net: alternate proposal for using macvlans with forwarding acceleration

[John Fastabend <john.fastabend@gmail.com>]

On 10/04/2013 01:10 PM, Neil Horman wrote:
> Hey all-
>       heres the next, updated version of the vsi/macvlan integration that we've
> been discussing.
>
> Some change notes:
>
> * Changes to the fowarding ops structure - Removed the priv_size field, and
> added a flags field.  Removal of the priv_size field was accomplished by just
> having the add method return a void * and using ERR_PTR and PTR_ERR checks,
> which also allows us to allocate memory for the acceleration path in the driver,
> which I like.  I'm not super happy still with how I'm using the flags (currenly
> only used to indicate support for feature sets), but at least we have the flags
> now, and they can be exposed to user space via iproute2 or ethtool if need be

For the flag why not use the existing feature flag namespace? Adding
NETIF_F_HW_L2FW_DOFFLOAD or something equivalent to netdev_features.h
and also doing the string updates would get the userspace support for
free.

The one downside of a global feature flag approach like this is if
the user wants to create some offloaded macvlan devices and some SW
only macvlans the control sequence is a bit clumsy. But unless we add
a new flag to the macvlan netlink message I'm not sure how to avoid
this. Further if you have multiple control applications
creating/deleting these macvlans the flag mechanism starts to break
down. One app sets the flag the other deletes, you get the idea.

It might be worth adding a netlink flag to change the state of
the HW offload. I know this goes against the 'it just gets offloaded'
line of thought, but if you make the default to offload then it should
be OK.

>
> * Changes to the Transmit path - Specifically I'm using dev_queue_xmit to send
> frames now, which I like as it makes the macvlan subject to the lowerdevs qdisc
> configuration.

This creates perhaps another oddity where on TX the packets will be
visible to the lowerdev. Think tcpdump and egress qdisc. But on ingress
the packets will be delivered directly to macvlan device. Also I was
thinking there might be good reasons to skip the lowerdev qdisc, for
performance reasons or QOS setups.

So it might be best the way you had it in the previous revision even if
it was not functionally equivalent to the SW path. If you skip the lower
dev and submit directly to the hardware then you also don't need a
sk_buff change. The driver can do a lookup via the skb->dev field and
additional hints from the stack are not needed.

If there is a perceived problem with not having the HW and SW completely
equivalent functionally we could always default the feature flag to off.
Then enabling the feature would be a single 'ethtool' command. This
seems like a nice compromise.

I like where this is going though!

Thanks,
John

-- 
John Fastabend         Intel Corporation