Re: [PATCH v3 2/2] macvtap: Allow tap features change when IFF_VNET_HDR is disabled.

[Vlad Yasevich <vyasevic@redhat.com>]

On 08/15/2013 03:27 PM, Michael S. Tsirkin wrote:
> On Thu, Aug 15, 2013 at 02:59:01PM -0400, Vlad Yasevich wrote:
>> On 08/15/2013 02:48 PM, Michael S. Tsirkin wrote:
>>> On Thu, Aug 15, 2013 at 02:39:39PM -0400, Vlad Yasevich wrote:
>>>> On 08/15/2013 01:33 PM, Michael S. Tsirkin wrote:
>>>>> On Thu, Aug 15, 2013 at 01:02:53PM -0400, Vlad Yasevich wrote:
>>>>>> When the user turns off IFF_VNET_HDR flag, attempts to change
>>>>>> offload features via TUNSETOFFLOAD do not work.  This could cause
>>>>>> GSO packets to be delivered to the user when the user is
>>>>>> not prepared to handle them.
>>>>>>
>>>>>> To solve, allow processing of TUNSETOFFLOAD when IFF_VNET_HDR is
>>>>>> disabled.  Also, take the setting of IFF_VNET_HDR into consideration
>>>>>> when determining the feature set to apply to incommig traffic.
>>>>>> If IFF_VNET_HDR is set, we use user-specfied feature set.  if the
>>>>>> IFF_VNET_HDR is clear, we mask off all tun-specific offload features,
>>>>>> since user can not be notified of the possbile offloads.
>>>>>>
>>>>>> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
>>>>>> ---
>>>>>>   drivers/net/macvtap.c | 30 ++++++++++++++++++++++++------
>>>>>>   1 file changed, 24 insertions(+), 6 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
>>>>>> index 8121358..df45a59 100644
>>>>>> --- a/drivers/net/macvtap.c
>>>>>> +++ b/drivers/net/macvtap.c
>>>>>> @@ -269,6 +269,28 @@ static void macvtap_del_queues(struct net_device *dev)
>>>>>>   		sock_put(&qlist[j]->sk);
>>>>>>   }
>>>>>>
>>>>>> +/* Determine features to be applied to the packet as it
>>>>>
>>>>> /*
>>>>>   * Please use comments
>>>>>   * like this
>>>>>   */
>>>>>
>>>>>> + * gets sent to the tap.  The features depend on the offloads
>>>>>> + * set by user and by whether or not IFF_VNET_HDR is enabled.
>>>>>> + */
>>>>>> +static netdev_features_t macvtap_skb_features(struct sk_buff *skb,
>>>>>> +					      struct macvtap_queue *q)
>>>>>> +{
>>>>>> +	struct macvlan_dev *vlan = netdev_priv(skb->dev);
>>>>>> +	netdev_features_t features = netif_skb_features(skb);
>>>>>> +
>>>>>> +	/* If VNET_HDR is enabled, user can receive offload info so
>>>>>> +	 * use user-specified tap_features.
>>>>>> +	 * Otherwise, mask off all tap offload features.
>>>>>> +	 */
>>>>>> +	if (q->flags & IFF_VNET_HDR)
>>>>>> +		features &= vlan->tap_features;
>>>>>> +	else
>>>>>> +		features &= ~TUN_OFFLOADS;
>>>>>> +
>>>>>> +	return features;
>>>>>> +}
>>>>>> +
>>>>>
>>>>> Okay, but this means we need to do netdev_update_features
>>>>> on SETIFF since it changes q->flags.
>>>>
>>>> Why?
>>>>   The whole point it not to change features on SETIFF, so that
>>>> they are preserved for when the flags change again.  It just that
>>>> flag values either allow whatever offload user specified (correctly
>>>> masked via netdev_update_features) or turn off all offloads at the tap.
>>>>
>>>> -vlad
>>>
>>> That's the point. As it is, if flag value changes, we don't call
>>> netdev_update_features so offloads aren't masked/unmasked.
>>>
>>> Example:
>>>
>>> 1. SETOFFLOADS to enable
>>> 2. VNET = 0
>>>
>>> offloads are still enabled
>>>
>>
>> But unused since since VNET=0.
>
>
> Well, I think that's true but it would still be better
> to disable RX offloads, no?

Disabled RX offloads doesn't do anything since rx offload
processing only happens on the lowest level device and not
macvlan.  I've toyed with disabling RX offloads on the lowest
device when all macvlans above it had RX disabled, but the
performance was lower then performing GSO.  Didn't dig into
the why.

>
>>   I am trying to be careful not to
>> break things when ioctl calls get all inverted and reversed, but
>> may be I am trying too hard and we should just let things break.
>
> What exactly will break by call to netdev_update_features?

I was referring to the proposed patch, not to calling 
netdev_update_features.  Sorry for the confusion.

There macvlan device features aren't changing, so why call
netdev_update_features().

The v1 of the patch attempted to change the features upon
flag change, and you didn't like that.

The way I see it, we have 2 options:
   1) Don't bother with tracking the state of VNET_HDR, and let
      broken apps find out the hard way that they are broken.  This
      would reduce this patch to just removing the 'if' check from
      SETOFFLOADS ioctl.

   2) Try to be smart and handle the situation where VNET_HDR is
      turned off while offloads still set.  Then we need essentially
      ignore the state the offloads.  We could change them, but then
      there would be no way to restore them and we'll have asymmetric
      behavior (v2 of the patch).  This v3, removes that asymmetry.

I guess I'd rather we be smart and handle the situation for the clueless
user/app, but I am OK with option 1 as well.

-vlad

>
>> I just feels kinds of silly since the app may now know all the
>> details, but the kernel does and can do the "right thing".
>>
>> -vlad
>>
>>>
>>>>>>   /*
>>>>>>    * Forward happens for data that gets sent from one macvlan
>>>>>>    * endpoint to another one in bridge mode. We just take
>>>>>> @@ -276,9 +298,9 @@ static void macvtap_del_queues(struct net_device *dev)
>>>>>>    */
>>>>>>   static int macvtap_forward(struct net_device *dev, struct sk_buff *skb)
>>>>>>   {
>>>>>> -	struct macvlan_dev *vlan = netdev_priv(dev);
>>>>>>   	struct macvtap_queue *q = macvtap_get_queue(dev, skb);
>>>>>>   	netdev_features_t features;
>>>>>> +
>>>>>>   	if (!q)
>>>>>>   		goto drop;
>>>>>>
>>>>>> @@ -289,7 +311,7 @@ static int macvtap_forward(struct net_device *dev, struct sk_buff *skb)
>>>>>>   	/* Apply the forward feature mask so that we perform segmentation
>>>>>>   	 * according to users wishes.
>>>>>>   	 */
>>>>>> -	features = netif_skb_features(skb) & vlan->tap_features;
>>>>>> +	features = macvtap_skb_features(skb, q);
>>>>>>   	if (netif_needs_gso(skb, features)) {
>>>>>>   		struct sk_buff *segs = __skb_gso_segment(skb, features, false);
>>>>>>
>>>>>> @@ -1161,10 +1183,6 @@ static long macvtap_ioctl(struct file *file, unsigned int cmd,
>>>>>>   			    TUN_F_TSO_ECN | TUN_F_UFO))
>>>>>>   			return -EINVAL;
>>>>>>
>>>>>> -		/* TODO: only accept frames with the features that
>>>>>> -			 got enabled for forwarded frames */
>>>>>> -		if (!(q->flags & IFF_VNET_HDR))
>>>>>> -			return  -EINVAL;
>>>>>>   		rtnl_lock();
>>>>>>   		ret = set_offload(q, arg);
>>>>>>   		rtnl_unlock();
>>>>>> --
>>>>>> 1.8.1.4