| Message ID | 1387167311-14763-4-git-send-email-xiyou.wangcong@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On 12/15/13 23:15, Cong Wang wrote: > When the target device is removed, the mirred action is > still there but with the dev pointer setting to NULL. > This makes the output from 'tc filter' ugly. There is no > reason to keep it. > Sorry - this one i have problems with. actions may be referenced from multiple filters, you cant just delete it (that would leave other users pointing to it dangling). Destroying would eventually delete it when the refcount goes to 0. [And when we delete actions we send netlink events to announce that]. The proper solution would require i.e tag it as to be deleted and implement some form of garbage collection. Do you mind leaving this one out for this series? cheers, jamal > Cc: Jamal Hadi Salim <jhs@mojatatu.com> > Cc: David S. Miller <davem@davemloft.net> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> > --- > include/net/tc_act/tc_mirred.h | 4 +++- > net/sched/act_mirred.c | 15 +++++++-------- > 2 files changed, 10 insertions(+), 9 deletions(-) > > diff --git a/include/net/tc_act/tc_mirred.h b/include/net/tc_act/tc_mirred.h > index cfe2943..2026cf6 100644 > --- a/include/net/tc_act/tc_mirred.h > +++ b/include/net/tc_act/tc_mirred.h > @@ -7,9 +7,11 @@ struct tcf_mirred { > struct tcf_common common; > int tcfm_eaction; > int tcfm_ifindex; > - int tcfm_ok_push; > + unsigned int tcfm_ok_push:1; > + unsigned int tcfm_bind:1; > struct net_device *tcfm_dev; > struct list_head tcfm_list; > + struct tc_action *tcfm_act; > }; > #define to_mirred(pc) \ > container_of(pc, struct tcf_mirred, common) > diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c > index 2523781..c26bfe5 100644 > --- a/net/sched/act_mirred.c > +++ b/net/sched/act_mirred.c > @@ -136,6 +136,8 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla, > dev_hold(dev); > m->tcfm_dev = dev; > m->tcfm_ok_push = ok_push; > + m->tcfm_bind = bind; > + m->tcfm_act = a; > } > spin_unlock_bh(&m->tcf_lock); > if (ret == ACT_P_CREATED) { > @@ -169,10 +171,6 @@ static int tcf_mirred(struct sk_buff *skb, const struct tc_action *a, > bstats_update(&m->tcf_bstats, skb); > > dev = m->tcfm_dev; > - if (!dev) { > - printk_once(KERN_NOTICE "tc mirred: target device is gone\n"); > - goto out; > - } > > if (!(dev->flags & IFF_UP)) { > net_notice_ratelimited("tc mirred to Houston: device %s is down\n", > @@ -244,13 +242,14 @@ static int mirred_device_event(struct notifier_block *unused, > unsigned long event, void *ptr) > { > struct net_device *dev = netdev_notifier_info_to_dev(ptr); > - struct tcf_mirred *m; > + struct tcf_mirred *m, *tmp; > > if (event == NETDEV_UNREGISTER) > - list_for_each_entry(m, &mirred_list, tcfm_list) { > + list_for_each_entry_safe(m, tmp, &mirred_list, tcfm_list) { > if (m->tcfm_dev == dev) { > - dev_put(dev); > - m->tcfm_dev = NULL; > + list_del(&m->tcfm_act->list); > + tcf_mirred_cleanup(m->tcfm_act, m->tcfm_bind); > + kfree(m->tcfm_act); > } > } > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Dec 18, 2013 at 6:31 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote: > On 12/15/13 23:15, Cong Wang wrote: >> >> When the target device is removed, the mirred action is >> still there but with the dev pointer setting to NULL. >> This makes the output from 'tc filter' ugly. There is no >> reason to keep it. >> > > Sorry - this one i have problems with. > actions may be referenced from multiple filters, > you cant just delete it (that would leave other users > pointing to it dangling). Destroying would eventually > delete it when the refcount goes to 0. How? tcf_action_init() always allocates a new action, it doesn't even look for an existing one. > [And when we delete actions we send netlink events to announce > that]. The proper solution would require i.e tag it as to be > deleted and implement some form of garbage collection. It doesn't worth to have a GC for this... Even if what you said is true, we should just make a copy for each of the filters. I just tried to create two different filters with same and different mirred actions, my patch works perfectly. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Cong Wang <xiyou.wangcong@gmail.com> Date: Wed, 18 Dec 2013 10:36:06 -0800 > On Wed, Dec 18, 2013 at 6:31 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote: >> On 12/15/13 23:15, Cong Wang wrote: >>> >>> When the target device is removed, the mirred action is >>> still there but with the dev pointer setting to NULL. >>> This makes the output from 'tc filter' ugly. There is no >>> reason to keep it. >>> >> >> Sorry - this one i have problems with. >> actions may be referenced from multiple filters, >> you cant just delete it (that would leave other users >> pointing to it dangling). Destroying would eventually >> delete it when the refcount goes to 0. > > How? tcf_action_init() always allocates a new action, > it doesn't even look for an existing one. > >> [And when we delete actions we send netlink events to announce >> that]. The proper solution would require i.e tag it as to be >> deleted and implement some form of garbage collection. > > It doesn't worth to have a GC for this... > > Even if what you said is true, we should just make a copy > for each of the filters. I just tried to create two different filters > with same and different mirred actions, my patch works > perfectly. Indeed, I think Jamal is confusing how he intended the code to work vs. how it actually does :-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/18/13 13:36, Cong Wang wrote: > On Wed, Dec 18, 2013 at 6:31 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote: >> On 12/15/13 23:15, Cong Wang wrote: >>> >>> When the target device is removed, the mirred action is >>> still there but with the dev pointer setting to NULL. >>> This makes the output from 'tc filter' ugly. There is no >>> reason to keep it. >>> >> >> Sorry - this one i have problems with. >> actions may be referenced from multiple filters, >> you cant just delete it (that would leave other users >> pointing to it dangling). Destroying would eventually >> delete it when the refcount goes to 0. > > How? tcf_action_init() always allocates a new action, > it doesn't even look for an existing one. > tc action blah index 123 tc action filter goo action blah index 123 tc action filter gah action blah index 123 Very useful for example for multiple flows to share the same policer. >> [And when we delete actions we send netlink events to announce >> that]. The proper solution would require i.e tag it as to be >> deleted and implement some form of garbage collection. > > It doesn't worth to have a GC for this... > > Even if what you said is true, we should just make a copy > for each of the filters. I just tried to create two different filters > with same and different mirred actions, my patch works > perfectly. > A copy wont work. We want sharing. cheers, jamal -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Dec 18, 2013 at 11:50 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote: > On 12/18/13 13:36, Cong Wang wrote: >> >> On Wed, Dec 18, 2013 at 6:31 AM, Jamal Hadi Salim <jhs@mojatatu.com> >> wrote: >>> >>> On 12/15/13 23:15, Cong Wang wrote: >>>> >>>> >>>> When the target device is removed, the mirred action is >>>> still there but with the dev pointer setting to NULL. >>>> This makes the output from 'tc filter' ugly. There is no >>>> reason to keep it. >>>> >>> >>> Sorry - this one i have problems with. >>> actions may be referenced from multiple filters, >>> you cant just delete it (that would leave other users >>> pointing to it dangling). Destroying would eventually >>> delete it when the refcount goes to 0. >> >> >> How? tcf_action_init() always allocates a new action, >> it doesn't even look for an existing one. >> > > > tc action blah index 123 > tc action filter goo action blah index 123 > tc action filter gah action blah index 123 > > Very useful for example for multiple flows to > share the same policer. > Ah, I see. I will create a test case for this and rework on the mirred patch. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
I am sorry Cong - I will still object to this change. I dont want to even bother testing it. You are making some serious policy decisions in the kernel. Such policy decisions should be made by user space not the kernel. Whoever made the idiotic decision of removing the device should modify or delete the flow rule - at minimal they may deserve some warning. Deleting the action is wrong. It is simple graph theory. Slightly related is this - look at the attached test (I went out of my way to annotate it) and imagine you have mirred where the policers are .... cheers, jamal On 12/18/13 16:42, Cong Wang wrote: > On Wed, Dec 18, 2013 at 11:50 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote: >> On 12/18/13 13:36, Cong Wang wrote: >>> >>> On Wed, Dec 18, 2013 at 6:31 AM, Jamal Hadi Salim <jhs@mojatatu.com> >>> wrote: >>>> >>>> On 12/15/13 23:15, Cong Wang wrote: >>>>> >>>>> >>>>> When the target device is removed, the mirred action is >>>>> still there but with the dev pointer setting to NULL. >>>>> This makes the output from 'tc filter' ugly. There is no >>>>> reason to keep it. >>>>> >>>> >>>> Sorry - this one i have problems with. >>>> actions may be referenced from multiple filters, >>>> you cant just delete it (that would leave other users >>>> pointing to it dangling). Destroying would eventually >>>> delete it when the refcount goes to 0. >>> >>> >>> How? tcf_action_init() always allocates a new action, >>> it doesn't even look for an existing one. >>> >> >> >> tc action blah index 123 >> tc action filter goo action blah index 123 >> tc action filter gah action blah index 123 >> >> Very useful for example for multiple flows to >> share the same policer. >> > > Ah, I see. I will create a test case for this > and rework on the mirred patch. > TC="/sbin/tc" # sudo ifconfig dummy0 up sudo ifconfig dummy1 up DEV1="eth0" DEV2="eth1" # # # sudo $TC qdisc del dev $DEV1 ingress sudo $TC qdisc add dev $DEV1 ingress sudo $TC qdisc del dev $DEV2 ingress sudo $TC qdisc add dev $DEV2 ingress #Note - older tc did not pipe for skbedit (add "pipe" if this is rejected) sudo $TC filter add dev $DEV1 parent ffff: protocol ip pref 10 \ u32 match ip protocol 1 0xff flowid 1:10 \ action skbedit mark 11 \ action police rate 10kbit burst 10k pipe index 1 \ action skbedit mark 12 \ action police rate 20kbit burst 20k pipe index 2 \ action action mirred egress mirror dev dummy0 #note sharing of the two policers by two flows #across multiple devices sudo $TC filter add dev $DEV2 parent ffff: protocol ip pref 10 \ u32 match ip protocol 1 0xff flowid 1:10 \ action skbedit mark 21 \ action police rate 10kbit burst 10k pipe index 1 \ action skbedit mark 22 \ action police rate 20kbit burst 20k pipe index 2 \ action action mirred egress mirror dev dummy1 # # # now do a ping -f to somewhere, for more exciting results # do a -s 2048 or so # sudo ping -f 192.168.1.100 -c 10000 # sudo ping -f 127.0.0.1 -c 10000 # sudo tc -s filter ls dev $DEV1 parent ffff: protocol ip sudo tc -s filter ls dev $DEV2 parent ffff: protocol ip sudo tc -s actions ls action police sudo tc -s actions ls action skbedit
On Sun, Dec 22, 2013 at 8:15 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote: > > > I am sorry Cong - I will still object to this change. I dont want > to even bother testing it. > You are making some serious policy decisions in the kernel. > Such policy decisions should be made by user space not the kernel. You know qdiscs and filters are removed too when the device is gone, right? So isn't that also a policy you are talking about? This doesn't make any sense to me, if it did, you should remove all net device notifications in kernel, right? > Whoever made the idiotic decision of removing the device should > modify or delete the flow rule - at minimal they > may deserve some warning. Deleting the action is wrong. It is simple > graph theory. Have you ever thought about how hard it is to remove a mirred action upon the device removal? Even with libnl, we still have to: 1) monitor the device removal notification 2) search the action cache to get the action matches the target device 3) search for the filters that contains such actions 4) remove the action by changing the filters it is attached Try it and see how much more work you will do, compare it with this kernel patch. It would not be hard to conclude which is easier. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/22/13 14:42, Cong Wang wrote: > On Sun, Dec 22, 2013 at 8:15 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote: > You know qdiscs and filters are removed too when the device > is gone, right? So isn't that also a policy you are talking about? > That is an easy optimization that made sense to make - we deleted the root of a graph. No need to spam the policy manager. What we are talking about is deleting a faulty node shared by multiple graphs and deleting the vertex but then leaving dangling edges around. Doesnt make sense. No action that is shared between two flows or devices is EVER going to be removed because you deleted a qdisc or a netdev. > This doesn't make any sense to me, if it did, you should remove > all net device notifications in kernel, right? > Refer to above. > Have you ever thought about how hard it is to remove a mirred action > upon the device removal? Even with libnl, we still have to: > > 1) monitor the device removal notification > 2) search the action cache to get the action matches the target device > 3) search for the filters that contains such actions > 4) remove the action by changing the filters it is attached > > Try it and see how much more work you will do, compare it with > this kernel patch. It would not be hard to conclude which is easier. > It is not about ease - it is about correctness. You are making a macroscopic decision with a microscopic view. This is why we have control planes. It is like the legs making decisions on behalf of the brain. cheers, jamal -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Dec 22, 2013 at 12:31 PM, Jamal Hadi Salim <jhs@mojatatu.com> wrote: > On 12/22/13 14:42, Cong Wang wrote: >> >> On Sun, Dec 22, 2013 at 8:15 AM, Jamal Hadi Salim <jhs@mojatatu.com> >> wrote: > > >> You know qdiscs and filters are removed too when the device >> is gone, right? So isn't that also a policy you are talking about? >> > > That is an easy optimization that made sense to make - we > deleted the root of a graph. No need to spam the policy manager. > What we are talking about is deleting a faulty node shared > by multiple graphs and deleting the vertex but then leaving > dangling edges around. Doesnt make sense. > No action that is shared between two flows or devices is EVER > going to be removed because you deleted a qdisc or a netdev. NOTHING you talked about here is relevant to the policy or mechanism you talked previously. Just correctness. If removing an shared action is impossible, what about non-shared ones? Actually for my _own_ use case, I even don't use nor care about shared one... -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/22/13 16:11, Cong Wang wrote: > NOTHING you talked about here is relevant to the policy or > mechanism you talked previously. Just correctness. > It is about _correct policy_ If you have a packets going: ->A-->B-->C-->D-->E That is the policy (decided by someone/thing in user space). A to E are mechanisms. If something goes wrong with mechanism D, you dont go deleting D because it is affecting other things in the graph. You have no clue what it would affect - and if you try to build that in it is not cheap for no good reason. You let the thing/person who set it know and do the deletion. You compared it to something going wrong with A(when A is netdev) and how we delete everything underneath. They are not the same, in that case, we know precisely that is the begining of the graph and we can unreference everything underneath. Does that make more sense? > If removing an shared action is impossible, what about non-shared > ones? Actually for my _own_ use case, I even don't use nor care > about shared one... > Refer to above. cheers, jamal -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Dec 23, 2013 at 4:41 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote: > On 12/22/13 16:11, Cong Wang wrote: > >> NOTHING you talked about here is relevant to the policy or >> mechanism you talked previously. Just correctness. >> > > It is about _correct policy_ > If you have a packets going: > ->A-->B-->C-->D-->E > > That is the policy (decided by someone/thing in user space). > A to E are mechanisms. Apparently you have a different definition of "policy" and "mechanism" with me, probably with others too... I assume you mean actions here, since we don't care about others in this thread. > If something goes wrong with mechanism D, you dont go deleting > D because it is affecting other things in the graph. You have > no clue what it would affect - and if you try to build that in > it is not cheap for no good reason. Since they are chained by a singly or doubly linked list in non-shared case, where are "other things in the graph"? Please do explain how can they be in a graph in non-shared case, it is not obvious to me at all. > You let the thing/person who set it know and do the deletion. > > You compared it to something going wrong with A(when A is netdev) > and how we delete everything underneath. They are not the same, > in that case, we know precisely that is the begining of the graph > and we can unreference everything underneath. Nope, I only want to remove A from the chain by list_del() for non-shared case. > > Does that make more sense? > Nope, you continue to mention graph, but don't explain why there is a graph in non-shared case. Nor I think you use "policy" or "mechanism" correctly. :) Jamal, please don't be abstract, just talk about actions. We DO NOT care about others in this thread. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/23/13 13:50, Cong Wang wrote: > On Mon, Dec 23, 2013 at 4:41 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote: >> >> It is about _correct policy_ >> If you have a packets going: >> ->A-->B-->C-->D-->E >> >> That is the policy (decided by someone/thing in user space). >> A to E are mechanisms. > > Apparently you have a different definition of "policy" and "mechanism" > with me, probably with others too... > I think only with you ;-> > I assume you mean actions here, since we don't care about others > in this thread. > [..] > > Since they are chained by a singly or doubly linked list in > non-shared case, where are "other things in the graph"? > Nothing to do with implementation - think conceptually. > Please do explain how can they be in a graph in non-shared > case, it is not obvious to me at all. > [..] > Nope, I only want to remove A from the chain by list_del() for non-shared > case. > [..] > > Nope, you continue to mention graph, but don't explain why there is > a graph in non-shared case. Nor I think you use "policy" or "mechanism" > correctly. :) > > Jamal, please don't be abstract, just talk about actions. We DO NOT > care about others in this thread. > It is not just actions that constitute the graph. It starts at netdev. Actions are more complex because you have a real DAG. i.e you can have branches etc. So how best to explain this? Ive gone the avenue of showing you shared actions and failed. So i am trying to simplify it to a single basic graph;-> I will try again to use the graph concept: If i or some program decide that a packet coming on is going to traverse: -->netdevX->qdiscA->classifierB->{ActionC->Action D->ActionE} That is a policy. It tells different parts of the kernel(the mechanisms) how to treat a packet that traverses them in order to create some resultant service. What i drew above is a graph (as noted above {} is more complex because you could have branches etc, eg i could go on Action C to do sometimes but skip to E other times based on state, actually you could have that with classifiers - but lets ignore that for now). Each of the things preceeded by "->" is a graph node, otherwise known as a vertex (eg netdevX). Each of the "->" is an known as a graph edge. Each of the nodes in a graph is a mechanism. If this still doesnt make sense to you, I can describe it differently without using concept of graphs. Now if you have such a graph: User space can delete the node netdevX and then you (kernel) can dereference all that is underneath it. That is a good optimization since user just deleted the root; we can then notify user only about netdevX. It doesnt matter if Action D is shared; reference count will decrease and if it hits zero, real destroy will happen. User can delete the qdiscA and then kernel can dereference everything underneath. That is fair optimization as above. User can delete the filter and kernel can dereference everything underneath it. That is also fair optimization as above. What you cannot do is, on your own as kernel, decide you want to delete one action that is _bound_ to a filter because one of its attributes is gone berserk. It doesnt matter whether such an action is mirred or foo or whether D and E dont exist. You can put a big hole where the town used to be and leave roads leading to the town. It will still be referenced by things preeceding it (primarily the classifier which keeps an action chain intact), which is bad when the next packet arrives. You let the user/control program which is monitoring things do that and "reroute" around the problem. If you insist on putting a little part of the medula oblangata in the stomach, then: the only correct way to do it is delete the filter. And you start doing that you are making some serious policy decisions in the kernel and adding lots of complexity. Ok, does it make sense now?;-> cheers, jamal -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Dec 23, 2013 at 12:41 PM, Jamal Hadi Salim <jhs@mojatatu.com> wrote: > > Now if you have such a graph: Making such an abstraction only helps misunderstanding, really... > What you cannot do is, on your own as kernel, decide you want to delete > one action that is _bound_ to a filter because one of its attributes is > gone berserk. It doesnt matter whether such an action is mirred or foo > or whether D and E dont exist. You can put a big hole where the town > used to be and leave roads leading to the town. Again, since (non-shared) actions attached to a filter are chained by a doubly linked list, why not? > It will still be referenced by things preeceding it (primarily the > classifier which keeps an action chain intact), which is bad when the > next packet arrives. You know, there is a head for such linked list for the filter to refer, so what's the problem with deleting a node from a linked list if we lock it properly? In non-shared case, no filter can refer to any action without the head of the chain, right? So what is the problem of "referenced by things preeceding it" when they are linked and locked properly? > You let the user/control program which is monitoring things do that > and "reroute" around the problem. If you insist on putting a little part > of the medula oblangata in the stomach, then: > the only correct way to do it is delete the filter. Apparent not, different actions can attached to the same filter and only mirred action has a target dev. > And you start doing that you are making some serious policy decisions > in the kernel and adding lots of complexity. > Why removing a filter when the netdev is removed is not policy? Actually it is, ONLY that it is not be able to shared with current implementation. Since you love mechanism _so much_, why not make filters shared by other qdisc's and stop removing them when netdev is gone in kernel? Be realistic, Jamal, user-space is hard, you can't simply let user-space decide everything, especially when you don't provide a simple way to do it. Netlink is already hard to use, even with libnl, since it enforces a cache layer. Sit down and spend some time to write some libnl code, compare it with this patch. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/23/13 17:14, Cong Wang wrote: > On Mon, Dec 23, 2013 at 12:41 PM, Jamal Hadi Salim <jhs@mojatatu.com> wrote: >> >> Now if you have such a graph: > > Making such an abstraction only helps misunderstanding, really... > So I used a graph that maps to a netdevice, qdisc, filter and actions like you asked to - there is still a misunderstanding? >> What you cannot do is, on your own as kernel, decide you want to delete >> one action that is _bound_ to a filter because one of its attributes is >> gone berserk. It doesnt matter whether such an action is mirred or foo >> or whether D and E dont exist. You can put a big hole where the town >> used to be and leave roads leading to the town. > > Again, since (non-shared) actions attached to a filter are chained by > a doubly linked list, why not? > >> It will still be referenced by things preeceding it (primarily the >> classifier which keeps an action chain intact), which is bad when the >> next packet arrives. > > You know, there is a head for such linked list for the filter to refer, > so what's the problem with deleting a node from a linked list if > we lock it properly? In non-shared case, no filter can refer to any > action without the head of the chain, right? So what is the problem > of "referenced by things preeceding it" when they are linked and locked > properly? > Huh? This has nothing to do with whether you are capable to remove a node from a list. It is about you making a decision that the node should be removed. You dont have sufficient information to make such a decision. Remember earlier i said you are making a macroscopic decision by looking at microscope? I just saw a white blood cell, let me chop that toe. >> You let the user/control program which is monitoring things do that >> and "reroute" around the problem. If you insist on putting a little part >> of the medula oblangata in the stomach, then: >> the only correct way to do it is delete the filter. > > Apparent not, different actions can attached to the same filter > and only mirred action has a target dev. > Parse error. >> And you start doing that you are making some serious policy decisions >> in the kernel and adding lots of complexity. >> > > Why removing a filter when the netdev is removed is not policy? > Actually it is, ONLY that it is not be able to shared with current > implementation. > Of course it is. Since you understand that why do you think removing an action which is part of that policy is a good idea? > Since you love mechanism _so much_, why not make filters shared > by other qdisc's and stop removing them when netdev is gone in kernel? > > Be realistic, Jamal, user-space is hard, you can't simply let user-space > decide everything, especially when you don't provide a simple way to do it. > Netlink is already hard to use, even with libnl, since it enforces a cache > layer. Sit down and spend some time to write some libnl code, compare it > with this patch. > You gotta be joking. I live through this every ither day. Frankly, I think i will have no progress if i tried to convince you the world is round. I am dropping from this discussion. cheers, jamal -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Dec 23, 2013 at 2:30 PM, Jamal Hadi Salim <jhs@mojatatu.com> wrote: > > Huh? This has nothing to do with whether you are capable to remove a > node from a list. It is about you making a decision that the node should > be removed. You dont have sufficient information to make such a decision. Then which lacks? >> Why removing a filter when the netdev is removed is not policy? >> Actually it is, ONLY that it is not be able to shared with current >> implementation. >> > > Of course it is. Since you understand that why do you think removing > an action which is part of that policy is a good idea? > Well, I think doing such policy decision for both filters and actions in kernel IS fine, simply because it is harder for user-space. We already have many of such policies, they fit well. > >> Since you love mechanism _so much_, why not make filters shared >> by other qdisc's and stop removing them when netdev is gone in kernel? >> >> Be realistic, Jamal, user-space is hard, you can't simply let user-space >> decide everything, especially when you don't provide a simple way to do >> it. >> Netlink is already hard to use, even with libnl, since it enforces a cache >> layer. Sit down and spend some time to write some libnl code, compare it >> with this patch. >> > > You gotta be joking. I live through this every ither day. Great! Then show me how to do remove a mirred action upon the target device is gone in user-space, please? > Frankly, I think i will have no progress if i tried to convince you the > world is round. I am dropping from this discussion. > Your replies show you care about policy vs mechanism very much, but you even don't do any thing with either 1) try to show me how the user-space implements it or 2) go head to make same mechanism for filters (and qdisc) as well. Do something to prove that you said. :) Otherwise, I think you are kidding me. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/23/13 17:51, Cong Wang wrote: [..] > Great! Then show me how to do remove a mirred action upon > the target device is gone in user-space, please? [..] > Your replies show you care about policy vs mechanism very much, > but you even don't do any thing with either 1) try to show me how > the user-space implements it or 2) go head to make same mechanism > for filters (and qdisc) as well. > > Do something to prove that you said. :) Otherwise, I think you are kidding > me. > No disrespect intended Cong - but lets just end this discussion agreeing to disagree. On my side I violently disagree. I hope not to spend any more energy on this. cheers, jamal -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Dec 23, 2013 at 3:05 PM, Jamal Hadi Salim <jhs@mojatatu.com> wrote: > > No disrespect intended Cong - but lets just end this discussion agreeing > to disagree. On my side I violently disagree. I hope not to spend any > more energy on this. > Sorry, I think your argument on policy vs mechanism doesn't make any sense at all. Therefore I don't accept it. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/net/tc_act/tc_mirred.h b/include/net/tc_act/tc_mirred.h index cfe2943..2026cf6 100644 --- a/include/net/tc_act/tc_mirred.h +++ b/include/net/tc_act/tc_mirred.h @@ -7,9 +7,11 @@ struct tcf_mirred { struct tcf_common common; int tcfm_eaction; int tcfm_ifindex; - int tcfm_ok_push; + unsigned int tcfm_ok_push:1; + unsigned int tcfm_bind:1; struct net_device *tcfm_dev; struct list_head tcfm_list; + struct tc_action *tcfm_act; }; #define to_mirred(pc) \ container_of(pc, struct tcf_mirred, common) diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c index 2523781..c26bfe5 100644 --- a/net/sched/act_mirred.c +++ b/net/sched/act_mirred.c @@ -136,6 +136,8 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla, dev_hold(dev); m->tcfm_dev = dev; m->tcfm_ok_push = ok_push; + m->tcfm_bind = bind; + m->tcfm_act = a; } spin_unlock_bh(&m->tcf_lock); if (ret == ACT_P_CREATED) { @@ -169,10 +171,6 @@ static int tcf_mirred(struct sk_buff *skb, const struct tc_action *a, bstats_update(&m->tcf_bstats, skb); dev = m->tcfm_dev; - if (!dev) { - printk_once(KERN_NOTICE "tc mirred: target device is gone\n"); - goto out; - } if (!(dev->flags & IFF_UP)) { net_notice_ratelimited("tc mirred to Houston: device %s is down\n", @@ -244,13 +242,14 @@ static int mirred_device_event(struct notifier_block *unused, unsigned long event, void *ptr) { struct net_device *dev = netdev_notifier_info_to_dev(ptr); - struct tcf_mirred *m; + struct tcf_mirred *m, *tmp; if (event == NETDEV_UNREGISTER) - list_for_each_entry(m, &mirred_list, tcfm_list) { + list_for_each_entry_safe(m, tmp, &mirred_list, tcfm_list) { if (m->tcfm_dev == dev) { - dev_put(dev); - m->tcfm_dev = NULL; + list_del(&m->tcfm_act->list); + tcf_mirred_cleanup(m->tcfm_act, m->tcfm_bind); + kfree(m->tcfm_act); } }
When the target device is removed, the mirred action is still there but with the dev pointer setting to NULL. This makes the output from 'tc filter' ugly. There is no reason to keep it. Cc: Jamal Hadi Salim <jhs@mojatatu.com> Cc: David S. Miller <davem@davemloft.net> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> --- include/net/tc_act/tc_mirred.h | 4 +++- net/sched/act_mirred.c | 15 +++++++-------- 2 files changed, 10 insertions(+), 9 deletions(-)