Date:   Mon, 20 Feb 2023 11:37:59 +0100
From:   Simon Horman <simon.horman@corigine.com>
To:     Eddy Tao <taoyuan_eddy@hotmail.com>
Cc:     netdev@vger.kernel.org, Pravin B Shelar <pshelar@ovn.org>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>, dev@openvswitch.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next v1 1/1] net: openvswitch: ovs_packet_cmd_execute
 put sw_flow mainbody in stack
Message-ID: <Y/NNh44zpBwPUacq@corigine.com>
References: <OS3P286MB229509B8AD0264CC84F57845F5A69@OS3P286MB2295.JPNP286.PROD.OUTLOOK.COM>
 <Y/IqJvWEJ+UNOs6i@corigine.com>
 <OS3P286MB22953C8741F17F500879FA86F5A49@OS3P286MB2295.JPNP286.PROD.OUTLOOK.COM>
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <OS3P286MB22953C8741F17F500879FA86F5A49@OS3P286MB2295.JPNP286.PROD.OUTLOOK.COM>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?bzQ0Rjhya0FhZnhyYSsrTVg4UFJObXp6ekdJdG5SL1NXNzh3UnRHK05qZmJJ?=
 =?utf-8?B?VVRRQzFjVmM5aS9rZklFN3RkcSs2TXJYWDd1TnhJZzFJb214Nnk5aDZxenZ2?=
 =?utf-8?B?YUlMb3dkckdXZmRRdnI2bGpXUG9ieWJ2aVk5R2RtNk81alBVdjdhS1h4bzBw?=
 =?utf-8?B?RGU3eHlrMG1PS0tyU2VwbnpjSXdjbVdaYUk1SnRXTTFkZGQ5UmhDU0tsY29F?=
 =?utf-8?B?NGlCOW1ZS2dGckpjTktDR0ttaDJSRjRMSG1YZkRmdldycDRKWEZhNHJNUFJj?=
 =?utf-8?B?VTNmdG02MGRZNGFBVlVhQzBTbDByTHRCbnVZQ2RCRVhmM09mbHFsRHFQVzVq?=
 =?utf-8?B?TFRsaVBYaXY2QlZFMlcxQWxsSCtKK1hNWmlpNWtaOTgvUFBweDJLSFZDVzBK?=
 =?utf-8?B?Q01wYW5SYjdscVN3UzJaek4zT2F3RFU3SWoxTTFwb2lVL0hYUGdVSEtjckVB?=
 =?utf-8?B?UWVQTDU1Z1VUTktacDZSdFNvOWNHWVo5MVlGM3dwOFRzWlhGQTNuRHlkU2kw?=
 =?utf-8?B?d3MyOU45U05zM2M4L3pKandJNmllNGZzWGhldVp3anJqWEwxbVR6dW1td2lZ?=
 =?utf-8?B?M28zQkhSOTVZVU1jNlZyTFVkZG9LUWRITjEzWFhCRWVCWUNLOVJrRGU1MnJu?=
 =?utf-8?B?dFJsS2J2Uk1HTm9COFNJODZ4SytaM0hHT2J3eTg4QS9Gd3R6KzV0NDlIRDQx?=
 =?utf-8?B?bjNFc3hzQkdHcXRVcktsRzVFUWljTmMxTDRERkZxSWQxVHZMcVJCT3dXSnd0?=
 =?utf-8?B?R01DMWdQdG9WM0N2UmxYMHZ3VFBUWDBQMHo2Mit3c1FsemQxWThFTy80Ynhm?=
 =?utf-8?B?eVdRMUVFcnlxOWtCV3dKQWw5bFVGTWhMamZsZHNHTk5FTzhSY1haekxhTWJi?=
 =?utf-8?B?WUplMUNGWlgwSXE0b1FBZ0huQzdBTFBkMG1IRHByUWJRNHhzMjNNNjdURmZr?=
 =?utf-8?B?eFZyaGZUcDYvQXZuVXZUb0Q2RDZBc0lRR2wxMy9HOHFzeTZEd2ZvUXZYL2Vy?=
 =?utf-8?B?aEYwRTJOeXR1L29CVldjcE02aFBuUGJJNXNCT2M0bmlHMGpib011VlVBSkEz?=
 =?utf-8?B?TkJjWW94bXBSZy9rWkVuZkxOdktuT2U0ckp3MVlBN3p5WWkvTVArOEZSQmpG?=
 =?utf-8?B?aVdXRDY3T0ZLdkMraExoMTRBMXl1blQ1SG40TW0yK2tLY3o2NWpleDNWSjJ4?=
 =?utf-8?B?K1dNTFU0S3RvUjFHYTVPb04yNXNvMDVvN1hYUTkvcnBSMithWTF6bnl4S0l2?=
 =?utf-8?B?Wmg0a01qeWp5T0pwOEZ5T29VcFJ3RjFPUnpZVG9RdDFGT3VwQ3hvanZnMmRs?=
 =?utf-8?B?bWMzQkZ3WnBsc1k4d2xtdFR0K25rcDJuODRFZ1I1VkczczY4WHlBSWljMlFG?=
 =?utf-8?B?Sk9JTWc5THJKS3NCMTMxN3FJQ2FuU2hMWVZvaGo5b3BiY3dJUmhtVHZQV2oz?=
 =?utf-8?B?emJJT29QZWEwd3pvUE43c09nZEhYcnp5VXlSbTFvcW56cE9jY3VWUWVWQjc5?=
 =?utf-8?B?RVNSaSt3NGpjcXVrbnN2azdRaVRPT1AzMG9IU0NDN2JvbHZGdnQ4akxIQmpG?=
 =?utf-8?B?dENqVk1wZlhPVUMvMFZCbGtOVG16bGROWGZrbTlsL3FJV2doRjZuT3VZd2hr?=
 =?utf-8?B?U2p1M2EycEFNenlzNEs3eTlTSHZ4dFJjQ2pzV1ZXT1M0MldZdkU3VHNka0tj?=
 =?utf-8?B?a0pBZ3JKZEJBdEpFZlVhcktReTFvOVE4cnVaSkdKaHRPa1prdU5SaUI4VVpI?=
 =?utf-8?B?LzBEYXk5N21LSGxXdVV3Z3FLTlFSeS9QZ2JVU1FWUFUyOVVQTFZ5Mm12YjdK?=
 =?utf-8?B?NHR2aXA0SmtObGI0ZUcyZVVIc1U0U1NTYkZFR3NLUGg5dkRuMjhuNVpZZjAv?=
 =?utf-8?B?cTdLUGRrNDVSRTlLWW9HRnFUWnlzNmRVa1AzMms2TDJhQTdxVjdyVDVObzVx?=
 =?utf-8?B?ditubTM5RVNDOVlGeXpkRXJGc3VjbzlQKzhvRCtzeWcrOUZheVR0eWNTSDN5?=
 =?utf-8?B?Sml2cEdXUGJaamlUSFVLZUo3SWdJUnZ4dWtaZ0lGM0ZKeEViVkFwR0tZQXBQ?=
 =?utf-8?B?V3VnTmlGdHNsUEVEU2NoeWRYUndobFJhZ1dMSDRWWk5ZT25rM2VaWFhLVFBv?=
 =?utf-8?B?RytZVmpVakUxWFRxMktwbzdaVHZRK21TZ2lBUGYwczhjQlE4bXZUK1hCRFNE?=
 =?utf-8?B?dlhVWW4wM3NwUGVCTzBxZDN2Y2ZUQkVoSmVCUGJxM21xRU5OZjdLZGcxenVr?=
 =?utf-8?B?S0hhK1YybkZjNkgrQXVsTitqOGd3PT0=?=
X-OriginatorOrg: corigine.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 0905338a-b409-4f22-daae-08db132e8cba
X-MS-Exchange-CrossTenant-AuthSource: PH0PR13MB4842.namprd13.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Feb 2023 10:38:05.5098
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: fe128f2c-073b-4c20-818e-7246a585940c
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: uW21yxNfwdyEyXZm9H/ZY76H2tpAjXZ0VnQXuctCsIYxZ94pvfiggj8Hw33GEMniKQY1eVz3e/SNstUdoPBhmlL9ciWlA3fk5z0iAtIrTZg=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR13MB4087
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Mon, Feb 20, 2023 at 03:11:17PM +0800, Eddy Tao wrote:
> Hi, Simon:
> 
> 
>     About your concern for the stack size, it leads to more room for
> improvement.
> 
> I will file a new version which will have smaller stack occupation and
> better performance
> 
> 
> The new revision is invoked by existing examples of using struct in stack,
> in the same file net/openvswitch/datapath.c
> 
> struct sw_flow_actions *get_flow_actions(..)
> {
>     struct sw_flow_key masked_key;==> sizeof sw_flow_key is 464 bytes
> 
> static noinline_for_stack int
> ovs_nla_init_match_and_action(..)
> {
>     struct sw_flow_mask mask;==> sizeof sw_flow_mask is 496 bytes
> 
> 
> The first example reminded me, revisiting the code in
> ovs_packet_cmd_execute, basically sw_flow serves as a container for
> sw_flow_actions and sw_flow_key only.
> 
> We do not need the bulk of tunnel info memory in sw_flow, which saves us
> 200+ bytes further -- less is more.
> 
> 
> The new revision will be presented shortly after some sanity and benchmark

Thanks, for addressing my review.
It is the stack size issue that is my greatest concern.

Please consider including performance results
in the patch description of v2.

Kind regards,
Simon