Re: [net-next PATCH v1 7/7] macvlan: add FDB bridge ops and new macvlan mode

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

On 4/11/2012 1:02 AM, Michael S. Tsirkin wrote:
> On Tue, Apr 10, 2012 at 06:42:47PM -0700, John Fastabend wrote:
>> On 4/10/2012 5:46 PM, Sridhar Samudrala wrote:
>>> On 4/10/2012 8:35 AM, John Fastabend wrote:
>>>> On 4/10/2012 8:30 AM, Michael S. Tsirkin wrote:
>>>>> On Tue, Apr 10, 2012 at 08:26:21AM -0700, John Fastabend wrote:
>>>>>> On 4/10/2012 7:35 AM, Michael S. Tsirkin wrote:
>>>>>>> On Tue, Apr 10, 2012 at 07:25:58AM -0700, John Fastabend wrote:
>>>>>>>>> Hmm okay, but this would mean we should convert
>>>>>>>>> MACVLAN_MODE_PASSTHRU_NOPROMISC to something
>>>>>>>>> that can combined with all modes. E.g.
>>>>>>>>> MACVLAN_MODE_BRIDGE | MACVLAN_MODE_FLAG_XXXXX
>>>>>>>>>
>>>>>>>>> and document that it does not promise to flood
>>>>>>>>> multicast.
>>>>>>>>>
>>>>>>>> How about changing MACVLAN_MODE_PASSTHRU_NOPROMISC ->  MACVLAN_MODE_NOPORMISC
>>>>>>>> for this patch. Then a follow on series can rework bridge
>>>>>>>> and VEPA to use it as well.
>>>>>>> Right. We probably need a better name if it's going to
>>>>>>> affect other things besides promisc though.
>>>>>>>
>>>>>> how about MACVLAN_MODE_FDBFLAG?
>>>>> The idea being that no one figures out what this means so
>>>>> no one will make any wrong assumptions? ;)
>>>>>
>>>> Well its a flag to enable the FDB (forwarding database) ops
>>>> and skip dev_set_promisc() on passthru mode. Any better ideas?
>>>> Maybe MACVLAN_MODE_FDBENABLE or MACVLAN_MODE_MANAGE_FDB?
>>> Do we need to introduce another mode? I think this patch is enabling passthru mode without the need
>>> to put the underlying device in promiscuous mode. So basically we can consider this patch as
>>> an optimization.
>>>
>>> Thanks
>>> Sridhar
>>>
>>
>> Sridhar, Michael,
>>
>> After thinking about this a bit I would propose keeping this
>> patch as is. Or if we prefer I can make this a flag but I don't
>> think that helps much. passthru mode is the only macvlan mode
>> that calls dev_set_promiscuity(). Either way I think this mode
>> or flag should _only_ toggle the call to dev_set_promiscuity().
>>
>> Setting multicast dev->flag IFF_ALLMULTI seems to be a completely
>> separate optimization that we can work with a follow up patch.
>>
>> Any thoughts?
>>
>> Thanks for the feedback,
>> John
> 
> I agree it's a separate optimization. But if we let the
> number of supported modes explode
> (MACVLAN_MODE_PASSTHRU_NOPROMISC MACVLAN_MODE_PASSTHRU_NOALLMULTI
> ....) supporting them all and combinations thereof might become a problem.
> 
> I'm looking for an interface solution that will limit this
> overhead without breaking existing setups.
> 

I'm going to respin this series today. I'll try to rework this with
a flags field so that we can set nopromisc and noallmutli using flags
and not have the supported mode space explode.

> 
> One idea was to have a flag that says basically "really obey device
> configuration".  Yes if we do this we can then split the support to
> multiple patches and consider each individual one a bugfix, though if we
> put a known broken solution in 3.5 there's a danger that someone will
> come to depend on the broken behaviour. Other ideas?
> 
> 

This sound reasonable.

Thanks,
John