From mboxrd@z Thu Jan 1 00:00:00 1970 From: Saeed Mahameed Subject: Re: [PATCH net-next v3 06/10] net/mlx5e: Change Mellanox references in DIM code Date: Mon, 8 Jan 2018 23:09:00 -0800 Message-ID: References: <1515478402-6048-1-git-send-email-andy@greyhouse.net> <1515478402-6048-7-git-send-email-andy@greyhouse.net> <47ddff01-23f9-5364-0dd4-b7c921512812@mellanox.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Cc: mchan@broadcom.com, talgi@mellanox.com, ogerlitz@mellanox.com, Andy Gospodarek To: Andy Gospodarek , netdev@vger.kernel.org Return-path: Received: from mail-he1eur01on0065.outbound.protection.outlook.com ([104.47.0.65]:52928 "EHLO EUR01-HE1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1754612AbeAIHJN (ORCPT ); Tue, 9 Jan 2018 02:09:13 -0500 In-Reply-To: <47ddff01-23f9-5364-0dd4-b7c921512812@mellanox.com> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 01/08/2018 11:06 PM, Saeed Mahameed wrote: > > > On 01/08/2018 10:13 PM, Andy Gospodarek wrote: >> From: Andy Gospodarek >> >> Change all appropriate mlx5_am* and MLX5_AM* references to net_dim and >> NET_DIM, respectively, in code that handles dynamic interrupt >> moderation.  Also change all references from 'am' to 'dim' when used as >> local variables and add generic profile references. >> >> Signed-off-by: Andy Gospodarek >> Acked-by: Tal Gilboa >> Acked-by: Saeed Mahameed >> --- >>   drivers/net/ethernet/mellanox/mlx5/core/en.h       |   9 +- >>   drivers/net/ethernet/mellanox/mlx5/core/en_dim.c   |  14 +- >>   .../net/ethernet/mellanox/mlx5/core/en_ethtool.c   |   6 +- >>   drivers/net/ethernet/mellanox/mlx5/core/en_main.c  |  40 ++- >>   drivers/net/ethernet/mellanox/mlx5/core/en_txrx.c  |   8 +- >>   drivers/net/ethernet/mellanox/mlx5/core/net_dim.c  | 286 >> ++++++++++----------- >>   drivers/net/ethernet/mellanox/mlx5/core/net_dim.h  |  63 ++--- >>   7 files changed, 225 insertions(+), 201 deletions(-) >> > > [...] > >>   #define IS_SIGNIFICANT_DIFF(val, ref) \ >>       (((100 * abs((val) - (ref))) / (ref)) > 10) /* more than 10% >> difference */ >> -static int mlx5e_am_stats_compare(struct mlx5e_rx_am_stats *curr, >> -                  struct mlx5e_rx_am_stats *prev) >> +static int net_dim_stats_compare(struct net_dim_stats *curr, >> +                 struct net_dim_stats *prev) >>   { >>       if (!prev->bpms) >> -        return curr->bpms ? MLX5E_AM_STATS_BETTER : >> -                    MLX5E_AM_STATS_SAME; >> +        return curr->bpms ? NET_DIM_STATS_BETTER : >> +                    NET_DIM_STATS_SAME; >>       if (IS_SIGNIFICANT_DIFF(curr->bpms, prev->bpms)) >> -        return (curr->bpms > prev->bpms) ? MLX5E_AM_STATS_BETTER : >> -                           MLX5E_AM_STATS_WORSE; >> +        return (curr->bpms > prev->bpms) ? NET_DIM_STATS_BETTER : >> +                           NET_DIM_STATS_WORSE; > > Hey Andy, > > I am currently reviewing a patch internally that fixes a bug in this > area, prev->ppms can be 0 and could cause IS_SIGNIFICANT_DIFF ouch ! I meant cause division by 0 in "IS_SIGNIFICANT_DIFF" > same goes for prev->eppm, for some reason we had a broken assumption > that if ppms is 0 for some reason then the bpms is 0 and the above > condition will cover us. > > Anyway the patch will go to net, which means when this series gets > accepted then net-next will fail to merge with net and we need to > manually push the fix to the new DIM library. > > But for now I don't think anything is required for this series other > than bringing this division by 0 issue and the future merge conflict to > your attention. > >>       if (IS_SIGNIFICANT_DIFF(curr->ppms, prev->ppms)) >> -        return (curr->ppms > prev->ppms) ? MLX5E_AM_STATS_BETTER : >> -                           MLX5E_AM_STATS_WORSE; >> +        return (curr->ppms > prev->ppms) ? NET_DIM_STATS_BETTER : >> +                           NET_DIM_STATS_WORSE; >>       if (IS_SIGNIFICANT_DIFF(curr->epms, prev->epms)) >> -        return (curr->epms < prev->epms) ? MLX5E_AM_STATS_BETTER : >> -                           MLX5E_AM_STATS_WORSE; >> +        return (curr->epms < prev->epms) ? NET_DIM_STATS_BETTER : >> +                           NET_DIM_STATS_WORSE; >> -    return MLX5E_AM_STATS_SAME; >> +    return NET_DIM_STATS_SAME; >>   }