| Message ID | 20230113165548.2692720-7-vladbu@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Allow offloading of UDP NEW connections via act_ct | expand |
On Fri, Jan 13, 2023 at 05:55:47PM +0100, Vlad Buslov wrote: > When processing connections allow offloading of UDP connections that don't > have IPS_ASSURED_BIT set as unidirectional. When performing table lookup Hmm. Considering that this is now offloading one direction only already, what about skipping this grace period: In nf_conntrack_udp_packet(), it does: /* If we've seen traffic both ways, this is some kind of UDP * stream. Set Assured. */ if (test_bit(IPS_SEEN_REPLY_BIT, &ct->status)) { ... /* Still active after two seconds? Extend timeout. */ if (time_after(jiffies, ct->proto.udp.stream_ts)) { extra = timeouts[UDP_CT_REPLIED]; stream = true; } ... /* Also, more likely to be important, and not a probe */ if (stream && !test_and_set_bit(IPS_ASSURED_BIT, &ct->status)) nf_conntrack_event_cache(IPCT_ASSURED, ct); Maybe the patch should be relying on IPS_SEEN_REPLY_BIT instead of ASSURED for UDP? Just a thought here, but I'm not seeing why not. > for reply packets check the current connection status: If UDP > unidirectional connection became assured also promote the corresponding > flow table entry to bidirectional and set the 'update' bit, else just set > the 'update' bit since reply directional traffic will most likely cause > connection status to become 'established' which requires updating the > offload state. > > Signed-off-by: Vlad Buslov <vladbu@nvidia.com> > --- > net/sched/act_ct.c | 48 ++++++++++++++++++++++++++++++++++------------ > 1 file changed, 36 insertions(+), 12 deletions(-) > > diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c > index bfddb462d2bc..563cbdd8341c 100644 > --- a/net/sched/act_ct.c > +++ b/net/sched/act_ct.c > @@ -369,7 +369,7 @@ static void tcf_ct_flow_tc_ifidx(struct flow_offload *entry, > > static void tcf_ct_flow_table_add(struct tcf_ct_flow_table *ct_ft, > struct nf_conn *ct, > - bool tcp) > + bool tcp, bool bidirectional) > { > struct nf_conn_act_ct_ext *act_ct_ext; > struct flow_offload *entry; > @@ -388,6 +388,8 @@ static void tcf_ct_flow_table_add(struct tcf_ct_flow_table *ct_ft, > ct->proto.tcp.seen[0].flags |= IP_CT_TCP_FLAG_BE_LIBERAL; > ct->proto.tcp.seen[1].flags |= IP_CT_TCP_FLAG_BE_LIBERAL; > } > + if (bidirectional) > + __set_bit(NF_FLOW_HW_BIDIRECTIONAL, &entry->flags); > > act_ct_ext = nf_conn_act_ct_ext_find(ct); > if (act_ct_ext) { > @@ -411,26 +413,34 @@ static void tcf_ct_flow_table_process_conn(struct tcf_ct_flow_table *ct_ft, > struct nf_conn *ct, > enum ip_conntrack_info ctinfo) > { > - bool tcp = false; > - > - if ((ctinfo != IP_CT_ESTABLISHED && ctinfo != IP_CT_ESTABLISHED_REPLY) || > - !test_bit(IPS_ASSURED_BIT, &ct->status)) > - return; > + bool tcp = false, bidirectional = true; > > switch (nf_ct_protonum(ct)) { > case IPPROTO_TCP: > - tcp = true; > - if (ct->proto.tcp.state != TCP_CONNTRACK_ESTABLISHED) > + if ((ctinfo != IP_CT_ESTABLISHED && > + ctinfo != IP_CT_ESTABLISHED_REPLY) || > + !test_bit(IPS_ASSURED_BIT, &ct->status) || > + ct->proto.tcp.state != TCP_CONNTRACK_ESTABLISHED) > return; > + > + tcp = true; > break; > case IPPROTO_UDP: > + if (!nf_ct_is_confirmed(ct)) > + return; > + if (!test_bit(IPS_ASSURED_BIT, &ct->status)) > + bidirectional = false; > break; > #ifdef CONFIG_NF_CT_PROTO_GRE > case IPPROTO_GRE: { > struct nf_conntrack_tuple *tuple; > > - if (ct->status & IPS_NAT_MASK) > + if ((ctinfo != IP_CT_ESTABLISHED && > + ctinfo != IP_CT_ESTABLISHED_REPLY) || > + !test_bit(IPS_ASSURED_BIT, &ct->status) || > + ct->status & IPS_NAT_MASK) > return; > + > tuple = &ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple; > /* No support for GRE v1 */ > if (tuple->src.u.gre.key || tuple->dst.u.gre.key) > @@ -446,7 +456,7 @@ static void tcf_ct_flow_table_process_conn(struct tcf_ct_flow_table *ct_ft, > ct->status & IPS_SEQ_ADJUST) > return; > > - tcf_ct_flow_table_add(ct_ft, ct, tcp); > + tcf_ct_flow_table_add(ct_ft, ct, tcp, bidirectional); > } > > static bool > @@ -625,13 +635,27 @@ static bool tcf_ct_flow_table_lookup(struct tcf_ct_params *p, > flow = container_of(tuplehash, struct flow_offload, tuplehash[dir]); > ct = flow->ct; > > + if (dir == FLOW_OFFLOAD_DIR_REPLY && > + !test_bit(NF_FLOW_HW_BIDIRECTIONAL, &flow->flags)) { > + /* Only offload reply direction after connection became > + * assured. > + */ > + if (test_bit(IPS_ASSURED_BIT, &ct->status)) > + set_bit(NF_FLOW_HW_BIDIRECTIONAL, &flow->flags); > + set_bit(NF_FLOW_HW_UPDATE, &flow->flags); > + return false; > + } > + > if (tcph && (unlikely(tcph->fin || tcph->rst))) { > flow_offload_teardown(flow); > return false; > } > > - ctinfo = dir == FLOW_OFFLOAD_DIR_ORIGINAL ? IP_CT_ESTABLISHED : > - IP_CT_ESTABLISHED_REPLY; > + if (dir == FLOW_OFFLOAD_DIR_ORIGINAL) > + ctinfo = test_bit(IPS_SEEN_REPLY_BIT, &ct->status) ? > + IP_CT_ESTABLISHED : IP_CT_NEW; > + else > + ctinfo = IP_CT_ESTABLISHED_REPLY; > > flow_offload_refresh(nf_ft, flow); > nf_conntrack_get(&ct->ct_general); > -- > 2.38.1 >
On Tue 17 Jan 2023 at 12:41, Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> wrote: > On Fri, Jan 13, 2023 at 05:55:47PM +0100, Vlad Buslov wrote: >> When processing connections allow offloading of UDP connections that don't >> have IPS_ASSURED_BIT set as unidirectional. When performing table lookup > > Hmm. Considering that this is now offloading one direction only > already, what about skipping this grace period: > > In nf_conntrack_udp_packet(), it does: > > /* If we've seen traffic both ways, this is some kind of UDP > * stream. Set Assured. > */ > if (test_bit(IPS_SEEN_REPLY_BIT, &ct->status)) { > ... > /* Still active after two seconds? Extend timeout. */ > if (time_after(jiffies, ct->proto.udp.stream_ts)) { > extra = timeouts[UDP_CT_REPLIED]; > stream = true; > } > ... > /* Also, more likely to be important, and not a probe */ > if (stream && !test_and_set_bit(IPS_ASSURED_BIT, &ct->status)) > nf_conntrack_event_cache(IPCT_ASSURED, ct); > > Maybe the patch should be relying on IPS_SEEN_REPLY_BIT instead of > ASSURED for UDP? Just a thought here, but I'm not seeing why not. The issue with this is that if we offload both directions early, then conntrack state machine will not receive any more packets and, consecutively, will never change the flow state to assured. I guess that could be mitigated somehow by periodically checking the hw stats and transitioning the flow to assured based on them, but as I said in previous email we don't want to over-complicate this series even more. Also, offloading to hardware isn't free and costs both memory and CPU, so it is not like offloading as early as possible is strictly beneficial for all cases... > >> for reply packets check the current connection status: If UDP >> unidirectional connection became assured also promote the corresponding >> flow table entry to bidirectional and set the 'update' bit, else just set >> the 'update' bit since reply directional traffic will most likely cause >> connection status to become 'established' which requires updating the >> offload state. >> >> Signed-off-by: Vlad Buslov <vladbu@nvidia.com> >> --- >> net/sched/act_ct.c | 48 ++++++++++++++++++++++++++++++++++------------ >> 1 file changed, 36 insertions(+), 12 deletions(-) >> >> diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c >> index bfddb462d2bc..563cbdd8341c 100644 >> --- a/net/sched/act_ct.c >> +++ b/net/sched/act_ct.c >> @@ -369,7 +369,7 @@ static void tcf_ct_flow_tc_ifidx(struct flow_offload *entry, >> >> static void tcf_ct_flow_table_add(struct tcf_ct_flow_table *ct_ft, >> struct nf_conn *ct, >> - bool tcp) >> + bool tcp, bool bidirectional) >> { >> struct nf_conn_act_ct_ext *act_ct_ext; >> struct flow_offload *entry; >> @@ -388,6 +388,8 @@ static void tcf_ct_flow_table_add(struct tcf_ct_flow_table *ct_ft, >> ct->proto.tcp.seen[0].flags |= IP_CT_TCP_FLAG_BE_LIBERAL; >> ct->proto.tcp.seen[1].flags |= IP_CT_TCP_FLAG_BE_LIBERAL; >> } >> + if (bidirectional) >> + __set_bit(NF_FLOW_HW_BIDIRECTIONAL, &entry->flags); >> >> act_ct_ext = nf_conn_act_ct_ext_find(ct); >> if (act_ct_ext) { >> @@ -411,26 +413,34 @@ static void tcf_ct_flow_table_process_conn(struct tcf_ct_flow_table *ct_ft, >> struct nf_conn *ct, >> enum ip_conntrack_info ctinfo) >> { >> - bool tcp = false; >> - >> - if ((ctinfo != IP_CT_ESTABLISHED && ctinfo != IP_CT_ESTABLISHED_REPLY) || >> - !test_bit(IPS_ASSURED_BIT, &ct->status)) >> - return; >> + bool tcp = false, bidirectional = true; >> >> switch (nf_ct_protonum(ct)) { >> case IPPROTO_TCP: >> - tcp = true; >> - if (ct->proto.tcp.state != TCP_CONNTRACK_ESTABLISHED) >> + if ((ctinfo != IP_CT_ESTABLISHED && >> + ctinfo != IP_CT_ESTABLISHED_REPLY) || >> + !test_bit(IPS_ASSURED_BIT, &ct->status) || >> + ct->proto.tcp.state != TCP_CONNTRACK_ESTABLISHED) >> return; >> + >> + tcp = true; >> break; >> case IPPROTO_UDP: >> + if (!nf_ct_is_confirmed(ct)) >> + return; >> + if (!test_bit(IPS_ASSURED_BIT, &ct->status)) >> + bidirectional = false; >> break; >> #ifdef CONFIG_NF_CT_PROTO_GRE >> case IPPROTO_GRE: { >> struct nf_conntrack_tuple *tuple; >> >> - if (ct->status & IPS_NAT_MASK) >> + if ((ctinfo != IP_CT_ESTABLISHED && >> + ctinfo != IP_CT_ESTABLISHED_REPLY) || >> + !test_bit(IPS_ASSURED_BIT, &ct->status) || >> + ct->status & IPS_NAT_MASK) >> return; >> + >> tuple = &ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple; >> /* No support for GRE v1 */ >> if (tuple->src.u.gre.key || tuple->dst.u.gre.key) >> @@ -446,7 +456,7 @@ static void tcf_ct_flow_table_process_conn(struct tcf_ct_flow_table *ct_ft, >> ct->status & IPS_SEQ_ADJUST) >> return; >> >> - tcf_ct_flow_table_add(ct_ft, ct, tcp); >> + tcf_ct_flow_table_add(ct_ft, ct, tcp, bidirectional); >> } >> >> static bool >> @@ -625,13 +635,27 @@ static bool tcf_ct_flow_table_lookup(struct tcf_ct_params *p, >> flow = container_of(tuplehash, struct flow_offload, tuplehash[dir]); >> ct = flow->ct; >> >> + if (dir == FLOW_OFFLOAD_DIR_REPLY && >> + !test_bit(NF_FLOW_HW_BIDIRECTIONAL, &flow->flags)) { >> + /* Only offload reply direction after connection became >> + * assured. >> + */ >> + if (test_bit(IPS_ASSURED_BIT, &ct->status)) >> + set_bit(NF_FLOW_HW_BIDIRECTIONAL, &flow->flags); >> + set_bit(NF_FLOW_HW_UPDATE, &flow->flags); >> + return false; >> + } >> + >> if (tcph && (unlikely(tcph->fin || tcph->rst))) { >> flow_offload_teardown(flow); >> return false; >> } >> >> - ctinfo = dir == FLOW_OFFLOAD_DIR_ORIGINAL ? IP_CT_ESTABLISHED : >> - IP_CT_ESTABLISHED_REPLY; >> + if (dir == FLOW_OFFLOAD_DIR_ORIGINAL) >> + ctinfo = test_bit(IPS_SEEN_REPLY_BIT, &ct->status) ? >> + IP_CT_ESTABLISHED : IP_CT_NEW; >> + else >> + ctinfo = IP_CT_ESTABLISHED_REPLY; >> >> flow_offload_refresh(nf_ft, flow); >> nf_conntrack_get(&ct->ct_general); >> -- >> 2.38.1 >>
On Tue, Jan 17, 2023 at 07:36:42PM +0200, Vlad Buslov wrote: > > On Tue 17 Jan 2023 at 12:41, Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> wrote: > > On Fri, Jan 13, 2023 at 05:55:47PM +0100, Vlad Buslov wrote: > >> When processing connections allow offloading of UDP connections that don't > >> have IPS_ASSURED_BIT set as unidirectional. When performing table lookup > > > > Hmm. Considering that this is now offloading one direction only > > already, what about skipping this grace period: > > > > In nf_conntrack_udp_packet(), it does: > > > > /* If we've seen traffic both ways, this is some kind of UDP > > * stream. Set Assured. > > */ > > if (test_bit(IPS_SEEN_REPLY_BIT, &ct->status)) { > > ... > > /* Still active after two seconds? Extend timeout. */ > > if (time_after(jiffies, ct->proto.udp.stream_ts)) { > > extra = timeouts[UDP_CT_REPLIED]; > > stream = true; > > } > > ... > > /* Also, more likely to be important, and not a probe */ > > if (stream && !test_and_set_bit(IPS_ASSURED_BIT, &ct->status)) > > nf_conntrack_event_cache(IPCT_ASSURED, ct); > > > > Maybe the patch should be relying on IPS_SEEN_REPLY_BIT instead of > > ASSURED for UDP? Just a thought here, but I'm not seeing why not. > > The issue with this is that if we offload both directions early, then > conntrack state machine will not receive any more packets and, > consecutively, will never change the flow state to assured. I guess that I'm missing how it would offload each direction independently. Wouldn't CT state machine see the 1st reply, because it is not offloaded yet, and match it to the original direction? > could be mitigated somehow by periodically checking the hw stats and > transitioning the flow to assured based on them, but as I said in > previous email we don't want to over-complicate this series even more. > Also, offloading to hardware isn't free and costs both memory and CPU, > so it is not like offloading as early as possible is strictly beneficial > for all cases... Yup. > > > > >> for reply packets check the current connection status: If UDP > >> unidirectional connection became assured also promote the corresponding > >> flow table entry to bidirectional and set the 'update' bit, else just set > >> the 'update' bit since reply directional traffic will most likely cause > >> connection status to become 'established' which requires updating the > >> offload state. > >> > >> Signed-off-by: Vlad Buslov <vladbu@nvidia.com> > >> --- > >> net/sched/act_ct.c | 48 ++++++++++++++++++++++++++++++++++------------ > >> 1 file changed, 36 insertions(+), 12 deletions(-) > >> > >> diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c > >> index bfddb462d2bc..563cbdd8341c 100644 > >> --- a/net/sched/act_ct.c > >> +++ b/net/sched/act_ct.c > >> @@ -369,7 +369,7 @@ static void tcf_ct_flow_tc_ifidx(struct flow_offload *entry, > >> > >> static void tcf_ct_flow_table_add(struct tcf_ct_flow_table *ct_ft, > >> struct nf_conn *ct, > >> - bool tcp) > >> + bool tcp, bool bidirectional) > >> { > >> struct nf_conn_act_ct_ext *act_ct_ext; > >> struct flow_offload *entry; > >> @@ -388,6 +388,8 @@ static void tcf_ct_flow_table_add(struct tcf_ct_flow_table *ct_ft, > >> ct->proto.tcp.seen[0].flags |= IP_CT_TCP_FLAG_BE_LIBERAL; > >> ct->proto.tcp.seen[1].flags |= IP_CT_TCP_FLAG_BE_LIBERAL; > >> } > >> + if (bidirectional) > >> + __set_bit(NF_FLOW_HW_BIDIRECTIONAL, &entry->flags); > >> > >> act_ct_ext = nf_conn_act_ct_ext_find(ct); > >> if (act_ct_ext) { > >> @@ -411,26 +413,34 @@ static void tcf_ct_flow_table_process_conn(struct tcf_ct_flow_table *ct_ft, > >> struct nf_conn *ct, > >> enum ip_conntrack_info ctinfo) > >> { > >> - bool tcp = false; > >> - > >> - if ((ctinfo != IP_CT_ESTABLISHED && ctinfo != IP_CT_ESTABLISHED_REPLY) || > >> - !test_bit(IPS_ASSURED_BIT, &ct->status)) > >> - return; > >> + bool tcp = false, bidirectional = true; > >> > >> switch (nf_ct_protonum(ct)) { > >> case IPPROTO_TCP: > >> - tcp = true; > >> - if (ct->proto.tcp.state != TCP_CONNTRACK_ESTABLISHED) > >> + if ((ctinfo != IP_CT_ESTABLISHED && > >> + ctinfo != IP_CT_ESTABLISHED_REPLY) || > >> + !test_bit(IPS_ASSURED_BIT, &ct->status) || > >> + ct->proto.tcp.state != TCP_CONNTRACK_ESTABLISHED) > >> return; > >> + > >> + tcp = true; > >> break; > >> case IPPROTO_UDP: > >> + if (!nf_ct_is_confirmed(ct)) > >> + return; > >> + if (!test_bit(IPS_ASSURED_BIT, &ct->status)) > >> + bidirectional = false; > >> break; > >> #ifdef CONFIG_NF_CT_PROTO_GRE > >> case IPPROTO_GRE: { > >> struct nf_conntrack_tuple *tuple; > >> > >> - if (ct->status & IPS_NAT_MASK) > >> + if ((ctinfo != IP_CT_ESTABLISHED && > >> + ctinfo != IP_CT_ESTABLISHED_REPLY) || > >> + !test_bit(IPS_ASSURED_BIT, &ct->status) || > >> + ct->status & IPS_NAT_MASK) > >> return; > >> + > >> tuple = &ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple; > >> /* No support for GRE v1 */ > >> if (tuple->src.u.gre.key || tuple->dst.u.gre.key) > >> @@ -446,7 +456,7 @@ static void tcf_ct_flow_table_process_conn(struct tcf_ct_flow_table *ct_ft, > >> ct->status & IPS_SEQ_ADJUST) > >> return; > >> > >> - tcf_ct_flow_table_add(ct_ft, ct, tcp); > >> + tcf_ct_flow_table_add(ct_ft, ct, tcp, bidirectional); > >> } > >> > >> static bool > >> @@ -625,13 +635,27 @@ static bool tcf_ct_flow_table_lookup(struct tcf_ct_params *p, > >> flow = container_of(tuplehash, struct flow_offload, tuplehash[dir]); > >> ct = flow->ct; > >> > >> + if (dir == FLOW_OFFLOAD_DIR_REPLY && > >> + !test_bit(NF_FLOW_HW_BIDIRECTIONAL, &flow->flags)) { > >> + /* Only offload reply direction after connection became > >> + * assured. > >> + */ > >> + if (test_bit(IPS_ASSURED_BIT, &ct->status)) > >> + set_bit(NF_FLOW_HW_BIDIRECTIONAL, &flow->flags); > >> + set_bit(NF_FLOW_HW_UPDATE, &flow->flags); > >> + return false; > >> + } > >> + > >> if (tcph && (unlikely(tcph->fin || tcph->rst))) { > >> flow_offload_teardown(flow); > >> return false; > >> } > >> > >> - ctinfo = dir == FLOW_OFFLOAD_DIR_ORIGINAL ? IP_CT_ESTABLISHED : > >> - IP_CT_ESTABLISHED_REPLY; > >> + if (dir == FLOW_OFFLOAD_DIR_ORIGINAL) > >> + ctinfo = test_bit(IPS_SEEN_REPLY_BIT, &ct->status) ? > >> + IP_CT_ESTABLISHED : IP_CT_NEW; > >> + else > >> + ctinfo = IP_CT_ESTABLISHED_REPLY; > >> > >> flow_offload_refresh(nf_ft, flow); > >> nf_conntrack_get(&ct->ct_general); > >> -- > >> 2.38.1 > >> >
On Tue 17 Jan 2023 at 14:56, Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> wrote: > On Tue, Jan 17, 2023 at 07:36:42PM +0200, Vlad Buslov wrote: >> >> On Tue 17 Jan 2023 at 12:41, Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> wrote: >> > On Fri, Jan 13, 2023 at 05:55:47PM +0100, Vlad Buslov wrote: >> >> When processing connections allow offloading of UDP connections that don't >> >> have IPS_ASSURED_BIT set as unidirectional. When performing table lookup >> > >> > Hmm. Considering that this is now offloading one direction only >> > already, what about skipping this grace period: >> > >> > In nf_conntrack_udp_packet(), it does: >> > >> > /* If we've seen traffic both ways, this is some kind of UDP >> > * stream. Set Assured. >> > */ >> > if (test_bit(IPS_SEEN_REPLY_BIT, &ct->status)) { >> > ... >> > /* Still active after two seconds? Extend timeout. */ >> > if (time_after(jiffies, ct->proto.udp.stream_ts)) { >> > extra = timeouts[UDP_CT_REPLIED]; >> > stream = true; >> > } >> > ... >> > /* Also, more likely to be important, and not a probe */ >> > if (stream && !test_and_set_bit(IPS_ASSURED_BIT, &ct->status)) >> > nf_conntrack_event_cache(IPCT_ASSURED, ct); >> > >> > Maybe the patch should be relying on IPS_SEEN_REPLY_BIT instead of >> > ASSURED for UDP? Just a thought here, but I'm not seeing why not. >> >> The issue with this is that if we offload both directions early, then >> conntrack state machine will not receive any more packets and, >> consecutively, will never change the flow state to assured. I guess that > > I'm missing how it would offload each direction independently. > Wouldn't CT state machine see the 1st reply, because it is not > offloaded yet, and match it to the original direction? What I meant is that if both directions are offloaded as soon as IPS_SEEN_REPLY_BIT is set, then nf_conntrack_udp_packet() will not be called for that connection anymore and would never be able to transition the connection to assured state. But main thing is, as I said in the previous reply, that we don't need to offload such connection ATM. It could be done in a follow-up if there is a use-case for it, maybe even made somehow configurable (with BPF! :)), so it could be dynamically controlled. > >> could be mitigated somehow by periodically checking the hw stats and >> transitioning the flow to assured based on them, but as I said in >> previous email we don't want to over-complicate this series even more. >> Also, offloading to hardware isn't free and costs both memory and CPU, >> so it is not like offloading as early as possible is strictly beneficial >> for all cases... > > Yup. > >> >> > >> >> for reply packets check the current connection status: If UDP >> >> unidirectional connection became assured also promote the corresponding >> >> flow table entry to bidirectional and set the 'update' bit, else just set >> >> the 'update' bit since reply directional traffic will most likely cause >> >> connection status to become 'established' which requires updating the >> >> offload state. >> >> >> >> Signed-off-by: Vlad Buslov <vladbu@nvidia.com> >> >> --- >> >> net/sched/act_ct.c | 48 ++++++++++++++++++++++++++++++++++------------ >> >> 1 file changed, 36 insertions(+), 12 deletions(-) >> >> >> >> diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c >> >> index bfddb462d2bc..563cbdd8341c 100644 >> >> --- a/net/sched/act_ct.c >> >> +++ b/net/sched/act_ct.c >> >> @@ -369,7 +369,7 @@ static void tcf_ct_flow_tc_ifidx(struct flow_offload *entry, >> >> >> >> static void tcf_ct_flow_table_add(struct tcf_ct_flow_table *ct_ft, >> >> struct nf_conn *ct, >> >> - bool tcp) >> >> + bool tcp, bool bidirectional) >> >> { >> >> struct nf_conn_act_ct_ext *act_ct_ext; >> >> struct flow_offload *entry; >> >> @@ -388,6 +388,8 @@ static void tcf_ct_flow_table_add(struct tcf_ct_flow_table *ct_ft, >> >> ct->proto.tcp.seen[0].flags |= IP_CT_TCP_FLAG_BE_LIBERAL; >> >> ct->proto.tcp.seen[1].flags |= IP_CT_TCP_FLAG_BE_LIBERAL; >> >> } >> >> + if (bidirectional) >> >> + __set_bit(NF_FLOW_HW_BIDIRECTIONAL, &entry->flags); >> >> >> >> act_ct_ext = nf_conn_act_ct_ext_find(ct); >> >> if (act_ct_ext) { >> >> @@ -411,26 +413,34 @@ static void tcf_ct_flow_table_process_conn(struct tcf_ct_flow_table *ct_ft, >> >> struct nf_conn *ct, >> >> enum ip_conntrack_info ctinfo) >> >> { >> >> - bool tcp = false; >> >> - >> >> - if ((ctinfo != IP_CT_ESTABLISHED && ctinfo != IP_CT_ESTABLISHED_REPLY) || >> >> - !test_bit(IPS_ASSURED_BIT, &ct->status)) >> >> - return; >> >> + bool tcp = false, bidirectional = true; >> >> >> >> switch (nf_ct_protonum(ct)) { >> >> case IPPROTO_TCP: >> >> - tcp = true; >> >> - if (ct->proto.tcp.state != TCP_CONNTRACK_ESTABLISHED) >> >> + if ((ctinfo != IP_CT_ESTABLISHED && >> >> + ctinfo != IP_CT_ESTABLISHED_REPLY) || >> >> + !test_bit(IPS_ASSURED_BIT, &ct->status) || >> >> + ct->proto.tcp.state != TCP_CONNTRACK_ESTABLISHED) >> >> return; >> >> + >> >> + tcp = true; >> >> break; >> >> case IPPROTO_UDP: >> >> + if (!nf_ct_is_confirmed(ct)) >> >> + return; >> >> + if (!test_bit(IPS_ASSURED_BIT, &ct->status)) >> >> + bidirectional = false; >> >> break; >> >> #ifdef CONFIG_NF_CT_PROTO_GRE >> >> case IPPROTO_GRE: { >> >> struct nf_conntrack_tuple *tuple; >> >> >> >> - if (ct->status & IPS_NAT_MASK) >> >> + if ((ctinfo != IP_CT_ESTABLISHED && >> >> + ctinfo != IP_CT_ESTABLISHED_REPLY) || >> >> + !test_bit(IPS_ASSURED_BIT, &ct->status) || >> >> + ct->status & IPS_NAT_MASK) >> >> return; >> >> + >> >> tuple = &ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple; >> >> /* No support for GRE v1 */ >> >> if (tuple->src.u.gre.key || tuple->dst.u.gre.key) >> >> @@ -446,7 +456,7 @@ static void tcf_ct_flow_table_process_conn(struct tcf_ct_flow_table *ct_ft, >> >> ct->status & IPS_SEQ_ADJUST) >> >> return; >> >> >> >> - tcf_ct_flow_table_add(ct_ft, ct, tcp); >> >> + tcf_ct_flow_table_add(ct_ft, ct, tcp, bidirectional); >> >> } >> >> >> >> static bool >> >> @@ -625,13 +635,27 @@ static bool tcf_ct_flow_table_lookup(struct tcf_ct_params *p, >> >> flow = container_of(tuplehash, struct flow_offload, tuplehash[dir]); >> >> ct = flow->ct; >> >> >> >> + if (dir == FLOW_OFFLOAD_DIR_REPLY && >> >> + !test_bit(NF_FLOW_HW_BIDIRECTIONAL, &flow->flags)) { >> >> + /* Only offload reply direction after connection became >> >> + * assured. >> >> + */ >> >> + if (test_bit(IPS_ASSURED_BIT, &ct->status)) >> >> + set_bit(NF_FLOW_HW_BIDIRECTIONAL, &flow->flags); >> >> + set_bit(NF_FLOW_HW_UPDATE, &flow->flags); >> >> + return false; >> >> + } >> >> + >> >> if (tcph && (unlikely(tcph->fin || tcph->rst))) { >> >> flow_offload_teardown(flow); >> >> return false; >> >> } >> >> >> >> - ctinfo = dir == FLOW_OFFLOAD_DIR_ORIGINAL ? IP_CT_ESTABLISHED : >> >> - IP_CT_ESTABLISHED_REPLY; >> >> + if (dir == FLOW_OFFLOAD_DIR_ORIGINAL) >> >> + ctinfo = test_bit(IPS_SEEN_REPLY_BIT, &ct->status) ? >> >> + IP_CT_ESTABLISHED : IP_CT_NEW; >> >> + else >> >> + ctinfo = IP_CT_ESTABLISHED_REPLY; >> >> >> >> flow_offload_refresh(nf_ft, flow); >> >> nf_conntrack_get(&ct->ct_general); >> >> -- >> >> 2.38.1 >> >> >>
On Tue, Jan 17, 2023 at 09:12:26PM +0200, Vlad Buslov wrote: > On Tue 17 Jan 2023 at 14:56, Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> wrote: > > On Tue, Jan 17, 2023 at 07:36:42PM +0200, Vlad Buslov wrote: > >> > >> On Tue 17 Jan 2023 at 12:41, Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> wrote: > >> > On Fri, Jan 13, 2023 at 05:55:47PM +0100, Vlad Buslov wrote: > >> >> When processing connections allow offloading of UDP connections that don't > >> >> have IPS_ASSURED_BIT set as unidirectional. When performing table lookup > >> > > >> > Hmm. Considering that this is now offloading one direction only > >> > already, what about skipping this grace period: > >> > > >> > In nf_conntrack_udp_packet(), it does: > >> > > >> > /* If we've seen traffic both ways, this is some kind of UDP > >> > * stream. Set Assured. > >> > */ > >> > if (test_bit(IPS_SEEN_REPLY_BIT, &ct->status)) { > >> > ... > >> > /* Still active after two seconds? Extend timeout. */ > >> > if (time_after(jiffies, ct->proto.udp.stream_ts)) { > >> > extra = timeouts[UDP_CT_REPLIED]; > >> > stream = true; > >> > } > >> > ... > >> > /* Also, more likely to be important, and not a probe */ > >> > if (stream && !test_and_set_bit(IPS_ASSURED_BIT, &ct->status)) > >> > nf_conntrack_event_cache(IPCT_ASSURED, ct); > >> > > >> > Maybe the patch should be relying on IPS_SEEN_REPLY_BIT instead of > >> > ASSURED for UDP? Just a thought here, but I'm not seeing why not. > >> > >> The issue with this is that if we offload both directions early, then > >> conntrack state machine will not receive any more packets and, > >> consecutively, will never change the flow state to assured. I guess that > > > > I'm missing how it would offload each direction independently. > > Wouldn't CT state machine see the 1st reply, because it is not > > offloaded yet, and match it to the original direction? > (ok, better reply this too instead of radio silence) > What I meant is that if both directions are offloaded as soon as > IPS_SEEN_REPLY_BIT is set, then nf_conntrack_udp_packet() will not be > called for that connection anymore and would never be able to transition > the connection to assured state. But main thing is, as I said in the Oh, right! > previous reply, that we don't need to offload such connection ATM. It > could be done in a follow-up if there is a use-case for it, maybe even > made somehow configurable (with BPF! :)), so it could be dynamically > controlled. > > > > >> could be mitigated somehow by periodically checking the hw stats and > >> transitioning the flow to assured based on them, but as I said in > >> previous email we don't want to over-complicate this series even more. > >> Also, offloading to hardware isn't free and costs both memory and CPU, > >> so it is not like offloading as early as possible is strictly beneficial > >> for all cases... > > > > Yup. > > > >> > >> > > >> >> for reply packets check the current connection status: If UDP > >> >> unidirectional connection became assured also promote the corresponding > >> >> flow table entry to bidirectional and set the 'update' bit, else just set > >> >> the 'update' bit since reply directional traffic will most likely cause > >> >> connection status to become 'established' which requires updating the > >> >> offload state. > >> >> > >> >> Signed-off-by: Vlad Buslov <vladbu@nvidia.com> > >> >> --- > >> >> net/sched/act_ct.c | 48 ++++++++++++++++++++++++++++++++++------------ > >> >> 1 file changed, 36 insertions(+), 12 deletions(-) > >> >> > >> >> diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c > >> >> index bfddb462d2bc..563cbdd8341c 100644 > >> >> --- a/net/sched/act_ct.c > >> >> +++ b/net/sched/act_ct.c > >> >> @@ -369,7 +369,7 @@ static void tcf_ct_flow_tc_ifidx(struct flow_offload *entry, > >> >> > >> >> static void tcf_ct_flow_table_add(struct tcf_ct_flow_table *ct_ft, > >> >> struct nf_conn *ct, > >> >> - bool tcp) > >> >> + bool tcp, bool bidirectional) > >> >> { > >> >> struct nf_conn_act_ct_ext *act_ct_ext; > >> >> struct flow_offload *entry; > >> >> @@ -388,6 +388,8 @@ static void tcf_ct_flow_table_add(struct tcf_ct_flow_table *ct_ft, > >> >> ct->proto.tcp.seen[0].flags |= IP_CT_TCP_FLAG_BE_LIBERAL; > >> >> ct->proto.tcp.seen[1].flags |= IP_CT_TCP_FLAG_BE_LIBERAL; > >> >> } > >> >> + if (bidirectional) > >> >> + __set_bit(NF_FLOW_HW_BIDIRECTIONAL, &entry->flags); > >> >> > >> >> act_ct_ext = nf_conn_act_ct_ext_find(ct); > >> >> if (act_ct_ext) { > >> >> @@ -411,26 +413,34 @@ static void tcf_ct_flow_table_process_conn(struct tcf_ct_flow_table *ct_ft, > >> >> struct nf_conn *ct, > >> >> enum ip_conntrack_info ctinfo) > >> >> { > >> >> - bool tcp = false; > >> >> - > >> >> - if ((ctinfo != IP_CT_ESTABLISHED && ctinfo != IP_CT_ESTABLISHED_REPLY) || > >> >> - !test_bit(IPS_ASSURED_BIT, &ct->status)) > >> >> - return; > >> >> + bool tcp = false, bidirectional = true; > >> >> > >> >> switch (nf_ct_protonum(ct)) { > >> >> case IPPROTO_TCP: > >> >> - tcp = true; > >> >> - if (ct->proto.tcp.state != TCP_CONNTRACK_ESTABLISHED) > >> >> + if ((ctinfo != IP_CT_ESTABLISHED && > >> >> + ctinfo != IP_CT_ESTABLISHED_REPLY) || > >> >> + !test_bit(IPS_ASSURED_BIT, &ct->status) || > >> >> + ct->proto.tcp.state != TCP_CONNTRACK_ESTABLISHED) > >> >> return; > >> >> + > >> >> + tcp = true; > >> >> break; > >> >> case IPPROTO_UDP: > >> >> + if (!nf_ct_is_confirmed(ct)) > >> >> + return; > >> >> + if (!test_bit(IPS_ASSURED_BIT, &ct->status)) > >> >> + bidirectional = false; > >> >> break; > >> >> #ifdef CONFIG_NF_CT_PROTO_GRE > >> >> case IPPROTO_GRE: { > >> >> struct nf_conntrack_tuple *tuple; > >> >> > >> >> - if (ct->status & IPS_NAT_MASK) > >> >> + if ((ctinfo != IP_CT_ESTABLISHED && > >> >> + ctinfo != IP_CT_ESTABLISHED_REPLY) || > >> >> + !test_bit(IPS_ASSURED_BIT, &ct->status) || > >> >> + ct->status & IPS_NAT_MASK) > >> >> return; > >> >> + > >> >> tuple = &ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple; > >> >> /* No support for GRE v1 */ > >> >> if (tuple->src.u.gre.key || tuple->dst.u.gre.key) > >> >> @@ -446,7 +456,7 @@ static void tcf_ct_flow_table_process_conn(struct tcf_ct_flow_table *ct_ft, > >> >> ct->status & IPS_SEQ_ADJUST) > >> >> return; > >> >> > >> >> - tcf_ct_flow_table_add(ct_ft, ct, tcp); > >> >> + tcf_ct_flow_table_add(ct_ft, ct, tcp, bidirectional); > >> >> } > >> >> > >> >> static bool > >> >> @@ -625,13 +635,27 @@ static bool tcf_ct_flow_table_lookup(struct tcf_ct_params *p, > >> >> flow = container_of(tuplehash, struct flow_offload, tuplehash[dir]); > >> >> ct = flow->ct; > >> >> > >> >> + if (dir == FLOW_OFFLOAD_DIR_REPLY && > >> >> + !test_bit(NF_FLOW_HW_BIDIRECTIONAL, &flow->flags)) { > >> >> + /* Only offload reply direction after connection became > >> >> + * assured. > >> >> + */ > >> >> + if (test_bit(IPS_ASSURED_BIT, &ct->status)) > >> >> + set_bit(NF_FLOW_HW_BIDIRECTIONAL, &flow->flags); > >> >> + set_bit(NF_FLOW_HW_UPDATE, &flow->flags); > >> >> + return false; > >> >> + } > >> >> + > >> >> if (tcph && (unlikely(tcph->fin || tcph->rst))) { > >> >> flow_offload_teardown(flow); > >> >> return false; > >> >> } > >> >> > >> >> - ctinfo = dir == FLOW_OFFLOAD_DIR_ORIGINAL ? IP_CT_ESTABLISHED : > >> >> - IP_CT_ESTABLISHED_REPLY; > >> >> + if (dir == FLOW_OFFLOAD_DIR_ORIGINAL) > >> >> + ctinfo = test_bit(IPS_SEEN_REPLY_BIT, &ct->status) ? > >> >> + IP_CT_ESTABLISHED : IP_CT_NEW; > >> >> + else > >> >> + ctinfo = IP_CT_ESTABLISHED_REPLY; > >> >> > >> >> flow_offload_refresh(nf_ft, flow); > >> >> nf_conntrack_get(&ct->ct_general); > >> >> -- > >> >> 2.38.1 > >> >> > >> >
diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c index bfddb462d2bc..563cbdd8341c 100644 --- a/net/sched/act_ct.c +++ b/net/sched/act_ct.c @@ -369,7 +369,7 @@ static void tcf_ct_flow_tc_ifidx(struct flow_offload *entry, static void tcf_ct_flow_table_add(struct tcf_ct_flow_table *ct_ft, struct nf_conn *ct, - bool tcp) + bool tcp, bool bidirectional) { struct nf_conn_act_ct_ext *act_ct_ext; struct flow_offload *entry; @@ -388,6 +388,8 @@ static void tcf_ct_flow_table_add(struct tcf_ct_flow_table *ct_ft, ct->proto.tcp.seen[0].flags |= IP_CT_TCP_FLAG_BE_LIBERAL; ct->proto.tcp.seen[1].flags |= IP_CT_TCP_FLAG_BE_LIBERAL; } + if (bidirectional) + __set_bit(NF_FLOW_HW_BIDIRECTIONAL, &entry->flags); act_ct_ext = nf_conn_act_ct_ext_find(ct); if (act_ct_ext) { @@ -411,26 +413,34 @@ static void tcf_ct_flow_table_process_conn(struct tcf_ct_flow_table *ct_ft, struct nf_conn *ct, enum ip_conntrack_info ctinfo) { - bool tcp = false; - - if ((ctinfo != IP_CT_ESTABLISHED && ctinfo != IP_CT_ESTABLISHED_REPLY) || - !test_bit(IPS_ASSURED_BIT, &ct->status)) - return; + bool tcp = false, bidirectional = true; switch (nf_ct_protonum(ct)) { case IPPROTO_TCP: - tcp = true; - if (ct->proto.tcp.state != TCP_CONNTRACK_ESTABLISHED) + if ((ctinfo != IP_CT_ESTABLISHED && + ctinfo != IP_CT_ESTABLISHED_REPLY) || + !test_bit(IPS_ASSURED_BIT, &ct->status) || + ct->proto.tcp.state != TCP_CONNTRACK_ESTABLISHED) return; + + tcp = true; break; case IPPROTO_UDP: + if (!nf_ct_is_confirmed(ct)) + return; + if (!test_bit(IPS_ASSURED_BIT, &ct->status)) + bidirectional = false; break; #ifdef CONFIG_NF_CT_PROTO_GRE case IPPROTO_GRE: { struct nf_conntrack_tuple *tuple; - if (ct->status & IPS_NAT_MASK) + if ((ctinfo != IP_CT_ESTABLISHED && + ctinfo != IP_CT_ESTABLISHED_REPLY) || + !test_bit(IPS_ASSURED_BIT, &ct->status) || + ct->status & IPS_NAT_MASK) return; + tuple = &ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple; /* No support for GRE v1 */ if (tuple->src.u.gre.key || tuple->dst.u.gre.key) @@ -446,7 +456,7 @@ static void tcf_ct_flow_table_process_conn(struct tcf_ct_flow_table *ct_ft, ct->status & IPS_SEQ_ADJUST) return; - tcf_ct_flow_table_add(ct_ft, ct, tcp); + tcf_ct_flow_table_add(ct_ft, ct, tcp, bidirectional); } static bool @@ -625,13 +635,27 @@ static bool tcf_ct_flow_table_lookup(struct tcf_ct_params *p, flow = container_of(tuplehash, struct flow_offload, tuplehash[dir]); ct = flow->ct; + if (dir == FLOW_OFFLOAD_DIR_REPLY && + !test_bit(NF_FLOW_HW_BIDIRECTIONAL, &flow->flags)) { + /* Only offload reply direction after connection became + * assured. + */ + if (test_bit(IPS_ASSURED_BIT, &ct->status)) + set_bit(NF_FLOW_HW_BIDIRECTIONAL, &flow->flags); + set_bit(NF_FLOW_HW_UPDATE, &flow->flags); + return false; + } + if (tcph && (unlikely(tcph->fin || tcph->rst))) { flow_offload_teardown(flow); return false; } - ctinfo = dir == FLOW_OFFLOAD_DIR_ORIGINAL ? IP_CT_ESTABLISHED : - IP_CT_ESTABLISHED_REPLY; + if (dir == FLOW_OFFLOAD_DIR_ORIGINAL) + ctinfo = test_bit(IPS_SEEN_REPLY_BIT, &ct->status) ? + IP_CT_ESTABLISHED : IP_CT_NEW; + else + ctinfo = IP_CT_ESTABLISHED_REPLY; flow_offload_refresh(nf_ft, flow); nf_conntrack_get(&ct->ct_general);
When processing connections allow offloading of UDP connections that don't have IPS_ASSURED_BIT set as unidirectional. When performing table lookup for reply packets check the current connection status: If UDP unidirectional connection became assured also promote the corresponding flow table entry to bidirectional and set the 'update' bit, else just set the 'update' bit since reply directional traffic will most likely cause connection status to become 'established' which requires updating the offload state. Signed-off-by: Vlad Buslov <vladbu@nvidia.com> --- net/sched/act_ct.c | 48 ++++++++++++++++++++++++++++++++++------------ 1 file changed, 36 insertions(+), 12 deletions(-)