qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH RFC v2 3/5] net/filter: Introduce a helper to ad


From: Jason Wang
Subject: Re: [Qemu-devel] [PATCH RFC v2 3/5] net/filter: Introduce a helper to add a filter to the netdev
Date: Fri, 5 Feb 2016 15:40:30 +0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1


On 02/05/2016 03:01 PM, Hailiang Zhang wrote:
> On 2016/2/5 14:19, Jason Wang wrote:
>>
>>
>> On 02/01/2016 06:40 PM, Hailiang Zhang wrote:
>>> On 2016/2/1 17:42, Jason Wang wrote:
>>>>
>>>>
>>>> On 02/01/2016 05:22 PM, Hailiang Zhang wrote:
>>>>> On 2016/2/1 17:04, Jason Wang wrote:
>>>>>>
>>>>>>
>>>>>> On 02/01/2016 03:56 PM, Hailiang Zhang wrote:
>>>>>>> On 2016/2/1 15:46, Jason Wang wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 02/01/2016 02:13 PM, Hailiang Zhang wrote:
>>>>>>>>> On 2016/2/1 11:14, Jason Wang wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 01/27/2016 04:29 PM, zhanghailiang wrote:
>>>>>>>>>>> We add a new helper function netdev_add_filter(), this function
>>>>>>>>>>> can help adding a filter object to a netdev.
>>>>>>>>>>> Besides, we add a is_default member for struct NetFilterState
>>>>>>>>>>> to indicate whether the filter is default or not.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: zhanghailiang <address@hidden>
>>>>>>>>>>> ---
>>>>>>>>>>> v2:
>>>>>>>>>>>       -Re-implement netdev_add_filter() by re-using
>>>>>>>>>>> object_create()
>>>>>>>>>>>        (Jason's suggestion)
>>>>>>>>>>> ---
>>>>>>>>>>>       include/net/filter.h |  7 +++++
>>>>>>>>>>>       net/filter.c         | 80
>>>>>>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>>>>>       2 files changed, 87 insertions(+)
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>>>>>> +
>>>>>>>>>>> +    optarg =
>>>>>>>>>>> g_strdup_printf("qom-type=%s,id=%s,netdev=%s,status=%s",
>>>>>>>>>>> +            filter_type, id, netdev_id, is_default ?
>>>>>>>>>>> "disable" :
>>>>>>>>>>> "enable"
>>>>>>>>>>
>>>>>>>>>> Instead of this, I wonder maybe it's better to:
>>>>>>>>>>
>>>>>>>>>> - store the default filter property into a pointer to string
>>>>>>>>>
>>>>>>>>> Do you mean, pass a string parameter which stores the filter
>>>>>>>>> property
>>>>>>>>> instead of
>>>>>>>>> assemble it in this helper ?
>>>>>>>>
>>>>>>>> Yes. E.g just a global string which could be changed by any
>>>>>>>> subsystem.
>>>>>>>> E.g colo may change it to
>>>>>>>> "filter-buffer,interval=0,status=disable". But
>>>>>>>> filter ids need to be generated automatically.
>>>>>>>>
>>>>>>>
>>>>>>> Got it. Then we don't need the global default_netfilter_type[] in
>>>>>>> patch 5,
>>>>>>
>>>>>> Yes.
>>>>>>
>>>>>>> Just use this global string instead ?
>>>>>>
>>>>>> Right.
>>>>>>
>>>>>>>
>>>>>>>>>
>>>>>>>>>> - colo code may change the pointer to
>>>>>>>>>> "filter-buffer,status=disable"
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> Then, there's no need for lots of codes above:
>>>>>>>>>> - no need a "is_default" parameter in netdev_add_filter which
>>>>>>>>>> does not
>>>>>>>>>> scale consider we may want to have more property in the future
>>>>>>>>>> - no need to hacking like "qemu_filter_opts"
>>>>>>>>>
>>>>>>>>> Yes, we can use qemu_find_opts("object") instead of it.
>>>>>>>>>
>>>>>>>>>> - no need to have a special flag like "is_default"
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> But we have to distinguish the default filter from the common
>>>>>>>>> filter, use the name (id) to distinguish it ?
>>>>>>>>
>>>>>>>> What's the reason that you want to distinguish default filters
>>>>>>>> from
>>>>>>>> others?
>>>>>>>>
>>>>>>>
>>>>>>> The default filters will be used by COLO or MC, (In COLO, we will
>>>>>>> use it
>>>>>>> to control packets buffering/releasing).
>>>>>>
>>>>>> A question is how will you do this?
>>>>>>
>>>>>
>>>>> Er, for COLO, we will enable all the default filter in the
>>>>> initialization stage,
>>>>> then the buffer-filter will buffer all netdev's packets,
>>>>> after doing a checkpoint, we will release all the buffered packets
>>>>> (Flush all default
>>>>> filters' packets).
>>>>
>>>> Right, that's the point. So looks several choices here:
>>>>
>>>> 1) Track each default filter explicitly, generate and record the
>>>> netdev
>>>> ids for default filter by COLO.  Walk through the ids list and release
>>>> the packet in each filter.
>>>> 2) Track the default filters implicitly. Link all buffer filters (with
>>>> zero interval) in a list during filter buffer initialization. And
>>>> export
>>>> a helper for COLO to walk them and release packets.
>>>>
>>>> Either looks fine, but maybe 2 is easier.
>>>>
>>>
>>> 2) is a good choice, but I'm a little worry about using zero to
>>> distinguish if a filter is default filter or not, because users
>>> can use qom-set to change its value.
>>
>> Then I see minor issues:
>>
>
> Great, you are still online :)

Right :)

>
>> 1) Looks like we should prevent user other than COLO from using zero
>> interval, neither from cli nor 'qom-set'.
>> 2) For the zero interval filter used by COLO, we should not allow user
>> to change the value of interval.
>>
>> So I was thinking a new type which is based on current filter-buffer
>> whose interval is invisible to user.
>>
>
> It seems that, we have prevented this case, if we set the interval
> to zero through qom-set, it will report error:
> "Property 'filter-buffer.interval' requires a positive value", also if
> launch qemu with CLI "-object
> filter-buffer,id=f0,netdev=hn0,queue=rx,interval=0",
> it also reported the error "Property 'filter-buffer.interval' requires
> a positive value".
>
> This is because we judge this value in filter_buffer_set_interval(),
> but the 'interval' property is optional, users can use ignore this
> property while launch
> qemu and its default value will be zero, so we still can't use this
> value to identify default
> filter.

Cool, good to know this.

>
> Other choices to drop the troublesome 'is_default' flag is,
> 1) use a list to store all the default filters, add them to the list in
> netdev_add_default_filters(). but we need add a new member
> 'QTAILQ_ENTRY(NetFilterState) internal_next' to struct NetFilterState,
> The benefit of this scheme is, it will more easy to traverse and control
> the default filters, we don't have to traverse all netfilters to
> identify them.

This is pretty similar to what I suggested above.  The only difference
is that you propose to track this at filter layer instead of buffer
filter implementation.

Think hard about this. Consider:

1) Buffer is the only user for something like default filter and there's
no plan for exporting default filter configuration through cli (at least
in short term).
2) All default filter buffer needs to be tracked in a list for COLO to
buffer/relase packets.

Adding the ability of default filter is good but may need more thoughts.
For example, cli design and to be generic enough for all kinds of filter
(you may want to generate filename dynamically for dump as a default
filter which sounds bad). To reduce the extra efforts and converge this
soon , maybe we could do something more simpler.

- Instead of a explicit default filter, using something like
notifier/callback in net_dev_init(). Then COLO can register a callback
for this notifier. Each time a netdev is created, COLO will be notified
and can do whatever it wants (E.g adding a buffer filter with zero
interval).
- With above, there's no need to care about the zero interval setting
from cli. Then filter buffer can track all filters with zero interval,
and export helpers for COLO to buffer or release the packet.


Thoughts?

Thanks

> 2) Use the name to identify them, more the name rule for default
> filter is
> '[netdev->id]nop'.
>
> Which one do you think is more acceptable ? Or any other suggestions ?
>
> Thanks,
> Hailiang
>
>>> If special flag like 'is_default' is not acceptable,
>>> then we have to use the filter ids to distinguish the default
>>> buffer-filter
>>> (here the rule of generation ids for default filter is
>>> '[netdev-id][DEFAULT_FILTER_TYPE]'
>>>
>>> Thanks,
>>> Hailiang
>>>
>>>>>    If VM is failover, we will set all default filters back to
>>>>> disabled
>>>>> status.
>>>>> (This is a periodic mode for COLO, different from another mode, which
>>>>> we will call it
>>>>> hybrid mode, that is based on colo-proxy, which is in developing by
>>>>> zhangchen)
>>>>>
>>>>> Thanks,
>>>>> Hailiang
>>>>
>>>> Yes, I see.
>>>>
>>>>
>>>> .
>>>>
>>>
>>>
>>
>>
>> .
>>
>
>




reply via email to

[Prev in Thread] Current Thread [Next in Thread]