From: Aaron Conole <aconole@redhat•com>
To: Ilya Maximets <i.maximets@ovn•org>
Cc: Nicholas Piggin <npiggin@gmail•com>,
netdev@vger•kernel.org, dev@openvswitch•org,
Eelco Chaudron <echaudro@redhat•com>,
Simon Horman <horms@ovn•org>
Subject: Re: [ovs-dev] [RFC PATCH 0/7] net: openvswitch: Reduce stack usage
Date: Tue, 03 Oct 2023 09:31:48 -0400 [thread overview]
Message-ID: <f7tcyxvj0hn.fsf@redhat.com> (raw)
In-Reply-To: <2c01d102-3c84-3edc-a92a-a0b9a889d70d@ovn.org> (Ilya Maximets's message of "Mon, 2 Oct 2023 13:56:02 +0200")
Ilya Maximets <i.maximets@ovn•org> writes:
> On 9/29/23 09:06, Nicholas Piggin wrote:
>> On Wed Sep 27, 2023 at 6:36 PM AEST, Ilya Maximets wrote:
>>> On 9/27/23 02:13, Nicholas Piggin wrote:
>>>> Hi,
>>>>
>>>> We've got a report of a stack overflow on ppc64le with a 16kB kernel
>>>> stack. Openvswitch is just one of many things in the stack, but it
>>>> does cause recursion and contributes to some usage.
>>>>
>>>> Here are a few patches for reducing stack overhead. I don't know the
>>>> code well so consider them just ideas. GFP_ATOMIC allocations
>>>> introduced in a couple of places might be controversial, but there
>>>> is still some savings to be had if you skip those.
>>>>
>>>> Here is one place detected where the stack reaches >14kB before
>>>> overflowing a little later. I massaged the output so it just shows
>>>> the stack frame address on the left.
>>>
>>> Hi, Nicholas. Thanks for the patches!
>>>
>>> Though it looks like OVS is not really playing a huge role in the
>>> stack trace below. How much of the stack does the patch set save
>>> in total? How much patches 2-7 contribute (I posted a patch similar
>>> to the first one last week, so we may not count it)?
>>
>> Stack usage was tested for the same path (this is backported to
>> RHEL9 kernel), and saving was 2080 bytes for that. It's enough
>> to get us out of trouble. But if it was a config that caused more
>> recursions then it might still be a problem.
>
> The 2K total value likely means that only patches 1 and 4 actually
> contribute much into the savings. And I agree that running at
> 85%+ stack utilization seems risky. It can likely be overflowed
> by just a few more recirculations in OVS pipeline or traversing
> one more network namespace on a way out. And it's possible that
> some of the traffic will take such a route in your system even if
> you didn't see it yet.
>
>>> Also, most of the changes introduced here has a real chance to
>>> noticeably impact performance. Did you run any performance tests
>>> with this to assess the impact?
>>
>> Some numbers were posted by Aaron as you would see. 2-4% for that
>> patch, but I suspect the rest should have much smaller impact.
>
> They also seem to have a very small impact on the stack usage,
> so may be not worth touching at all, since performance evaluation
> for them will be necessary before they can be accepted.
Actually, it's also important to keep in mind that the vport_receive is
only happening once in my performance test. I expect it gets worse when
running in the scenario (br-ex, br-int, br-tun setup).
>>
>> Maybe patch 2 if you were doing a lot of push_nsh operations, but
>> that might be less important since it's out of the recursive path.
>
> It's also unlikely that you have NHS pipeline configured in OVS.
>
>>
>>>
>>> One last thing is that at least some of the patches seem to change
>>> non-inlined non-recursive functions. Seems unnecessary.
>>>
>>> Best regards, Ilya Maximets.
>>>
>>
>> One thing I do notice in the trace:
>>
>>
>> clone_execute is an action which can be deferred AFAIKS, but it is
>> not deferred until several recursions deep.
>>
>> If we deferred always when possible, then might avoid such a big
>> stack (at least for this config). Is it very costly to defer? Would
>> it help here, or is it just going to process it right away and
>> cause basically the same call chain?
>
> It may save at most two stack frames maybe, because deferred actions
> will be called just one function above in ovs_execute_actions(), and
> it will not save us from packets exiting openvswitch module and
> re-entering from a different port, which is a case in the provided
> trace.
It used to always be deferred but, IIUC we were hitting deferred actions
limit quite a bit so when the sample and clone actions were unified there
was a choice to recurse to avoid dropping packets.
> Also, I'd vote against deferring, because then we'll start hitting
> the limit of deferred actions much faster causing packet drops, which
> is already a problem for some OVN deployments. And deferring involves
> copying a lot of memory, which will hit performance once again.
>
> Best regards, Ilya Maximets.
prev parent reply other threads:[~2023-10-03 13:32 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-27 0:13 [RFC PATCH 0/7] net: openvswitch: Reduce stack usage Nicholas Piggin
2023-09-27 0:13 ` [RFC PATCH 1/7] net: openvswitch: Move NSH buffer out of do_execute_actions Nicholas Piggin
2023-09-27 8:26 ` [ovs-dev] " Ilya Maximets
2023-09-27 10:03 ` Nicholas Piggin
2023-09-27 0:13 ` [RFC PATCH 2/7] net: openvswitch: Reduce execute_push_nsh stack overhead Nicholas Piggin
2023-09-27 0:13 ` [RFC PATCH 3/7] net: openvswitch: uninline action execution Nicholas Piggin
2023-09-27 0:13 ` [RFC PATCH 4/7] net: openvswitch: ovs_vport_receive reduce stack usage Nicholas Piggin
2023-09-28 15:26 ` [ovs-dev] " Aaron Conole
2023-09-29 7:00 ` Nicholas Piggin
2023-09-29 8:38 ` Eelco Chaudron
2023-10-04 7:11 ` Nicholas Piggin
2023-10-04 15:15 ` Aaron Conole
2023-10-05 2:01 ` Nicholas Piggin
2023-10-11 13:34 ` Aaron Conole
2023-10-11 23:58 ` Nicholas Piggin
2023-10-04 7:29 ` Nicholas Piggin
2023-10-04 15:16 ` Aaron Conole
2023-09-27 0:13 ` [RFC PATCH 5/7] net: openvswitch: uninline ovs_fragment to control " Nicholas Piggin
2023-09-27 0:13 ` [RFC PATCH 6/7] net: openvswitch: Reduce ovs_fragment " Nicholas Piggin
2023-09-27 0:13 ` [RFC PATCH 7/7] net: openvswitch: Reduce stack usage in ovs_dp_process_packet Nicholas Piggin
2023-09-27 8:36 ` [ovs-dev] [RFC PATCH 0/7] net: openvswitch: Reduce stack usage Ilya Maximets
2023-09-28 1:52 ` Nicholas Piggin
2023-10-02 11:54 ` Ilya Maximets
2023-10-04 9:56 ` Nicholas Piggin
2023-09-29 7:06 ` Nicholas Piggin
2023-10-02 11:56 ` Ilya Maximets
2023-10-03 13:31 ` Aaron Conole [this message]
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=f7tcyxvj0hn.fsf@redhat.com \
--to=aconole@redhat$(echo .)com \
--cc=dev@openvswitch$(echo .)org \
--cc=echaudro@redhat$(echo .)com \
--cc=horms@ovn$(echo .)org \
--cc=i.maximets@ovn$(echo .)org \
--cc=netdev@vger$(echo .)kernel.org \
--cc=npiggin@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