Message-ID: <OS3P286MB229533DAC2CF9CE98C54B428F5D89@OS3P286MB2295.JPNP286.PROD.OUTLOOK.COM>
Date:   Wed, 8 Feb 2023 11:08:44 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [PATCH net-next v1 1/1] net: openvswitch: remove unnecessary vlan
 init in key_extract
Content-Language: en-US
To:     Simon Horman <simon.horman@corigine.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
References: <OS3P286MB229551D6705894E6578778DCF5DB9@OS3P286MB2295.JPNP286.PROD.OUTLOOK.COM>
 <Y+KIr5Pwlpoy/sn3@corigine.com>
From:   Eddy Tao <taoyuan_eddy@hotmail.com>
In-Reply-To: <Y+KIr5Pwlpoy/sn3@corigine.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-MessageSentRepresentingType: 1
X-MS-PublicTrafficType: Email
X-MS-TrafficTypeDiagnostic: OS3P286MB2295:EE_|OS3P286MB1616:EE_
X-MS-Office365-Filtering-Correlation-Id: 9c10686a-d01e-4547-f54d-08db0981cba1
X-Microsoft-Antispam: BCL:0;
X-Microsoft-Antispam-Message-Info: WZeoLUR4lZtaXO+G6eSIE625R6aUAcgJ+ef1mhON54+APKaSWyweO1+9FekGUCjNpUaftG4DyLCPchE/GVD9CH/IkqLKze30+kRTEVtvdaCMBBNRDPW95R3ffvqYx3xmbMAGchJsRhW1+ufP0UTRqtY6kvbzQ1BmHclfmlVSTc9kxiM2US+ximCkqPY4k/OfYLYbwi+tWC6oTznJP4s6+Rvdt+NacsanTcTwQo7IZovFipcRUwJMy3uyqEhLfb20Qt6sKOPm9SeVIma2Ya4gNuFoKxDSyNnFn0p+qiULAlY/RYNdfk2oIA9TU5QMtFMrtrmaH209lZbmVjGWgHgK/Ff9o4PHnm8hDjWLZiPdzsiD+r8F5hDgC5Tw1SOLp4DSw6dKQFyZ2BbCKU5jmFcQLkQe2dhMpVy0hyZqwwUeaED55wRSTsikvfTv365V0M42KZS8toENmjxUUrqMAcTuQMbt6RfXWBN/ket4/YAF/Xc7gk5tWSSQfkRwclYKpcVdopJnPq5w1GmcgFJdqo8yoQ3h30uwDZ6fv4ENbwJkjgjQm7+xrsGBLtyGVFJHqLQgbAGZP8VzQbl4kPBpYU/6scdrmIvuPpaem9tLWcU+cSrcmpYeJlrAJ5Ma1lfoxbsqNgIi60uet91xmn+2Na/E0g==
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?a3pVUHV3dlM5ZjJLSGxoRGt1MUN5Z0FZOHltcnpFUUFNNVpNTHpOdFN6bjND?=
 =?utf-8?B?UjdRcnlaMlIzRTIrYU55VzkybFNQcVhqOFJZSFRwTnpYZktSaVdYcE5RV1Vw?=
 =?utf-8?B?NUdVeXJGdzhjcUhiekRxbzF5L05YWnRxSnpCMmhJdmZZZzBKb3JZb3BFS21S?=
 =?utf-8?B?emhYSUhUdUNpWVdNVXJIT1Q5eDk4ZVJPN3VEcXZXNXdrclB1V0h5NVY2MXpz?=
 =?utf-8?B?QVJTcWRFQVV1UGd4U1NFVUJvN09HOTNMVUp1T2QwbVcxZjB2K1Q3aFZYakhz?=
 =?utf-8?B?RlZldDYwZjJPSGxKb2dZdjhoZG4rdDFZbmJ0WjV4RUMxdnh0ejNaTFlvbXlW?=
 =?utf-8?B?VzdnNzdKZzVCTm5pNWJPWE1KakFoMzZjWmlpd2NmNXZvNEJLNWlocHRUZ0xY?=
 =?utf-8?B?WE83R2hDY0doK2t2andKT1J6MlNlRGc2dHVVejlVTnpPSDY2N3JpWmhJNVVu?=
 =?utf-8?B?MGVqMHo0dVJJQmlJTi9xSVBPVDU2M1JlMFB5TGpwRC9BeGNOR2tnRGlNMkNv?=
 =?utf-8?B?eHFXeTM3SWk1bFlsYU0rWHlDRy9qRHY2MUd1NlNlRVZvQ3R2MDNXeXVBRkhC?=
 =?utf-8?B?RzZrTU9kaHRDUlNRYndnNzR3ZEVJbTlHaitsTkZOSmFIcGc3WTNCYlM1cWRH?=
 =?utf-8?B?MHArTlhORVovRWFvVllVaWR4bS9qNFB6L3FUQnhuS0xqbzlnZFpvb00xOHo2?=
 =?utf-8?B?UHNUaGZVcDZLd1ZJRTFoejVncFdtTTAyTW1GQlJHQXRySTdhNXFXZHp5WTJv?=
 =?utf-8?B?TTA5Wk41N244bUFZQUlpUGRheWppNzZNVHc1M2FSVTMyeE1hcUVtK2lHc1Fs?=
 =?utf-8?B?SU01MjhCNjltaHFyNjZPTk0xWjJJMkdTTWpjYnlsMEtoME1HZUxRZWM1Z1VP?=
 =?utf-8?B?UTllei9VTzd5ZTYrLzFqQzl1QWxjS2tVdlhrSEtSNVJ6L1hNVVVtaW05Sm1u?=
 =?utf-8?B?K1BSOXZWb1dtV0FqLzBzOEY3K0lBalU0Z3ZNcWJUbUNYVzVkS0Q0dU1IWWdX?=
 =?utf-8?B?Sy8rREVZL1FsM2ZsbWswcDc4NzdsT0tNNVJyQjdvZXBmLzV0dnZ4dVMxUUl0?=
 =?utf-8?B?cllJbTV2akdpV2ZzQy82SzV4bVJqRWxkbVl3OVhDNStRNXRJTnhUdHpDWE5E?=
 =?utf-8?B?SnJhRGhFWDJYTVl4dDE3ZHdxQjlhMnNCaXJzbndGa01yZ0liZlJ2SGtmVE1O?=
 =?utf-8?B?TnNuVlVOMmhFU0E0UGdPOE1GeFJDY1lFZTlQSmtzNzVjR1JUOUlUdGpoR29K?=
 =?utf-8?B?dUI5VkpqdmZub2ZKME9lWnp4aUNxL3c0dDM1ckViaVBSOEEra0hyTU8zZTZv?=
 =?utf-8?B?Nk5GUFlBWWNDWVVPM2x1UFZRKzFFa0FhbGg1RHl6ZDVsbThHRjFzZEVtT1po?=
 =?utf-8?B?ZklrRGY0d0V5bXdXWEpBcjRkUFA4UGVZOEVacU05dWtkTnVuYmNFWEZYcWJW?=
 =?utf-8?B?UWpPRlJHMFlkQVV3a2RTVlFGUEtmVk5oNmJkeGdFTHhDZWsxMVdSaFhSNFdW?=
 =?utf-8?B?RHNKeWJ2dXJkUzVNNFA1amZrRlFmUGR5VmlhSkJycGhSZkpQNlAzRlB4MS9Q?=
 =?utf-8?B?SDhMRFgrbW1NeFpOOHZsNy9FVVY1WkZnZTc1SjcwWk85akpHUU5EUkZiUHBo?=
 =?utf-8?B?MVAzUnBic2ZyTE5sVjBqcVRYRVVWTFlSazdwZTVZN1RXdUs4QWVPRXQrQ21p?=
 =?utf-8?B?cWZrRDI2OHhqVXM2T2dBSGVoc1RwRC96UCtQQ0hwZUdLSlFCRmlYKzZ1L2Nw?=
 =?utf-8?Q?tq9SxgBUHuKofLXxZ4=3D?=
X-OriginatorOrg: sct-15-20-4755-11-msonline-outlook-05f45.templateTenant
X-MS-Exchange-CrossTenant-Network-Message-Id: 9c10686a-d01e-4547-f54d-08db0981cba1
X-MS-Exchange-CrossTenant-AuthSource: OS3P286MB2295.JPNP286.PROD.OUTLOOK.COM
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Feb 2023 03:08:47.8001
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 84df9e7f-e9f6-40af-b435-aaaaaaaaaaaa
X-MS-Exchange-CrossTenant-RMS-PersistedConsumerOrg: 00000000-0000-0000-0000-000000000000
X-MS-Exchange-Transport-CrossTenantHeadersStamped: OS3P286MB1616
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi, Simon:

     Thanks for looking into this. by looking into the compiled 
instructions, i am convinced the patch is logically correct but not help 
in performance, and it adds complexity,

Below function is actually optimized to one movq instruction with no 
special -O settings on my centos8, which seems good enough for me

static void clear_vlan(struct sw_flow_key *key)
{
     key->eth.vlan.tci = 0;
     key->eth.vlan.tpid = 0;
     key->eth.cvlan.tci = 0;
     key->eth.cvlan.tpid = 0;
}

optimized to movq   $0x0,0x158(%rbx)

I am withdrawing this patch

About your comment below, since it is after the parse_vlan handling, the 
code looks fine and does not affect the logic of the patch

 >>I think you missed the following case further down: >>>>if 
(unlikely(key->eth.type == htons(0))) Have a great day

eddy