From: Daniel Borkmann <daniel@iogearbox•net>
To: Willem de Bruijn <willemdebruijn.kernel@gmail•com>
Cc: Network Development <netdev@vger•kernel.org>,
David Miller <davem@davemloft•net>,
Florian Westphal <fw@strlen•de>,
dborkman@iogearbox•net, Jamal Hadi Salim <jhs@mojatatu•com>,
Alexei Starovoitov <alexei.starovoitov@gmail•com>,
Willem de Bruijn <willemb@google•com>
Subject: Re: [PATCH net-next rfc 5/6] net-tc: convert tc_at to tc_at_ingress
Date: Wed, 04 Jan 2017 20:51:13 +0100 [thread overview]
Message-ID: <586D5231.5080706@iogearbox.net> (raw)
In-Reply-To: <CAF=yD-+x7zP2Z5Ctsn9-_uYz7sfdZMf4n414CE1-bzVQcN3nBA@mail.gmail.com>
On 01/04/2017 08:26 PM, Willem de Bruijn wrote:
>>> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
>>> index ef53ede..be4e18d 100644
>>> --- a/net/sched/sch_api.c
>>> +++ b/net/sched/sch_api.c
>>> @@ -1865,6 +1865,7 @@ int tc_classify(struct sk_buff *skb, const struct
>>> tcf_proto *tp,
>>> const struct tcf_proto *old_tp = tp;
>>> int limit = 0;
>>>
>>> + skb->tc_at_ingress = !!(tp && tp->q->flags & TCQ_F_INGRESS);
>>
>> I'd prefer if skb->tc_at_ingress is set directly to 0/1 in
>> sch_handle_ingress()
>> and __dev_queue_xmit() as we do right now, this would avoid above tests in
>> fast
>> path and it would also avoid to set the same thing in tc_classify() multiple
>> times f.e. on egress path walking through multiple qdiscs. I don't see
>> anything
>> in layers above tc that would read it and expect an AT_STACK-like
>> equivalent.
>> skb_reset_tc() could thus still remain as you have above in fast-path like
>> __netif_receive_skb_core().
>
> I had been thinking about that. After submitting this I noticed that Florian's
> patchset had an elegant solution to avoid the branch: set tc_at_ingress in
> handle_ing before tc_classify and clear it on the return path.
>
> Then we only set + clear it once on ingress regardless of the depth
> of classifiers and do not touch it at all in other code.
>
> https://patchwork.ozlabs.org/patch/472698/
>
> What do you think of that approach?
I think this approach might not work, it would certainly change semantics
since right now *before* going into tc_classify() we always update skb->tc_verd's
"at" location. After the patch we'd set TC_AT_INGRESS in ingress and could
redirect within tc_classify() [and we'd skip that skb->tc_verd = 0 we have in
__netif_receive_skb_core() for these] to xmit from there where next call into
classifier from __dev_queue_xmit() call-site misses that we're not at ingress
anymore but already at egress, so it would do wrong pull/push of headers in
some cls, for example. I mentioned above that I think your skb_reset_tc() for
the __netif_receive_skb_core() fast-path could stay as you have it in that patch
as afaik there's no user that reads out this value in above layers, so if we
keep the "at" info always correct before going into tc_classify(), we should
be good.
Thanks,
Daniel
next prev parent reply other threads:[~2017-01-04 19:51 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-12-28 19:13 [PATCH net-next rfc 0/6] convert tc_verd to integer bitfields Willem de Bruijn
2016-12-28 19:13 ` [PATCH net-next rfc 1/6] net-tc: remove unused tc_verd fields Willem de Bruijn
2016-12-28 19:13 ` [PATCH net-next rfc 2/6] net-tc: make MAX_RECLASSIFY_LOOP local Willem de Bruijn
2016-12-28 19:13 ` [PATCH net-next rfc 3/6] net-tc: extract skip classify bit from tc_verd Willem de Bruijn
2016-12-28 19:13 ` [PATCH net-next rfc 4/6] net-tc: convert tc_verd to integer bitfields Willem de Bruijn
2016-12-28 19:13 ` [PATCH net-next rfc 5/6] net-tc: convert tc_at to tc_at_ingress Willem de Bruijn
2017-01-04 19:18 ` Daniel Borkmann
2017-01-04 19:26 ` Willem de Bruijn
2017-01-04 19:51 ` Daniel Borkmann [this message]
2017-01-04 20:20 ` Willem de Bruijn
2016-12-28 19:13 ` [PATCH net-next rfc 6/6] net-tc: convert tc_from to tc_from_ingress and tc_redirected Willem de Bruijn
2016-12-30 3:21 ` [PATCH net-next rfc 0/6] convert tc_verd to integer bitfields David Miller
2016-12-31 0:02 ` Willem de Bruijn
2017-01-02 16:09 ` Jamal Hadi Salim
2017-01-03 17:05 ` Willem de Bruijn
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=586D5231.5080706@iogearbox.net \
--to=daniel@iogearbox$(echo .)net \
--cc=alexei.starovoitov@gmail$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=dborkman@iogearbox$(echo .)net \
--cc=fw@strlen$(echo .)de \
--cc=jhs@mojatatu$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=willemb@google$(echo .)com \
--cc=willemdebruijn.kernel@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox