Re: [PATCH v3 net-next 1/1] net_sched: Introduce skbmod action

On 16-09-08 12:11 PM, Eric Dumazet wrote: On Thu, 2016-09-08 at 09:02 -0700, Eric Dumazet wrote: On Thu, 2016-09-08 at 06:38 -0400, Jamal Hadi Salim wrote: On 16-09-06 10:25 AM, Eric Dumazet wrote: This simply works for me. But maybe you read the stats at the wrong place ? Pleas give us

Re: [PATCH v3 net-next 1/1] net_sched: Introduce skbmod action

On Thu, 2016-09-08 at 09:02 -0700, Eric Dumazet wrote: > On Thu, 2016-09-08 at 06:38 -0400, Jamal Hadi Salim wrote: > > On 16-09-06 10:25 AM, Eric Dumazet wrote: > > > > > > > > Just use u16 in the array ? > > > > > > u16 tmpaddr[ETH_ALEN / 2]; /* ether_addr_copy() requirement */ > > > > > >

Re: [PATCH v3 net-next 1/1] net_sched: Introduce skbmod action

On Thu, 2016-09-08 at 06:38 -0400, Jamal Hadi Salim wrote: > On 16-09-06 10:25 AM, Eric Dumazet wrote: > > > > > Just use u16 in the array ? > > > > u16 tmpaddr[ETH_ALEN / 2]; /* ether_addr_copy() requirement */ > > > > ether_addr_copy((u8 *)tmpaddr, eth_hdr(skb)->h_dest); > > ... > > > > Ok -

Re: [PATCH v3 net-next 1/1] net_sched: Introduce skbmod action

On 16-09-06 10:25 AM, Eric Dumazet wrote: Just use u16 in the array ? u16 tmpaddr[ETH_ALEN / 2]; /* ether_addr_copy() requirement */ ether_addr_copy((u8 *)tmpaddr, eth_hdr(skb)->h_dest); ... Ok - thanks Eric. BTW: Nobody has answered my other question: I can get stats to increment with:

Re: [PATCH v3 net-next 1/1] net_sched: Introduce skbmod action

On Tue, 2016-09-06 at 08:08 -0400, Jamal Hadi Salim wrote: > ng > On 16-08-30 08:44 AM, Eric Dumazet wrote: > > On Tue, 2016-08-30 at 07:57 -0400, Jamal Hadi Salim wrote: > >> if (flags & SKBMOD_F_SWAPMAC) { > >> u8 tmpaddr[ETH_ALEN]; > >> /*XXX: I am sure

Re: [PATCH v3 net-next 1/1] net_sched: Introduce skbmod action

ng On 16-08-30 08:44 AM, Eric Dumazet wrote: On Tue, 2016-08-30 at 07:57 -0400, Jamal Hadi Salim wrote: if (flags & SKBMOD_F_SWAPMAC) { u8 tmpaddr[ETH_ALEN]; /*XXX: I am sure we can come up with something more efficient */

Re: [PATCH v3 net-next 1/1] net_sched: Introduce skbmod action

On 16-08-30 08:35 AM, Eric Dumazet wrote: synchronize_rcu() might bee to expensive if you plan to change actions hundred of times per second. You could instead add a 'struct rcu_head rcu;' field in struct tcf_skbmod_params (but make sure this is not exported to user space) Then :

Re: [PATCH v3 net-next 1/1] net_sched: Introduce skbmod action

On Tue, 2016-08-30 at 07:57 -0400, Jamal Hadi Salim wrote: > if (flags & SKBMOD_F_SWAPMAC) { > u8 tmpaddr[ETH_ALEN]; > /*XXX: I am sure we can come up with something more efficient > */ > ether_addr_copy(tmpaddr, eth_hdr(skb)->h_dest); >

Re: [PATCH v3 net-next 1/1] net_sched: Introduce skbmod action

On Tue, 2016-08-30 at 07:57 -0400, Jamal Hadi Salim wrote: > rcu_assign_pointer(d->skbmod_p, p); > if (ovr) { > spin_unlock_bh(>tcf_lock); > synchronize_rcu(); > } > > kfree(p_old); Overall patch looks good Jamal, thanks.

Re: [PATCH v3 net-next 1/1] net_sched: Introduce skbmod action

On Tue, 2016-08-30 at 07:12 -0400, Jamal Hadi Salim wrote: > On 16-08-29 02:20 PM, Eric Dumazet wrote: > > > > > You would need to store everything in an object, managed by rcu. > > > > struct my_rcu_safe_struct { > > u32 flags; > > u8 eth_dst[ETH_ALEN]; > > u8

Re: [PATCH v3 net-next 1/1] net_sched: Introduce skbmod action

On 16-08-30 07:12 AM, Jamal Hadi Salim wrote: Thanks Eric. This is the approach i thought Cong was going to take but in a way that it applies to all actions. It requires I do this extra allocation per update/create - I am not sure how much of a big deal that is (we take pride in our update

Re: [PATCH v3 net-next 1/1] net_sched: Introduce skbmod action

On 16-08-29 02:20 PM, Eric Dumazet wrote: You would need to store everything in an object, managed by rcu. struct my_rcu_safe_struct { u32 flags; u8 eth_dst[ETH_ALEN]; u8 eth_src[ETH_ALEN]; __be16 eth_type; }; And then allocate a new one when you need to update the

Re: [PATCH v3 net-next 1/1] net_sched: Introduce skbmod action

On Mon, 2016-08-29 at 07:35 -0400, Jamal Hadi Salim wrote: >flags = READ_ONCE(d->flags); > rcu_read_lock(); > if (flags & SKBMOD_F_DMAC) > ether_addr_copy(eth_hdr(skb)->h_dest, d->eth_dst); > if (flags & SKBMOD_F_SMAC) >

Re: [PATCH v3 net-next 1/1] net_sched: Introduce skbmod action

On Sun, 2016-08-28 at 22:10 -0700, Cong Wang wrote: > On Sun, Aug 28, 2016 at 9:07 AM, Eric Dumazet wrote: > > > > Adding an action with a spinlock held in fast path in 2016 is > > a way to tell people : It is a toy, do not use it for real. > > > > Sorry guys. Friends do

Re: [PATCH v3 net-next 1/1] net_sched: Introduce skbmod action

On 16-08-29 07:40 AM, Jamal Hadi Salim wrote: On 16-08-29 07:00 AM, Daniel Borkmann wrote: Sorry missed that. Let me give it an attempt. I think challenge is going to be what length to use. For now it is ethernet; but i had a change which swapped VLANs that i took out. something like this?

Re: [PATCH v3 net-next 1/1] net_sched: Introduce skbmod action

On 16-08-29 06:38 AM, Jamal Hadi Salim wrote: Let me see if i can try something. Trickery with RCU is not one of my virtues - so i may get it wrong but hope you can comment. Something like attached? Compile tested. I tried to emulate gact - but greping around I havent seen sample code which

Re: [PATCH v3 net-next 1/1] net_sched: Introduce skbmod action

On 16-08-29 07:00 AM, Daniel Borkmann wrote: You could probably just keep these meta data in a separate struct managed by RCU that tcf_skbmod points to. Which seem like could be made generic for all actions. Maybe thats what Cong is thinking. One more thing I commented on your last patch

Re: [PATCH v3 net-next 1/1] net_sched: Introduce skbmod action

On 08/29/2016 12:38 PM, Jamal Hadi Salim wrote: On 16-08-28 12:07 PM, Eric Dumazet wrote: Adding an action with a spinlock held in fast path in 2016 is a way to tell people : It is a toy, do not use it for real. Sorry guys. Friends do not let friends do that anymore. Generally agree. Cong

Re: [PATCH v3 net-next 1/1] net_sched: Introduce skbmod action

On 16-08-28 12:07 PM, Eric Dumazet wrote: Adding an action with a spinlock held in fast path in 2016 is a way to tell people : It is a toy, do not use it for real. Sorry guys. Friends do not let friends do that anymore. Generally agree. Cong says he is working on generic patches. Let me see

Re: [PATCH v3 net-next 1/1] net_sched: Introduce skbmod action

On 16-08-28 10:48 AM, Alexei Starovoitov wrote: On Sun, Aug 28, 2016 at 08:19:16AM -0400, Jamal Hadi Salim wrote: the address is incorrect. That's why it's recommended to skip this section in all headers. First paragraph in the above 'This program is ... GPL' would have been enough. I am

Re: [PATCH v3 net-next 1/1] net_sched: Introduce skbmod action

On Sun, Aug 28, 2016 at 9:07 AM, Eric Dumazet wrote: > > Adding an action with a spinlock held in fast path in 2016 is > a way to tell people : It is a toy, do not use it for real. > > Sorry guys. Friends do not let friends do that anymore. > Please stop joking, this is

Re: [PATCH v3 net-next 1/1] net_sched: Introduce skbmod action

On Sun, 2016-08-28 at 08:19 -0400, Jamal Hadi Salim wrote: > From: Jamal Hadi Salim ... > +static int tcf_skbmod_run(struct sk_buff *skb, const struct tc_action *a, > + struct tcf_result *res) > +{ > + struct tcf_skbmod *d = to_skbmod(a); > + > +

Re: [PATCH v3 net-next 1/1] net_sched: Introduce skbmod action

On Sun, Aug 28, 2016 at 08:19:16AM -0400, Jamal Hadi Salim wrote: > --- /dev/null > +++ b/include/uapi/linux/tc_act/tc_skbmod.h > @@ -0,0 +1,49 @@ > +/* > + * Copyright (c) 2016, Jamal Hadi Salim > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the