On Tue 17 Jan 2023 at 14:56, Marcelo Ricardo Leitner <marcelo.leitner@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxxx>
>> >> ---
>> >> 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
>> >>
>>
