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

[Vlad Buslov <vladbu@nvidia.com>]

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