Re: [PATCH net-next v2 6/7] net/sched: act_ct: offload UDP NEW connections

[Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>]

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
> >> >> 
> >> 
>