From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f51.google.com (mail-wm0-f51.google.com [74.125.82.51]) by dpdk.org (Postfix) with ESMTP id AA7BAB6D for ; Thu, 2 Feb 2017 11:37:48 +0100 (CET) Received: by mail-wm0-f51.google.com with SMTP id r141so82582800wmg.1 for ; Thu, 02 Feb 2017 02:37:48 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to; bh=yfgFibSm8JpWvLIpYXQtJCkd5BHC2Y0lmOqxGsmm7IA=; b=xHVXpydsLA1uWy38fsznIY+FisVCxlr6Z4weBRJwrD6ikTTSIn1BtxcFD58xvhhIOj K+kjcnJarzuhTR4JvubdiwPv/IyOkjAEp3TK/3cI/CYA0f1doz3xh8jTMH1Qw0qF7UP5 j1ngUtzVLYqqgsYSYZbK4omm8H0UVEFvHiKXKpfcwpynflatGMRCHKUSiulDCnqLVO0D XETiNW6R0HzsmD4xspzDkdCNHIOVVtXMWdPLo3jrD1GxHMxGnITC4++MZqmVGwTt+Mj3 B3JGTotupFZbjw/M9n1sVvCntjxLPFlF8E2GgsIu20Ed64QJv2w3k/POva94Vc86J52I we+Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to; bh=yfgFibSm8JpWvLIpYXQtJCkd5BHC2Y0lmOqxGsmm7IA=; b=sOGxZww3EVUsm6yybKZtHgwly1Y37v5aDhv5v6b/PEXQt79Wjl4dFd3P+4Q7Fdowb6 jG9RQ0Ve6ioyIoMBDiLV4uNuUubrLK7A4cWhtS2XL4hxzOTGu2ADJRcBtNxh/BFdEZ1b 5gys+3oEcvBJQVE3T4zy8FwX0fdtjZSc1mE0+GYrOyS6+1Xn/ukl06/jUEs8foVDfaj5 AALpWoXupfyyhBWDwngXmNDPLU551xJ1zPCMAoXjO50OTqDDC67vY/bb5DyF34sMbkwr l1stVTfpqffcjeXxtil29RnD4u4S7fL+W4wG5TyzSUwKpHmiXosojP0b4NFcoGP1URCz 3KXA== X-Gm-Message-State: AIkVDXK5t8l8KbY2jIX4i5TtycrH6rHB8XaMG+l93kmMOnMyAtmhYudulJ4cXOeanm9cwEYD X-Received: by 10.223.132.166 with SMTP id 35mr6800015wrg.122.1486031868340; Thu, 02 Feb 2017 02:37:48 -0800 (PST) Received: from 6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id j18sm39136151wrb.33.2017.02.02.02.37.47 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 02 Feb 2017 02:37:47 -0800 (PST) Date: Thu, 2 Feb 2017 11:37:40 +0100 From: Adrien Mazarguil To: Ferruh Yigit Cc: Shahaf Shuler , =?utf-8?B?TsOpbGlv?= Laranjeiro , "dev@dpdk.org" , "stable@dpdk.org" Message-ID: <20170202103740.GU10133@6wind.com> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1c2be312-c11e-62ab-4a36-d91f25520ab2@intel.com> 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:37:48 -0000 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? > > 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, > >>>>>>> <...> > >>>>> > >>>> > >>> > >> > > > -- Adrien Mazarguil 6WIND