netdev - Re: [PATCH net-next v3 2/3] net: sched: Make tc-related drop reason more flexible for remaining qdiscs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Tue, 12 Dec 2023 11:27:52 -0500
From: Jamal Hadi Salim <jhs@...atatu.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: Victor Nogueira <victor@...atatu.com>, xiyou.wangcong@...il.com, jiri@...nulli.us, 
	davem@...emloft.net, edumazet@...gle.com, pabeni@...hat.com, 
	daniel@...earbox.net, dcaratti@...hat.com, netdev@...r.kernel.org, 
	kernel@...atatu.com
Subject: Re: [PATCH net-next v3 2/3] net: sched: Make tc-related drop reason
 more flexible for remaining qdiscs

On Mon, Dec 11, 2023 at 9:25 PM Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Tue,  5 Dec 2023 17:50:29 -0300 Victor Nogueira wrote:
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index 4b84b72ebae8..f38c928a34aa 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -3753,6 +3753,8 @@ static inline int __dev_xmit_skb(struct sk_buff *skb, struct Qdisc *q,
> >
> >       qdisc_calculate_pkt_len(skb, q);
> >
> > +     tcf_set_drop_reason(skb, SKB_DROP_REASON_QDISC_DROP);
> > +
> >       if (q->flags & TCQ_F_NOLOCK) {
> >               if (q->flags & TCQ_F_CAN_BYPASS && nolock_qdisc_is_empty(q) &&
> >                   qdisc_run_begin(q)) {
> > @@ -3782,7 +3784,7 @@ static inline int __dev_xmit_skb(struct sk_buff *skb, struct Qdisc *q,
> >  no_lock_out:
> >               if (unlikely(to_free))
> >                       kfree_skb_list_reason(to_free,
> > -                                           SKB_DROP_REASON_QDISC_DROP);
> > +                                           tcf_get_drop_reason(to_free));
> >               return rc;
> >       }
> >
> > @@ -3837,7 +3839,8 @@ static inline int __dev_xmit_skb(struct sk_buff *skb, struct Qdisc *q,
> >       }
> >       spin_unlock(root_lock);
> >       if (unlikely(to_free))
> > -             kfree_skb_list_reason(to_free, SKB_DROP_REASON_QDISC_DROP);
> > +             kfree_skb_list_reason(to_free,
> > +                                   tcf_get_drop_reason(to_free));
>
> You stuff the drop reason into every skb but then only use the one from
> the head? Herm. __qdisc_drop() only uses the next pointer can't we
> overload the prev pointer to carry the drop reason. That means only
> storing it if we already plan to drop the packet.
>

So when we looked at this code there was some mystery. It wasnt clear
how to_free could have more than one skb.
According to: 520ac30f4551 Eric says:

"I measured a performance increase of up to 12 %, but this patch is a
prereq so that future batches in enqueue() can fly. "

I am not sure if the batch enqueue ever happened and if it didnt then
there's only one skb in that list... When 7faef0547f4c added the
reason code it assumed a single code for the "list" - so it felt safer
to leave it that way. I guess it will depend on if a list could exist
to rethink this. Eric?

> BTW I lack TC knowledge but struct tc_skb_cb is even more clsact
> specific today than tcf_result. And reserving space for drop reason
> in a state structure seems odd. Maybe that's just me.

It is not clsact main use - it is a scratchpad for all of tc:

struct qdisc_skb_cb {
        struct {
                unsigned int            pkt_len;
                u16                     slave_dev_queue_mapping;
                u16                     tc_classid;
        };
#define QDISC_CB_PRIV_LEN 20
        unsigned char           data[QDISC_CB_PRIV_LEN];
};

struct tc_skb_cb {
        struct qdisc_skb_cb qdisc_cb;

        u16 mru;
        u8 post_ct:1;
        u8 post_ct_snat:1;
        u8 post_ct_dnat:1;
        u16 zone; /* Only valid if post_ct = true */
};

cheers,
jamal