| Message ID | 20190329204438.68777-1-ldir@darbyshire-bryant.me.uk |
|---|---|
| Headers | show |
| Series | net: sched: Introduce conntrack action | expand |
Hi, On Fri, Mar 29, 2019 at 08:45:06PM +0000, Kevin 'ldir' Darbyshire-Bryant wrote: > Hi Cong, > > OK, so I've renamed conndscp to conntrack and hopefully this are > flexible enough for future conntrack->skb operations to be added in the > future. How does this one fly? This work sort of clashes with the work that Paul Blakey and I are doing to integrate conntrack with tc and vice-versa. Considering that in this patch the action is not RCU-ified, that it is using a struct as netlink parameter and it is dealing only with the dscp info, seems it's easier if we/you extend our code to support this feature as well. How does that sound to you? The RFC I had posted is VERY outdated (message-id cover.1548285996.git.mleitner@redhat.com), please don't use it as reference. Not sure if Paul can post a refreshed RFC already. Thanks, Marcelo
> On 1 Apr 2019, at 14:14, Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> wrote: > > Hi, > > On Fri, Mar 29, 2019 at 08:45:06PM +0000, Kevin 'ldir' Darbyshire-Bryant wrote: >> Hi Cong, >> >> OK, so I've renamed conndscp to conntrack and hopefully this are >> flexible enough for future conntrack->skb operations to be added in the >> future. How does this one fly? > > This work sort of clashes with the work that Paul Blakey and I are > doing to integrate conntrack with tc and vice-versa. > > Considering that in this patch the action is not RCU-ified, that it is > using a struct as netlink parameter and it is dealing only with the > dscp info, seems it's easier if we/you extend our code to support this > feature as well. How does that sound to you? > > The RFC I had posted is VERY outdated (message-id > cover.1548285996.git.mleitner@redhat.com), please don't use it as > reference. Not sure if Paul can post a refreshed RFC already. > > Thanks, > Marcelo I think the reality is that I’m way out of my depth here. The idea was to have something so simple that I could write(copy - see act_connmark) it/use it for my use case. I looked at the email you suggested and have not a clue! Sorry. Maybe someone can see the idea and run with it. Kevin
> > >> On 1 Apr 2019, at 14:14, Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> wrote: >> >> Hi, >> >> On Fri, Mar 29, 2019 at 08:45:06PM +0000, Kevin 'ldir' Darbyshire-Bryant wrote: >>> Hi Cong, >>> >>> OK, so I've renamed conndscp to conntrack and hopefully this are >>> flexible enough for future conntrack->skb operations to be added in the >>> future. How does this one fly? >> >> This work sort of clashes with the work that Paul Blakey and I are >> doing to integrate conntrack with tc and vice-versa. >> >> Considering that in this patch the action is not RCU-ified, that it is >> using a struct as netlink parameter and it is dealing only with the >> dscp info, seems it's easier if we/you extend our code to support this >> feature as well. How does that sound to you? >> >> The RFC I had posted is VERY outdated (message-id >> cover.1548285996.git.mleitner@redhat.com), please don't use it as >> reference. Not sure if Paul can post a refreshed RFC already. >> >> Thanks, >> Marcelo > > I think the reality is that I’m way out of my depth here. The idea was to have something so simple that I could write(copy - see act_connmark) it/use it for my use case. I looked at the email you suggested and have not a clue! Sorry. > > Maybe someone can see the idea and run with it. > > Kevin > > Hi, We are working on act_ct (basically a act_conntrack) which will be an action to send packets to conntrack for connection tracking. This two modes of operation are so different I don't think they need merging. This would probably be better off with the previous name act_conndscp. Thanks, Paul.
On Mon, Apr 1, 2019 at 7:22 AM Paul Blakey <paulb@mellanox.com> wrote: > > > > > > > > >> On 1 Apr 2019, at 14:14, Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> wrote: > >> > >> Hi, > >> > >> On Fri, Mar 29, 2019 at 08:45:06PM +0000, Kevin 'ldir' Darbyshire-Bryant wrote: > >>> Hi Cong, > >>> > >>> OK, so I've renamed conndscp to conntrack and hopefully this are > >>> flexible enough for future conntrack->skb operations to be added in the > >>> future. How does this one fly? > >> > >> This work sort of clashes with the work that Paul Blakey and I are > >> doing to integrate conntrack with tc and vice-versa. > >> > >> Considering that in this patch the action is not RCU-ified, that it is > >> using a struct as netlink parameter and it is dealing only with the > >> dscp info, seems it's easier if we/you extend our code to support this > >> feature as well. How does that sound to you? > >> > >> The RFC I had posted is VERY outdated (message-id > >> cover.1548285996.git.mleitner@redhat.com), please don't use it as > >> reference. Not sure if Paul can post a refreshed RFC already. > >> > >> Thanks, > >> Marcelo > > > > I think the reality is that I’m way out of my depth here. The idea was to have something so simple that I could write(copy - see act_connmark) it/use it for my use case. I looked at the email you suggested and have not a clue! Sorry. > > > > Maybe someone can see the idea and run with it. > > > > Kevin > > > > > > Hi, > > We are working on act_ct (basically a act_conntrack) which will be an > action to send packets to conntrack for connection tracking. This two > modes of operation are so different I don't think they need merging. What do you mean by "send packets to conntrack"? It is kinda confusing as TC is L2 and conntrack is L3, you want to re-inject the packets to L3?? > > This would probably be better off with the previous name act_conndscp. If naming is the only concern here, then it is not hard to find a name we are all satisfied with, like act_ctinfo (if we still want more than just DSCP). Thanks.
Hi Cong & all, > On 1 Apr 2019, at 22:06, Cong Wang <xiyou.wangcong@gmail.com> wrote: > > On Mon, Apr 1, 2019 at 7:22 AM Paul Blakey <paulb@mellanox.com> wrote: >> > <snip some context> > >> >> This would probably be better off with the previous name act_conndscp. > > > If naming is the only concern here, then it is not hard to find > a name we are all satisfied with, like act_ctinfo (if we still want more > than just DSCP). Personally I don’t *need* anything more than restoring the DSCP from conntrack mark, however my own narrow viewpoint doesn’t preclude some future desire to restore something else. For that reason I changed the name from act_conndscp to act_conntrack and hope that the latest suggested name change ‘act_ctinfo’ be the last. Incidentally I like the name ‘act_ctinfo’ especially if it is intended there be a ’send to conntrack’ type action of act_ct/act_conntrack. Moving on from naming the darn thing and assuming ‘act_ctinfo’ sticks, is there anything fundamentally wrong with the code? Nitpicks (or bigpicks) in approach or style? I’m naively hoping the first real submission results in “That’s wonderful code, we’d be mad not to accept it” instead of “you’ve done that wrong, and that, and that, and that….” :-) > > Thanks. Cheers, Kevin D-B gpg: 012C ACB2 28C6 C53E 9775 9123 B3A2 389B 9DE2 334A
Kevin 'ldir' Darbyshire-Bryant wrote on 02/04/2019 12:24: > Hi Cong & all, > >> On 1 Apr 2019, at 22:06, Cong Wang <xiyou.wangcong@gmail.com> wrote: >> >> On Mon, Apr 1, 2019 at 7:22 AM Paul Blakey <paulb@mellanox.com> wrote: >>> >> > <snip some context> >> >>> >>> This would probably be better off with the previous name act_conndscp. >> >> >> If naming is the only concern here, then it is not hard to find >> a name we are all satisfied with, like act_ctinfo (if we still want more >> than just DSCP). > > Personally I don’t *need* anything more than restoring the DSCP from conntrack mark, however my own narrow viewpoint doesn’t preclude some future desire to restore something else. For that reason I changed the name from act_conndscp to act_conntrack and hope that the latest suggested name change ‘act_ctinfo’ be the last. Incidentally I like the name ‘act_ctinfo’ especially if it is intended there be a ’send to conntrack’ type action of act_ct/act_conntrack. > > Moving on from naming the darn thing and assuming ‘act_ctinfo’ sticks, is there anything fundamentally wrong with the code? Nitpicks (or bigpicks) in approach or style? I’m naively hoping the first real submission results in “That’s wonderful code, we’d be mad not to accept it” instead of “you’ve done that wrong, and that, and that, and that….” :-) > > >> >> Thanks. > > > Cheers, > > Kevin D-B > > gpg: 012C ACB2 28C6 C53E 9775 9123 B3A2 389B 9DE2 334A > Hi Kevin, If you'd like, You can rebase it on our upcoming act_ct (our act_conntrack) once we're done (hopefully in a couple of weeks). If you going with the a standalone action, it's fine with me as well. > + /* let's not trust userspace entirely */ > + /* need at least contiguous 6 bit mask */ > + if ((0x3f & (ci->mask >> ci->maskshift)) != 0x3f) > + ci->mode &= ~CONNTRACK_FLAG_SETDSCP; > + /* mask & statemask must not overlap */ > + if (ci->mask & ci->statemask) > + ci->mode &= ~CONNTRACK_FLAG_SETDSCP; > + Some nitpicks, you check if the user provides sane input in conntrack_parmset, but instead of returning an error, you just disable the only action the user provided and the module supports, so it won't do nothing, yet the command succeeds. As marcelo said, this module isn't RCUified... see the design pattern in act_vlan, act_tunnel_key, act_csum, or what this commits changed: commit 4c5b9d9642c859f7369338fc42c0f62f4151bef3 Author: Manish Kurup <kurup.manish@gmail.com> act_vlan: VLAN action rewrite to use RCU lock/unlock and update commit 9c5f69bbd75a7db80578782b037629c5f1e59dce Author: Davide Caratti <dcaratti@redhat.com> net/sched: act_csum: don't use spinlock in the fast path And regarding the > + ct = nf_ct_get(skb, &ctinfo); > + if (!ct) { /* look harder */ The packet didn't pass connection tracking yet right (!ct) but you're setting the DSCP early? > + if (!nf_ct_get_tuplepr(skb, skb_network_offset(skb), > + proto, ca->net, &tuple)) > + goto out; > + zone.id = ca->zone; > + zone.dir = NF_CT_DEFAULT_ZONE_DIR; > + > + thash = nf_conntrack_find_get(ca->net, &zone, &tuple); > + if (!thash) > + goto out; > + > + if (!nf_ct_get_tuplepr(skb, skb_network_offset(skb), > + proto, ca->net, &tuple)) > + goto out; > + Aren't searching for the same tuple twice? Thanks, Paul.
> On 3 Apr 2019, at 08:47, Paul Blakey <paulb@mellanox.com> wrote: > > > > Kevin 'ldir' Darbyshire-Bryant wrote on 02/04/2019 12:24: >> Hi Cong & all, >> >>> On 1 Apr 2019, at 22:06, Cong Wang <xiyou.wangcong@gmail.com> wrote: >>> >>> On Mon, Apr 1, 2019 at 7:22 AM Paul Blakey <paulb@mellanox.com> wrote: >>>> >>> >> <snip some context> > > > Hi Kevin, > If you'd like, You can rebase it on our upcoming act_ct (our > act_conntrack) once we're done (hopefully in a couple of weeks). If you > going with the a standalone action, it's fine with me as well. > > >> + /* let's not trust userspace entirely */ >> + /* need at least contiguous 6 bit mask */ >> + if ((0x3f & (ci->mask >> ci->maskshift)) != 0x3f) >> + ci->mode &= ~CONNTRACK_FLAG_SETDSCP; >> + /* mask & statemask must not overlap */ >> + if (ci->mask & ci->statemask) >> + ci->mode &= ~CONNTRACK_FLAG_SETDSCP; >> + > > Some nitpicks, you check if the user provides sane input in > conntrack_parmset, but instead of returning an error, you just disable > the only action the user provided and the module supports, so it won't > do nothing, yet the command succeeds. Ok, I’ll return an -E something. I guess I’m still stuck between this generic ‘act_ctinfo’ potentially does more than one thing vs ‘act_conndscp’ doing a single thing. > > As marcelo said, this module isn't RCUified... see the design pattern in > act_vlan, act_tunnel_key, act_csum, or what this commits changed: > > commit 4c5b9d9642c859f7369338fc42c0f62f4151bef3 > Author: Manish Kurup <kurup.manish@gmail.com> > act_vlan: VLAN action rewrite to use RCU lock/unlock and update > > commit 9c5f69bbd75a7db80578782b037629c5f1e59dce > Author: Davide Caratti <dcaratti@redhat.com> > net/sched: act_csum: don't use spinlock in the fast path Ok, will take a look. Note current act_connmark on which this is a shameless copy has the same problem. Is that an oversight and also needs fixing? > > > And regarding the >> + ct = nf_ct_get(skb, &ctinfo); >> + if (!ct) { /* look harder */ > > > The packet didn't pass connection tracking yet right (!ct) but you're > setting the DSCP early? > > >> + if (!nf_ct_get_tuplepr(skb, skb_network_offset(skb), >> + proto, ca->net, &tuple)) >> + goto out; >> + zone.id = ca->zone; >> + zone.dir = NF_CT_DEFAULT_ZONE_DIR; >> + >> + thash = nf_conntrack_find_get(ca->net, &zone, &tuple); >> + if (!thash) >> + goto out; >> + >> + if (!nf_ct_get_tuplepr(skb, skb_network_offset(skb), >> + proto, ca->net, &tuple)) >> + goto out; >> + > > Aren't searching for the same tuple twice? Again, I’m not doing anything that act_connmark (conntrack mark to skb mark setting) isn’t doing already, so is act_connmark also wrong? As you can almost certainly tell I’m working in areas that I don’t understand, I only (think I) know the result I wish to achieve and so far it is working. More luck than judgement?! :-) > > Thanks, > Paul.
Kevin 'ldir' Darbyshire-Bryant wrote on 03/04/2019 11:23: > > >> On 3 Apr 2019, at 08:47, Paul Blakey <paulb@mellanox.com> wrote: >> >> >> >> Kevin 'ldir' Darbyshire-Bryant wrote on 02/04/2019 12:24: >>> Hi Cong & all, >>> >>>> On 1 Apr 2019, at 22:06, Cong Wang <xiyou.wangcong@gmail.com> wrote: >>>> >>>> On Mon, Apr 1, 2019 at 7:22 AM Paul Blakey <paulb@mellanox.com> wrote: >>>>> >>>> >>> <snip some context> >> >> >> Hi Kevin, >> If you'd like, You can rebase it on our upcoming act_ct (our >> act_conntrack) once we're done (hopefully in a couple of weeks). If you >> going with the a standalone action, it's fine with me as well. >> >> >>> + /* let's not trust userspace entirely */ >>> + /* need at least contiguous 6 bit mask */ >>> + if ((0x3f & (ci->mask >> ci->maskshift)) != 0x3f) >>> + ci->mode &= ~CONNTRACK_FLAG_SETDSCP; >>> + /* mask & statemask must not overlap */ >>> + if (ci->mask & ci->statemask) >>> + ci->mode &= ~CONNTRACK_FLAG_SETDSCP; >>> + >> >> Some nitpicks, you check if the user provides sane input in >> conntrack_parmset, but instead of returning an error, you just disable >> the only action the user provided and the module supports, so it won't >> do nothing, yet the command succeeds. > > Ok, I’ll return an -E something. I guess I’m still stuck between this > generic ‘act_ctinfo’ potentially does more than one thing vs ‘act_conndscp’ > doing a single thing. > >> >> As marcelo said, this module isn't RCUified... see the design pattern in >> act_vlan, act_tunnel_key, act_csum, or what this commits changed: >> >> commit 4c5b9d9642c859f7369338fc42c0f62f4151bef3 >> Author: Manish Kurup <kurup.manish@gmail.com> >> act_vlan: VLAN action rewrite to use RCU lock/unlock and update >> >> commit 9c5f69bbd75a7db80578782b037629c5f1e59dce >> Author: Davide Caratti <dcaratti@redhat.com> >> net/sched: act_csum: don't use spinlock in the fast path > > Ok, will take a look. Note current act_connmark on which this is a > shameless copy has the same problem. Is that an oversight and also > needs fixing? I think it's a performance consideration (and not an error) for now, and there were yet to be updated. > >> >> >> And regarding the >>> + ct = nf_ct_get(skb, &ctinfo); >>> + if (!ct) { /* look harder */ >> >> >> The packet didn't pass connection tracking yet right (!ct) but you're >> setting the DSCP early? >> >> >>> + if (!nf_ct_get_tuplepr(skb, skb_network_offset(skb), >>> + proto, ca->net, &tuple)) >>> + goto out; >>> + zone.id = ca->zone; >>> + zone.dir = NF_CT_DEFAULT_ZONE_DIR; >>> + >>> + thash = nf_conntrack_find_get(ca->net, &zone, &tuple); >>> + if (!thash) >>> + goto out; >>> + >>> + if (!nf_ct_get_tuplepr(skb, skb_network_offset(skb), >>> + proto, ca->net, &tuple)) >>> + goto out; >>> + >> >> Aren't searching for the same tuple twice? > > Again, I’m not doing anything that act_connmark (conntrack mark to > skb mark setting) isn’t doing already, so is act_connmark also wrong? > As you can almost certainly tell I’m working in areas that I don’t > understand, I only (think I) know the result I wish to achieve and so far > it is working. More luck than judgement?! :-) > if I recall correctly, act_connmark doesn't call nf_ct_get_tuplepr twice. >> >> Thanks, >> Paul. >
> On 3 Apr 2019, at 12:56, Paul Blakey <paulb@mellanox.com> wrote: > > > > Kevin 'ldir' Darbyshire-Bryant wrote on 03/04/2019 11:23: >> >> >>> On 3 Apr 2019, at 08:47, Paul Blakey <paulb@mellanox.com> wrote: >>> >>> >>> >>> Kevin 'ldir' Darbyshire-Bryant wrote on 02/04/2019 12:24: >>>> Hi Cong & all, >>>> >>>>> On 1 Apr 2019, at 22:06, Cong Wang <xiyou.wangcong@gmail.com> wrote: >>>>> >>>>> On Mon, Apr 1, 2019 at 7:22 AM Paul Blakey <paulb@mellanox.com> wrote: >>>>>> >>>>> >>>> <snip some context> >>> >>> >>> Hi Kevin, >>> If you'd like, You can rebase it on our upcoming act_ct (our >>> act_conntrack) once we're done (hopefully in a couple of weeks). If you >>> going with the a standalone action, it's fine with me as well. >>> >>> >>>> + /* let's not trust userspace entirely */ >>>> + /* need at least contiguous 6 bit mask */ >>>> + if ((0x3f & (ci->mask >> ci->maskshift)) != 0x3f) >>>> + ci->mode &= ~CONNTRACK_FLAG_SETDSCP; >>>> + /* mask & statemask must not overlap */ >>>> + if (ci->mask & ci->statemask) >>>> + ci->mode &= ~CONNTRACK_FLAG_SETDSCP; >>>> + >>> >>> Some nitpicks, you check if the user provides sane input in >>> conntrack_parmset, but instead of returning an error, you just disable >>> the only action the user provided and the module supports, so it won't >>> do nothing, yet the command succeeds. >> >> Ok, I’ll return an -E something. I guess I’m still stuck between this >> generic ‘act_ctinfo’ potentially does more than one thing vs ‘act_conndscp’ >> doing a single thing. >> >>> >>> As marcelo said, this module isn't RCUified... see the design pattern in >>> act_vlan, act_tunnel_key, act_csum, or what this commits changed: >>> >>> commit 4c5b9d9642c859f7369338fc42c0f62f4151bef3 >>> Author: Manish Kurup <kurup.manish@gmail.com> >>> act_vlan: VLAN action rewrite to use RCU lock/unlock and update >>> >>> commit 9c5f69bbd75a7db80578782b037629c5f1e59dce >>> Author: Davide Caratti <dcaratti@redhat.com> >>> net/sched: act_csum: don't use spinlock in the fast path >> >> Ok, will take a look. Note current act_connmark on which this is a >> shameless copy has the same problem. Is that an oversight and also >> needs fixing? > > I think it's a performance consideration (and not an error) for now, and > there were yet to be updated. > >> >>> >>> >>> And regarding the >>>> + ct = nf_ct_get(skb, &ctinfo); >>>> + if (!ct) { /* look harder */ >>> >>> >>> The packet didn't pass connection tracking yet right (!ct) but you're >>> setting the DSCP early? >>> >>> >>>> + if (!nf_ct_get_tuplepr(skb, skb_network_offset(skb), >>>> + proto, ca->net, &tuple)) >>>> + goto out; >>>> + zone.id = ca->zone; >>>> + zone.dir = NF_CT_DEFAULT_ZONE_DIR; >>>> + >>>> + thash = nf_conntrack_find_get(ca->net, &zone, &tuple); >>>> + if (!thash) >>>> + goto out; >>>> + >>>> + if (!nf_ct_get_tuplepr(skb, skb_network_offset(skb), >>>> + proto, ca->net, &tuple)) >>>> + goto out; >>>> + >>> >>> Aren't searching for the same tuple twice? >> >> Again, I’m not doing anything that act_connmark (conntrack mark to >> skb mark setting) isn’t doing already, so is act_connmark also wrong? >> As you can almost certainly tell I’m working in areas that I don’t >> understand, I only (think I) know the result I wish to achieve and so far >> it is working. More luck than judgement?! :-) >> > > if I recall correctly, act_connmark doesn't call nf_ct_get_tuplepr twice. > Whoops! Hangs head in shame - copy/paste merge error at some point - will fix… along with all the other stuff. > >>> >>> Thanks, >>> Paul. Cheers, Kevin D-B