From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46513) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aRbmV-0005jK-2z for qemu-devel@nongnu.org; Fri, 05 Feb 2016 03:30:20 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aRbmQ-0004kT-6o for qemu-devel@nongnu.org; Fri, 05 Feb 2016 03:30:19 -0500 Received: from szxga01-in.huawei.com ([58.251.152.64]:52808) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aRbmP-0004k1-4g for qemu-devel@nongnu.org; Fri, 05 Feb 2016 03:30:14 -0500 References: <1453883380-10532-1-git-send-email-zhang.zhanghailiang@huawei.com> <1453883380-10532-4-git-send-email-zhang.zhanghailiang@huawei.com> <56AECDA8.6000402@redhat.com> <56AEF788.3080706@huawei.com> <56AF0D63.3010707@redhat.com> <56AF0FC4.7070809@huawei.com> <56AF1F8E.2020604@redhat.com> <56AF23BB.8000203@huawei.com> <56AF2890.4090304@redhat.com> <56AF362E.9000302@huawei.com> <56B43EE0.7080405@redhat.com> <56B448C7.4030503@huawei.com> <56B451EE.8010600@redhat.com> From: Hailiang Zhang Message-ID: <56B45D77.2000100@huawei.com> Date: Fri, 5 Feb 2016 16:29:43 +0800 MIME-Version: 1.0 In-Reply-To: <56B451EE.8010600@redhat.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH RFC v2 3/5] net/filter: Introduce a helper to add a filter to the netdev List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jason Wang , qemu-devel@nongnu.org Cc: peter.huangpeng@huawei.com, zhangchen.fnst@cn.fujitsu.com, hongyang.yang@easystack.cn On 2016/2/5 15:40, Jason Wang wrote: > > > 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 >>>>>>>>>>>> --- >>>>>>>>>>>> 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. > Totally agree, and for now, it seems that only COLO use the buffer filter. After colo-proxy is merged, COLO will switch to colo-proxy. So realizing it in a simple way is a good idea. > - 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). Yes, in this way, we can still support hot-add netfilter, and can also handle it with the later colo-proxy. > - 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. > > Thank you very much for your patience :) > 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. >>>>> >>>>> >>>>> . >>>>> >>>> >>>> >>> >>> >>> . >>> >> >> > > > . >