From mboxrd@z Thu Jan 1 00:00:00 1970 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965184AbeALVCX (ORCPT + 1 other); Fri, 12 Jan 2018 16:02:23 -0500 Received: from mail-ve1eur01on0045.outbound.protection.outlook.com ([104.47.1.45]:30656 "EHLO EUR01-VE1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S964844AbeALVCV (ORCPT ); Fri, 12 Jan 2018 16:02:21 -0500 Authentication-Results: spf=none (sender IP is ) smtp.mailfrom=saeedm@mellanox.com; Subject: Re: [PATCH] net/mlx4_en: ensure rx_desc updating reaches HW before prod db updating To: Eric Dumazet , Jason Gunthorpe , Jianchao Wang Cc: tariqt@mellanox.com, junxiao.bi@oracle.com, netdev@vger.kernel.org, linux-rdma@vger.kernel.org, linux-kernel@vger.kernel.org References: <1515728542-3060-1-git-send-email-jianchao.w.wang@oracle.com> <20180112163247.GB15974@ziepe.ca> <1515775567.131759.42.camel@gmail.com> <85116e56-52b1-944d-6ee2-916ccfc3a7a6@mellanox.com> <1515788191.131759.48.camel@gmail.com> From: Saeed Mahameed Message-ID: Date: Fri, 12 Jan 2018 13:01:56 -0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.0 MIME-Version: 1.0 In-Reply-To: <1515788191.131759.48.camel@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Originating-IP: [209.116.155.178] X-ClientProxiedBy: CY4PR1101CA0017.namprd11.prod.outlook.com (2603:10b6:910:15::27) To DB4PR05MB0864.eurprd05.prod.outlook.com (2a01:111:e400:985f::22) X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: 4e02f4b4-26d6-4564-be8a-08d559ffc215 X-MS-Office365-Filtering-HT: Tenant X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:(7020087)(4652020)(5600026)(4604075)(48565401081)(2017052603307)(7153060)(7193020);SRVR:DB4PR05MB0864; X-Microsoft-Exchange-Diagnostics: 1;DB4PR05MB0864;3:n0W5aw/aiaPelUYyWdQYmeWbEQZDSwTJuAeXGmf5Nvl0vTeMLytZ+DJ1XMv5iXk/NIXwAuuHAou5iSCN/00tYduCQi+hXYVy77DLYqlWQqa9LVcC/Maz6IxwukdJK2IeHkPisERiSuqpcQbPD3Fp9aAIYyKtEdIaYIal002vo0EMzksrPFMcQFK+v7yOEtcXboZhzJjEM3Q72+uYL8jdDXrCV39ABm4VfmW+9RHKlRYiPK1HNxFnUQOHEbtGEKSk;25:b0KkR1zVMCoDf+lCj0Irgx56jn69WvvhomTlrI3MHahR12jDSLJzPOje/0iMPKOMoTJF9gtslZHT4P+/guGtYQHQcbyIgrms+W0OTinN8lBjp1EeR1/JT4YnWv7PMUJxusJIYmfeqk/Ygwu+hCCARQnbLlxwh4GcRFkRapQ4/9JwO0Wt9I7UkNzZh+3cu952MeFZhs9kSK8YN/jnBvf8V3c2dE49GbjjvPfBaKgimqY1Rk6uHH1yg78/9DtaNZCsOA7TjBedZq7mcIc4eaMPgtWRNsqIKfBWmusgckymzOKVS1zaFwYp6ei/B5mMPZ5SNrmdSfFuTTdVfhHiTGQfkg==;31:2yMmnzQ1nBuWNOaM/DWXdEuYxsnZFd4VL0jVa9ArHzAIwvCc7a8vJ75cMDC2AYL/Y4LYy9yrEe/MsKKuDAfzzWwA69lvvNSDUCt0UhVk4UoEcuOTK5CIeFt7o9cFggRI5/CWE79jGdfK90yjhoIjEaujSQvIgPvB7Y1Ta1eH9sAJQiGHCVJaxZS67xCbmaZm24Pz0p9RXxPPH/F/6rnWnjeUJJYcwYGuIyn1fj/IQ4Y= X-MS-TrafficTypeDiagnostic: DB4PR05MB0864: X-Microsoft-Exchange-Diagnostics: 1;DB4PR05MB0864;20:pCr9CRRf7XTeXvrXHM3I+Ut4ulklwqU35mAhp9NE5FHimcse1xaPdVwIQUQHNLKOiYs9ds2fRwYGDLdLVOQEnWvZQYOP8sg3haIPXEK8wNHFBeVXewrE2RXxir460o3BhapIsPIn+aH3Ncu6wxbGvfe7odYeOTiA7olpo01MeM1pnPMM70A8lXKdDNV9zkKIk7popSDLSO1PLTWBEklkAYqyzSXJ/XiAaETfbMau2rqK/AmBp3555rWSoZU1BbsAeLNlr2RjfpY3cR7C6vJsBZI2BcsizV3kH6gZRiV88t+mpmNw3hEjdJpLTajQvDOzOpftlspxlYAfyu7H3SmEUadKBRfzBqqdB37dED7Siyt0M8KVbd6uUMZlWoc1nxXnffifLCUA9Ltf3OqR9kRzfXDfCi8hJrmi5GilNfMkLFW0iEPyAs+vFToAbZ+rlyp/jfzPg54B/hIy9mRt12Bil9AbS4v54QF5EEZTqTqW5akc4olarvTxagP316ed9u7A;4:kW+JkqywzFZ4sxKtjQ6Lpjx7NJY16fOkK1koTXlRWJvavbY+p+7MRQc4twboHcyMEPalRjM6RYq9d72hCJ2ZQOBW8kFohnFkaa47t5tb/spomdodyPYNy/FujKeelMlW8WumgH1HQtQ+lqiEeHSyL3n+zSDdJCjZ1g9XQiICR9uURnQygelYjuNc+osCwleARw+qT+NymbkrcQVIYrQ+1/DHfW/Eaj6QfWJx+nuDdJk7Mjo16OB2vh0SZHhF2n1XIfdD4d8fK++xiItbKfYzq8mcIUXGYV2fDZDjYXXFN2rVeYvjLpNfjYCyzY7yPYdQ X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:(146099531331640); X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(6040470)(2401047)(8121501046)(5005006)(10201501046)(93006095)(93001095)(3231023)(944501147)(3002001)(6055026)(6041268)(20161123564045)(20161123562045)(20161123558120)(20161123560045)(201703131423095)(201702281528075)(20161123555045)(201703061421075)(201703061406153)(6072148)(201708071742011);SRVR:DB4PR05MB0864;BCL:0;PCL:0;RULEID:(100000803101)(100110400095);SRVR:DB4PR05MB0864; X-Forefront-PRVS: 0550778858 X-Forefront-Antispam-Report: SFV:NSPM;SFS:(10009020)(6049001)(396003)(39860400002)(39380400002)(376002)(346002)(366004)(189003)(199004)(377424004)(24454002)(7736002)(230700001)(386003)(97736004)(316002)(93886005)(59450400001)(305945005)(229853002)(68736007)(3846002)(6666003)(2906002)(478600001)(2950100002)(117156002)(6486002)(52116002)(23676004)(53546011)(25786009)(52146003)(77096006)(2486003)(6116002)(4326008)(58126008)(5660300001)(53936002)(16526018)(83506002)(110136005)(16576012)(65826007)(76176011)(31696002)(105586002)(66066001)(106356001)(39060400002)(65956001)(86362001)(6246003)(8676002)(81156014)(36756003)(8936002)(31686004)(50466002)(47776003)(65806001)(64126003)(81166006);DIR:OUT;SFP:1101;SCL:1;SRVR:DB4PR05MB0864;H:[192.168.70.195];FPR:;SPF:None;PTR:InfoNoRecords;MX:1;A:1;LANG:en; X-Microsoft-Exchange-Diagnostics: =?utf-8?B?MTtEQjRQUjA1TUIwODY0OzIzOkUvQXZJU3dCZndmY2N1Y04xV3FNVzZ2eWsw?= =?utf-8?B?cy9FNHdhbXVYZysrektPUS9Db3l5WGFmQmhDM3lEVzVhdHo3ZkRpRXJDZlU0?= =?utf-8?B?c1ZvMnlVdkd0L0s5VnU3TzFXazMvV0FwdXhNSGF3bXN0Y0FSTzV5WjlNT3lq?= =?utf-8?B?Um9YUFQxTzQvNUF2SEFUYXIzRGhRUnhRSWFnR2p4eGUwY1RSbzA2MTZvNWpN?= =?utf-8?B?SnF0MDlNMEVyS2hSV1p3d2I2d0F0a3g3MEhwM0hZbmRMV3VJOW1YK2lQQWQx?= =?utf-8?B?REtKaVpBdWVieWxZL3k3NVl1alhSY1BNTUxGRFQyZTdXVEJWZ3hIV1l3THBk?= =?utf-8?B?amZ4ZGlwN1BNeEIzZDQ2SWJSNFIyVmdIMnI2aTNkRDgwUUU4S2k5REwySHZH?= =?utf-8?B?cnhYc1BhM0RMcVl6Y0lKdDN1YU5mdEc3QlRRTXdaamc4YXJseElaSU1wbVJX?= =?utf-8?B?RXR1ZjFvN2tDVTYwRCs4SVN4bzNGdGVRREx6eWg5Z3dIWnJXWDE5WTUzdnBJ?= =?utf-8?B?eDlPZkRwTE1vTVVWWjBFcU5jR3YrQVhENGdSenhzbEZFVHVQTHE4N2d6U2M2?= =?utf-8?B?cGFpcmQzODFObk5NOGhLUnZJeHU5b01MdkVBMGtaZ1ljRUNwT2Q1RTdEYmR1?= =?utf-8?B?dlAvaHZrVTFXbmdNQVpZQW92dWpnTFVQNkxVY1pUMTR0QWE5cUl4SjlWbEpq?= =?utf-8?B?bzBQUUFVcHdUMU1XTHh5cWlVamMrNHZ5TSt2UllRV2piZkNKczRORVFSTXFj?= =?utf-8?B?QlowbDlWYkxrQVhPL01ITDZnL2xMWWZWTXpLdEpSM2RkbHY1RkxhVEdUTlNT?= =?utf-8?B?aDhaY2dLSUkrYVZmb3p1NDB2S0Y2L1g1NE1WbGhORzd3TEplNmRWYXEyRm9W?= =?utf-8?B?WWJoVm5WUXFsMkI3ZFJCR3hxZ1d3T25QOHdRNEhDYnN2OGo1TkhXZENtNjZr?= =?utf-8?B?NUdibENtcXZUcmhXZGN3QVExdkFreG96bmZwdjE0dkNzbUI1YUZ5R211a1Zk?= =?utf-8?B?RGxteDBLRjRIcEhsYzhoYmNQY1cwNFRwaDVmNm5uQ2dZZmRDZ01xTU5KSkhn?= =?utf-8?B?NEM1VlAra0huNTZrSnhhcHVIemZnNGRYNEFnS3RQazdONGd0NFFkQndzRUdk?= =?utf-8?B?TGlCUHZMaTJGdkVwWkxFQ2xYaUo5N2lCWDh2eUVNQlhXb0JtVUhkeXMyOEJw?= =?utf-8?B?T3RqY3VZUitubWhrZzdweG1oS2htZC9QVlNiRE9MN09zbUZBY1BDcllrM3NX?= =?utf-8?B?d21ja1FUQVFGeWd2MjBOK1QzMkpQVWtaTkcyNWlqWm5PRU9QNHZPVnF4OFps?= =?utf-8?B?VjloTll4L3hadEd1QmhoRmpVK2JyV01BV3BnTk1SWXh6aDlHVWFDNGtGRm81?= =?utf-8?B?MGNOakUxNGRtOTdjT1lwM2o5cmNXOUJNYXp0WGZEL29ySUdSdXM5UVNjbEl6?= =?utf-8?B?OEZHNERSUkNyRldyRGg5WlZUU2RrYmJnQUx1SmkwSlFDeURZUlVCUlh6UGk2?= =?utf-8?B?Ti9RRURBb24wTXp6UHQ4WHFBcWYwRER0RmlnNmRvYU94MnZtTlAwRkk1bGxk?= =?utf-8?B?QVV6NVl4Skp6WmxFVkhBa3p1bGZKcnVNdDdnRXBWNlFkWXE0NHZRS0dweUxN?= =?utf-8?B?Unc4MWY2WWZhSTBlUEp2L3RRajBhZlJPYmJLOFl4L1dqeldmbHBQMmhMRGVY?= =?utf-8?B?MW13WWFaMjBrV3RhNVVHTmVpdXBvUFI0a1dNVkYxTnJ0V0s5Z05OWFdkb2Zy?= =?utf-8?B?Q0FRaVpPcXMvZERZQ2xyUHNiNlgyN2d2cDMxVDh2VUJlajB0UVZDNG9yMU8r?= =?utf-8?B?R3g5diswRmZJdDZlSTdxMWNvUFd5UXBuMEZSUEx0OHV1eFhVQUtlckRTNkwx?= =?utf-8?B?QVFTTWpwejVEQnEyUTByT3NBNGxBaGJMRXBHQTl2MThBaVhOZHhYTVVDcmRi?= =?utf-8?B?UHBhKzF5R28wNEtlRHc0VWNLT0pZTUpMMHh3NVhRN0ZWUW9uM3V1OTZsMUdz?= =?utf-8?Q?PNFeWL?= X-Microsoft-Exchange-Diagnostics: 1;DB4PR05MB0864;6:q65v3N14ouBWxNNmSdsFwY4o8hq/rp4i3WqsObxqCJ+FSOOCjx3q5FdMq3RBeakDGv3Ce4QUQBdOs1J1OXn4GMHdUzinw+B1sid8c0sA4kOr/nHQUiYEYOgmICcQaeuBDMbMmo5HE0akV3e3u5GUHwT9cn1HzAe61OC2po6eptMk1pg4GS0uQ4FbG+6eRKz34koXalEaYoBjROERCZWKpoCwL6PlUzUYeljy/Dz7NP9+eLUcaVWwxwPEnyb8/VX/j1jqCbxbfn1+i6usa3UjKuDNwLvVpH0/vb7BwN8ne0Iw3eqMvYmhmHNmqJpGoVhH8gikOSktcpSxE74JR9cOpT1xo0/MAKbBMsZm4iA4t7k=;5:Y2uNe4Y2LndNS99mDPNcwO1hOc/qa9f+myBU9PVOIhnWeMnrrHFe1J5bx00xKO1B2V54cMT9GcUTXVPpMDOiaI/1QhWgthXOADEjq7+hSMPZ0k3FUzSh1bazjxIkgOiqdmBeRorzZEAUxkqx/0SExJWrX3oxd1L9NPR3i6IweMY=;24:nEUmWiDSpMZUDGqzUDbsWaQALO5lShy7RrW0AQBm2V/8jms9kGyzjxsY31enEneg7TMAovt/okiV+nmsgeV8Mbc6v8DhVXovymLMDjsSER8=;7:8Fa6k35runvo9/1TZE+2DaZJJ35/hPiJ2A5LvgbsVHKUlG04tKOtTCsqCzdjiKc2LTqVp0FHDxuaYPNqTN74tG7qFlQ3hV+jTuUjO/l1PgPmXq5x+DdDXyc1KwuCoULRxvnUIQyH73gp311Qh71jIT+LWTGtbyHEG0sn5kXrAB12PuDvqm4dzG7fEaPaIoKB5o4yjGeOqxZ0QJJXJdyUoLcPRZMAVvOGigb+kWmzt2a/1sz4txPcyurBQTK4TOy9 SpamDiagnosticOutput: 1:99 SpamDiagnosticMetadata: NSPM X-OriginatorOrg: Mellanox.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 Jan 2018 21:02:12.5530 (UTC) X-MS-Exchange-CrossTenant-Network-Message-Id: 4e02f4b4-26d6-4564-be8a-08d559ffc215 X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: a652971c-7d2e-4d9b-a6a4-d149256f461b X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB4PR05MB0864 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: On 01/12/2018 12:16 PM, Eric Dumazet wrote: > On Fri, 2018-01-12 at 11:53 -0800, Saeed Mahameed wrote: >> >> On 01/12/2018 08:46 AM, Eric Dumazet wrote: >>> On Fri, 2018-01-12 at 09:32 -0700, Jason Gunthorpe wrote: >>>> On Fri, Jan 12, 2018 at 11:42:22AM +0800, Jianchao Wang wrote: >>>>> Customer reported memory corruption issue on previous mlx4_en driver >>>>> version where the order-3 pages and multiple page reference counting >>>>> were still used. >>>>> >>>>> Finally, find out one of the root causes is that the HW may see stale >>>>> rx_descs due to prod db updating reaches HW before rx_desc. Especially >>>>> when cross order-3 pages boundary and update a new one, HW may write >>>>> on the pages which may has been freed and allocated again by others. >>>>> >>>>> To fix it, add a wmb between rx_desc and prod db updating to ensure >>>>> the order. Even thougth order-0 and page recycling has been introduced, >>>>> the disorder between rx_desc and prod db still could lead to corruption >>>>> on different inbound packages. >>>>> >>>>> Signed-off-by: Jianchao Wang >>>>> drivers/net/ethernet/mellanox/mlx4/en_rx.c | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_rx.c b/drivers/net/ethernet/mellanox/mlx4/en_rx.c >>>>> index 85e28ef..eefa82c 100644 >>>>> +++ b/drivers/net/ethernet/mellanox/mlx4/en_rx.c >>>>> @@ -555,7 +555,7 @@ static void mlx4_en_refill_rx_buffers(struct mlx4_en_priv *priv, >>>>> break; >>>>> ring->prod++; >>>>> } while (likely(--missing)); >>>>> - >>>>> + wmb(); /* ensure rx_desc updating reaches HW before prod db updating */ >>>>> mlx4_en_update_rx_prod_db(ring); >>>>> } >>>>> >>>> >>>> Does this need to be dma_wmb(), and should it be in >>>> mlx4_en_update_rx_prod_db ? >>>> >>> >>> +1 on dma_wmb() >>> >>> On what architecture bug was observed ? >>> >>> In any case, the barrier should be moved in mlx4_en_update_rx_prod_db() >>> I think. >>> >> >> +1 on dma_wmb(), thanks Eric for reviewing this. >> >> The barrier is also needed elsewhere in the code as well, but I wouldn't >> put it in mlx4_en_update_rx_prod_db(), just to allow batch filling of >> all rx rings and then hit the barrier only once. As a rule of thumb, mem >> barriers are the ring API caller responsibility. >> >> e.g. in mlx4_en_activate_rx_rings(): >> between mlx4_en_fill_rx_buffers(priv); and the loop that updates rx prod >> for all rings ring, the dma_wmb is needed, see below. >> >> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_rx.c >> b/drivers/net/ethernet/mellanox/mlx4/en_rx.c >> index b4d144e67514..65541721a240 100644 >> --- a/drivers/net/ethernet/mellanox/mlx4/en_rx.c >> +++ b/drivers/net/ethernet/mellanox/mlx4/en_rx.c >> @@ -370,6 +370,8 @@ int mlx4_en_activate_rx_rings(struct mlx4_en_priv *priv) >> if (err) >> goto err_buffers; >> >> + dma_wmb(); >> + >> for (ring_ind = 0; ring_ind < priv->rx_ring_num; ring_ind++) { >> ring = priv->rx_ring[ring_ind]; > > > Why bother, considering dma_wmb() is a nop on x86, > simply a compiler barrier. > > Putting it in mlx4_en_update_rx_prod_db() and have no obscure bugs... > Simply putting a memory barrier on the top or the bottom of a functions, means nothing unless you are looking at the whole picture, of all the callers of that function to understand why is it there. which is better to grasp ?: update_doorbell() { dma_wmb(); ring->db = prod; } or fill buffers(); dma_wmb(); update_doorbell(); I simply like the 2nd one since with one look you can understand what this dma_wmb is protecting. Anyway this is truly a nit, Tariq can decide what is better for him :). > Also we might change the existing wmb() in mlx4_en_process_rx_cq() by > dma_wmb(), that would help performance a bit. > > +1, Tariq will you handle ?