From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id D00B8DE5; Thu, 2 Feb 2017 11:43:22 +0100 (CET) Received: from orsmga005.jf.intel.com ([10.7.209.41]) by orsmga101.jf.intel.com with ESMTP; 02 Feb 2017 02:43:21 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,324,1477983600"; d="scan'208";a="60004505" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.237.220.38]) ([10.237.220.38]) by orsmga005.jf.intel.com with ESMTP; 02 Feb 2017 02:43:20 -0800 To: Adrien Mazarguil References: <1485348178-43771-1-git-send-email-shahafs@mellanox.com> <1485863129-6326-1-git-send-email-shahafs@mellanox.com> <20170201090711.GP10133@6wind.com> <64089318-6d12-28b4-61e8-6e52785ce692@intel.com> <20170201125758.GS10133@6wind.com> <963be495-c009-2937-5a14-204ae3a3b245@intel.com> <20170202084534.GT10133@6wind.com> <1c2be312-c11e-62ab-4a36-d91f25520ab2@intel.com> <20170202103740.GU10133@6wind.com> Cc: Shahaf Shuler , =?UTF-8?Q?N=c3=a9lio_Laranjeiro?= , "dev@dpdk.org" , "stable@dpdk.org" From: Ferruh Yigit Message-ID: <38cc5272-fbe9-d53e-0012-0f9c4d3dda05@intel.com> Date: Thu, 2 Feb 2017 10:43:19 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 MIME-Version: 1.0 In-Reply-To: <20170202103740.GU10133@6wind.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [dpdk-stable] [PATCH v2] net/mlx5: fix link status query X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 02 Feb 2017 10:43:23 -0000 On 2/2/2017 10:37 AM, Adrien Mazarguil wrote: > On Thu, Feb 02, 2017 at 10:15:08AM +0000, Ferruh Yigit wrote: >> On 2/2/2017 8:45 AM, Adrien Mazarguil wrote: >>> On Wed, Feb 01, 2017 at 06:11:17PM +0000, Ferruh Yigit wrote: >>>> On 2/1/2017 12:57 PM, Adrien Mazarguil wrote: >>>>> On Wed, Feb 01, 2017 at 11:13:59AM +0000, Ferruh Yigit wrote: >>>>>> On 2/1/2017 9:07 AM, Adrien Mazarguil wrote: >>>>>>> On Wed, Feb 01, 2017 at 06:53:55AM +0000, Shahaf Shuler wrote: >>>>>>>> : Tuesday, January 31, 2017 6:17 PM, Ferruh Yigit: >>>>>>>>> On 1/31/2017 11:45 AM, Shahaf Shuler wrote: >>>>>>>>>> Trying to query the link status through new kernel ioctl API >>>>>>>>>> ETHTOOL_GLINKSETTINGS was always failing due to kernel bug. >>>>>>>>>> The bug was fixed on version 4.9 >>>>>>>>>> this patch uses the legacy ioctl API for lower kernels. >>>>>>>>>> >>>>>>>>>> Fixes: 188408719888 ("net/mlx5: fix support for newer link speeds") >>>>>>>>>> CC: stable@dpdk.org >>>>>>>>>> >>>>>>>>>> Signed-off-by: Shahaf Shuler >>>>>>>>>> --- >>>>>>>>> >>>>>>>>> <...> >>>>>>>>> >>>>>>>>>> @@ -707,7 +708,7 @@ struct priv * >>>>>>>>>> static int >>>>>>>>>> mlx5_link_update_unlocked_gs(struct rte_eth_dev *dev, int >>>>>>>>>> wait_to_complete) { -#ifdef ETHTOOL_GLINKSETTINGS >>>>>>>>>> +#if KERNEL_VERSION(4, 9, 0) <= LINUX_VERSION_CODE >>>>>>>>> >>>>>>>>> Mostly it is not good idea to do kernel version check in the .c file. >>>>>>>>> >>>>>>>>> It is possible to move this comparison to the .h file, and set a feature >>>>>>>>> macro based on comparison result, like HAVE_ETHTOOL_GLINKSETTINGS, >>>>>>>>> and >>>>>>>>> use this macro in the .c file. >>>>>>>>> >>>>>>>>> This makes .c code easier to understand. And the abstraction in the >>>>>>>>> header file lets you update the comparison in the future without >>>>>>>>> changing the code itself. >>>>>>>>> >>>>>>>>> But it is your call, do you prefer to continue with this one? >>>>>>>> >>>>>>>> This is a good suggestion. >>>>>>>> Adrien, Nélio what do you think? >>>>>>> >>>>>>> Let's include this patch as-is. Doing so in a header file such as mlx5.h >>>>>>> would require including linux/version.h from that file and cause the entire >>>>>>> PMD to be even more OS-dependent. >>>>>>> >>>>>>> We'll move this check elsewhere in the future if we need several such >>>>>>> workarounds, thanks. >>>>>> >>>>>> OK. >>>>>> >>>>>> One more thing, comment log says: >>>>>> "The bug was fixed on version 4.9" >>>>>> >>>>>> And code does: >>>>>> "+#if KERNEL_VERSION(4, 9, 0) <= LINUX_VERSION_CODE" >>>>>> >>>>>> If the bug is fixed in 4.9, should check be "<" instead of "<=" >>>>> >>>>> I'll concede the argument order used in this condition is somewhat unusual >>>>> but it actually ends up being the same as: >>>>> >>>>> #if LINUX_VERSION_CODE > KERNEL_VERSION(4, 9, 0) >>>> >>>> I don't think they are same, unless I am missing something obvious. >>>> >>>> "+#if KERNEL_VERSION(4, 9, 0) <= LINUX_VERSION_CODE" >>>> vs >>>> "#if LINUX_VERSION_CODE > KERNEL_VERSION(4, 9, 0)" >>>> >>>> Even if you change the argument order, one covers 4.9 release other not. >>> >>> All right, turns out we're both wrong because it should have been >= anyway >>> regardless of the order as you pointed out. The block must appear when >>> kernel version is higher or equal to 4.9.0. >> >> I see now, my logic was wrong, and I can see what are you trying to >> explain by changing argument order, thanks ... >> >>> >>> Now I think there are a couple of additional issues with this patch that >>> require a revert and rework: >>> >>> - Kernel headers do not necessarily match the running version. What if this >>> code is compiled against a 4.10.0 source tree and the running kernel is >>> 4.5? How about the reverse? >> >> It is possible to use proper kernel headers via RTE_KERNELDIR >> environment variable. >> >> But without this patch, /usr/include/linux/ethtool.h used directly in >> Makefile, which has same problem you mentioned. > > Right, what I'm getting at is this code is supposed to work regardless of > the running kernel version (uname -r), userland headers version > (/usr/include/linux) and source tree version (/usr/src/linux/), all of them > may differ. Basically this PMD can be built on a system and run on another > since it's not a kernel module. In short the following scenarios should be > handled: > > - Kernel (uname -r) is 3.2, /usr/include/linux comes from 4.5, and > /usr/src/linux comes from 4.10. > > - Kernel (uname -r) is 4.10, /usr/include/linux comes from 3.2 and > /usr/src/linux does not even exist. > > - Kernel (uname -r) is 4.8, /usr/include/linux comes from 4.9. > > - Kernel (uname -r) is 4.9, /usr/include/linux comes from 4.9 (when one gets > lucky enough). > >>> - By removing the check on ETHTOOL_GLINKSETTINGS, this code breaks >>> compilation for Linux kernel version < 4.5, which I think is a problem. >> >> Please help me understand. >> >> ETHTOOL_GLINKSETTINGS defined for kernels > 4.5, and this patch replaces >> "#ifdef ETHTOOL_GLINKSETTINGS" >> with >> "#if KERNEL_VERSION(4, 9, 0) <= LINUX_VERSION_CODE" >> >> So, kernel version <= 4.5 part should be same, why it is giving a >> compile error? > > You're right, ETHTOOL_GLINKSETTINGS should be consistent with > LINUX_VERSION_CODE since both includes come from the same > directory. We normally don't need to manage the case where a system has > broken includes, however as you're pointing out below distributions > sometimes make backports and the kernel version means nothing. Relying on > ETHTOOL_GLINKSETTINGS's presence makes more sense. > >>> So I believe this patch should be re-worked to maintain a check on >>> ETHTOOL_GLINKSETTINGS (or define it then not present) and perform an >>> additional version check at runtime to use the most appropriate ioctl API. >> >> Overall, it would be better if you can find a way without dealing kernel >> versions, which causes lots of maintenance work. > > My thought as well. > >> Also another issue we are experiencing while maintaining KNI is >> backported kernels, they tend to break version based checks. So there >> are distro based checks combined with version checks, I think this >> wouldn't be something you want J > > Yeah, especially since it's not a kernel module, a PMD must perform a > runtime version check. > >>> Ferruh, please revert. >> >> >> But original patch looks good, I will convert it to initial patch, and >> keep it until above two items clarified. > > I've talked to Shahaf/Nelio, they already intend to send an updated version > due to the above concerns. Do you prefer a fix on top of it? No, if there will be a new version, I will drop the existing one in the repo. > >>> stable@dpdk.org, please ignore this commit for the the time being, and sorry >>> for the noise. >>> >>>>> Which is the correct intent. I guess you can update this line for clarity if >>>>> you think it's necessary. >>>> >>>> If the intention is as following, I can fix it while applying: >>>> #if KERNEL_VERSION(4, 9, 0) < LINUX_VERSION_CODE >>>> >>>>> >>>>>>> >>>>>>>>>> struct priv *priv = mlx5_get_priv(dev); >>>>>>>>>> struct ethtool_link_settings edata = { >>>>>>>>>> .cmd = ETHTOOL_GLINKSETTINGS, >>>>>>>>> <...> >>>>>>> >>>>>> >>>>> >>>> >>> >> >