All the mail mirrored from lore.kernel.org
 help / color / mirror / Atom feed
* [fw filter]: Broken! fw mark based tc class selection not working
@ 2015-09-11 16:34 Akshat Kakkar
  2015-09-11 20:32 ` Cong Wang
  0 siblings, 1 reply; 7+ messages in thread
From: Akshat Kakkar @ 2015-09-11 16:34 UTC (permalink / raw)
  To: netdev; +Cc: Cong Wang, Stephen Hemminger

Recently I came to know that,
Without any options fw classifier maps fwmark to classid.

tc filter add dev <iface> parent <qhandle> protocol ip prio 1 fw

i.e. if my packet has mark(0x10001) and class id is not set,
then above tc filter, will set class id = 0x10001 i.e. 1:1

But when I am trying it out, its not working!
I am having class 1:1 defined but its not at all hit.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [fw filter]: Broken! fw mark based tc class selection not working
  2015-09-11 16:34 [fw filter]: Broken! fw mark based tc class selection not working Akshat Kakkar
@ 2015-09-11 20:32 ` Cong Wang
  2015-09-11 22:24   ` Akshat Kakkar
  0 siblings, 1 reply; 7+ messages in thread
From: Cong Wang @ 2015-09-11 20:32 UTC (permalink / raw)
  To: Akshat Kakkar; +Cc: netdev, Cong Wang, Stephen Hemminger

On Fri, Sep 11, 2015 at 9:34 AM, Akshat Kakkar <akshat.1984@gmail.com> wrote:
> Recently I came to know that,
> Without any options fw classifier maps fwmark to classid.
>
> tc filter add dev <iface> parent <qhandle> protocol ip prio 1 fw
>
> i.e. if my packet has mark(0x10001) and class id is not set,
> then above tc filter, will set class id = 0x10001 i.e. 1:1
>
> But when I am trying it out, its not working!
> I am having class 1:1 defined but its not at all hit.

Did you specify a right filter handle? What is your setup?

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [fw filter]: Broken! fw mark based tc class selection not working
  2015-09-11 20:32 ` Cong Wang
@ 2015-09-11 22:24   ` Akshat Kakkar
  2015-09-12  0:00     ` Cong Wang
  0 siblings, 1 reply; 7+ messages in thread
From: Akshat Kakkar @ 2015-09-11 22:24 UTC (permalink / raw)
  To: Cong Wang; +Cc: netdev, Cong Wang, Stephen Hemminger

There is no handle with fw filter. That's the whole point is. If
handle and class (flow id) is not specified, then whatever be the mark
on the packet, its automatically set as flowid. So if mark is 0x10003,
then this fw filter

tc filter add dev eth0 parent 1:0 protocol ip fw

will cause 0x10003 being set as classid I.e. 1:3.


tc qdisc add dev eth0 root handle 1: htb
tc class add dev eth0 parent 1: classid 1:a htb rate 1mbit
tc class add dev eth0 parent 1: classid 1:b htb rate 1mbit
tc class add dev eth0 parent 1: classid 1:c htb rate 1mbit
tc filter add dev eth0 parent 1:0 protocol ip fw

iptables -t mangle -I OUTPUT -o eth0 -p tcp -j MARK --set-mark 0x1000a
iptables -t mangle -I OUTPUT -o eth0 -p icmp -j MARK --set-mark 0x1000b
iptables -t mangle -I OUTPUT -o eth0 -p udp -j MARK --set-mark 0x1000c

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [fw filter]: Broken! fw mark based tc class selection not working
  2015-09-11 22:24   ` Akshat Kakkar
@ 2015-09-12  0:00     ` Cong Wang
  2015-09-14 12:28       ` Jamal Hadi Salim
  0 siblings, 1 reply; 7+ messages in thread
From: Cong Wang @ 2015-09-12  0:00 UTC (permalink / raw)
  To: Akshat Kakkar; +Cc: netdev, Cong Wang, Stephen Hemminger, Jamal Hadi Salim

[-- Attachment #1: Type: text/plain, Size: 1050 bytes --]

On Fri, Sep 11, 2015 at 3:24 PM, Akshat Kakkar <akshat.1984@gmail.com> wrote:
> There is no handle with fw filter. That's the whole point is. If
> handle and class (flow id) is not specified, then whatever be the mark
> on the packet, its automatically set as flowid. So if mark is 0x10003,
> then this fw filter
>
> tc filter add dev eth0 parent 1:0 protocol ip fw
>
> will cause 0x10003 being set as classid I.e. 1:3.
>
>
> tc qdisc add dev eth0 root handle 1: htb
> tc class add dev eth0 parent 1: classid 1:a htb rate 1mbit
> tc class add dev eth0 parent 1: classid 1:b htb rate 1mbit
> tc class add dev eth0 parent 1: classid 1:c htb rate 1mbit
> tc filter add dev eth0 parent 1:0 protocol ip fw
>
> iptables -t mangle -I OUTPUT -o eth0 -p tcp -j MARK --set-mark 0x1000a
> iptables -t mangle -I OUTPUT -o eth0 -p icmp -j MARK --set-mark 0x1000b
> iptables -t mangle -I OUTPUT -o eth0 -p udp -j MARK --set-mark 0x1000c


Hmm, I didn't know that before either. Looks like my tp->init change
breaks it.

Could you try the following patch?

Thanks!

[-- Attachment #2: cls_fw.diff --]
[-- Type: text/plain, Size: 971 bytes --]

diff --git a/net/sched/cls_fw.c b/net/sched/cls_fw.c
index 715e01e..b6394ab 100644
--- a/net/sched/cls_fw.c
+++ b/net/sched/cls_fw.c
@@ -34,6 +34,7 @@
 struct fw_head {
 	u32			mask;
 	bool			mask_set;
+	bool			old_method;
 	struct fw_filter __rcu	*ht[HTSIZE];
 	struct rcu_head		rcu;
 };
@@ -65,7 +66,7 @@ static int fw_classify(struct sk_buff *skb, const struct tcf_proto *tp,
 	int r;
 	u32 id = skb->mark;
 
-	if (head != NULL) {
+	if (!head->old_method) {
 		id &= head->mask;
 
 		for (f = rcu_dereference_bh(head->ht[fw_hash(id)]); f;
@@ -120,6 +121,7 @@ static int fw_init(struct tcf_proto *tp)
 	if (head == NULL)
 		return -ENOBUFS;
 
+	head->old_method = true;
 	head->mask_set = false;
 	rcu_assign_pointer(tp->root, head);
 	return 0;
@@ -302,6 +304,7 @@ static int fw_change(struct net *net, struct sk_buff *in_skb,
 	if (!handle)
 		return -EINVAL;
 
+	head->old_method = false;
 	if (!head->mask_set) {
 		head->mask = 0xFFFFFFFF;
 		if (tb[TCA_FW_MASK])

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [fw filter]: Broken! fw mark based tc class selection not working
  2015-09-12  0:00     ` Cong Wang
@ 2015-09-14 12:28       ` Jamal Hadi Salim
  2015-09-14 22:04         ` Cong Wang
  0 siblings, 1 reply; 7+ messages in thread
From: Jamal Hadi Salim @ 2015-09-14 12:28 UTC (permalink / raw)
  To: Cong Wang, Akshat Kakkar; +Cc: netdev, Cong Wang, Stephen Hemminger

On 09/11/15 20:00, Cong Wang wrote:
> On Fri, Sep 11, 2015 at 3:24 PM, Akshat Kakkar <akshat.1984@gmail.com> wrote:

> Hmm, I didn't know that before either. Looks like my tp->init change
> breaks it.
>
> Could you try the following patch?
>

I would just make init() empty for this classifier (return 0?).
If someone wants to add classids ids, change() is available.
The most common (efficient) use case is what Akshat shows.
So even the check in the classify should optimize for that i.e
if (head == NULL)
     do old method
else
     ...

cheers,
jamal

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [fw filter]: Broken! fw mark based tc class selection not working
  2015-09-14 12:28       ` Jamal Hadi Salim
@ 2015-09-14 22:04         ` Cong Wang
  2015-09-17 12:16           ` Jamal Hadi Salim
  0 siblings, 1 reply; 7+ messages in thread
From: Cong Wang @ 2015-09-14 22:04 UTC (permalink / raw)
  To: Jamal Hadi Salim; +Cc: Akshat Kakkar, netdev, Cong Wang, Stephen Hemminger

On Mon, Sep 14, 2015 at 5:28 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> On 09/11/15 20:00, Cong Wang wrote:
>>
>> On Fri, Sep 11, 2015 at 3:24 PM, Akshat Kakkar <akshat.1984@gmail.com>
>> wrote:
>
>
>> Hmm, I didn't know that before either. Looks like my tp->init change
>> breaks it.
>>
>> Could you try the following patch?
>>
>
> I would just make init() empty for this classifier (return 0?).
> If someone wants to add classids ids, change() is available.
> The most common (efficient) use case is what Akshat shows.
> So even the check in the classify should optimize for that i.e
> if (head == NULL)
>     do old method
> else
>     ...

That is exactly the original code. But it is not readable at all,
at least I still missed it when I touched the tp->init() part. :(
Having a boolean doesn't harm anything.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [fw filter]: Broken! fw mark based tc class selection not working
  2015-09-14 22:04         ` Cong Wang
@ 2015-09-17 12:16           ` Jamal Hadi Salim
  0 siblings, 0 replies; 7+ messages in thread
From: Jamal Hadi Salim @ 2015-09-17 12:16 UTC (permalink / raw)
  To: Cong Wang; +Cc: Akshat Kakkar, netdev, Cong Wang, Stephen Hemminger

On 09/14/15 18:04, Cong Wang wrote:

>
> That is exactly the original code. But it is not readable at all,
> at least I still missed it when I touched the tp->init() part. :(
> Having a boolean doesn't harm anything.

The default should really be no head alloced (given that is the
main use case).
The other part you can make more readable.

cheers,
jamal

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2015-09-17 12:16 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-09-11 16:34 [fw filter]: Broken! fw mark based tc class selection not working Akshat Kakkar
2015-09-11 20:32 ` Cong Wang
2015-09-11 22:24   ` Akshat Kakkar
2015-09-12  0:00     ` Cong Wang
2015-09-14 12:28       ` Jamal Hadi Salim
2015-09-14 22:04         ` Cong Wang
2015-09-17 12:16           ` Jamal Hadi Salim

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.