| Message ID | 20220520011538.1098888-11-vinicius.gomes@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ethtool: Add support for frame preemption | expand |
Hi Vinicius,
I love your patch! Perhaps something to improve:
[auto build test WARNING on net-next/master]
url: https://github.com/intel-lab-lkp/linux/commits/Vinicius-Costa-Gomes/ethtool-Add-support-for-frame-preemption/20220520-092800
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git df98714e432abf5cbdac3e4c1a13f94c65ddb8d3
config: s390-buildonly-randconfig-r002-20220519 (https://download.01.org/0day-ci/archive/20220520/202205201422.84XYwlpY-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project e00cbbec06c08dc616a0d52a20f678b8fbd4e304)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install s390 cross compiling tool for clang build
# apt-get install binutils-s390x-linux-gnu
# https://github.com/intel-lab-lkp/linux/commit/a42e940bc53c40ee4e33a1bbf022a663bb28a9c7
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Vinicius-Costa-Gomes/ethtool-Add-support-for-frame-preemption/20220520-092800
git checkout a42e940bc53c40ee4e33a1bbf022a663bb28a9c7
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=s390 SHELL=/bin/bash drivers/net/ethernet/intel/igc/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
In file included from drivers/net/ethernet/intel/igc/igc_main.c:6:
In file included from include/linux/if_vlan.h:10:
In file included from include/linux/netdevice.h:38:
In file included from include/net/net_namespace.h:40:
In file included from include/linux/skbuff.h:31:
In file included from include/linux/dma-mapping.h:10:
In file included from include/linux/scatterlist.h:9:
In file included from arch/s390/include/asm/io.h:75:
include/asm-generic/io.h:464:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __raw_readb(PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:477:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:37:59: note: expanded from macro '__le16_to_cpu'
#define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
^
include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16'
#define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
^
In file included from drivers/net/ethernet/intel/igc/igc_main.c:6:
In file included from include/linux/if_vlan.h:10:
In file included from include/linux/netdevice.h:38:
In file included from include/net/net_namespace.h:40:
In file included from include/linux/skbuff.h:31:
In file included from include/linux/dma-mapping.h:10:
In file included from include/linux/scatterlist.h:9:
In file included from arch/s390/include/asm/io.h:75:
include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:35:59: note: expanded from macro '__le32_to_cpu'
#define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
^
include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32'
#define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
^
In file included from drivers/net/ethernet/intel/igc/igc_main.c:6:
In file included from include/linux/if_vlan.h:10:
In file included from include/linux/netdevice.h:38:
In file included from include/net/net_namespace.h:40:
In file included from include/linux/skbuff.h:31:
In file included from include/linux/dma-mapping.h:10:
In file included from include/linux/scatterlist.h:9:
In file included from arch/s390/include/asm/io.h:75:
include/asm-generic/io.h:501:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writeb(value, PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:511:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:521:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:609:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsb(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:617:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsw(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:625:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsl(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:634:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesb(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:643:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesw(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:652:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesl(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
>> drivers/net/ethernet/intel/igc/igc_main.c:5919:6: warning: variable 'ring' is uninitialized when used here [-Wuninitialized]
if (ring->preemptible) {
^~~~
drivers/net/ethernet/intel/igc/igc_main.c:5914:23: note: initialize the variable 'ring' to silence this warning
struct igc_ring *ring;
^
= NULL
13 warnings generated.
vim +/ring +5919 drivers/net/ethernet/intel/igc/igc_main.c
5910
5911 static int igc_save_launchtime_params(struct igc_adapter *adapter, int queue,
5912 bool enable)
5913 {
5914 struct igc_ring *ring;
5915
5916 if (queue < 0 || queue >= adapter->num_tx_queues)
5917 return -EINVAL;
5918
> 5919 if (ring->preemptible) {
5920 netdev_err(adapter->netdev, "Cannot enable LaunchTime on a preemptible queue\n");
5921 return -EINVAL;
5922 }
5923
5924 ring = adapter->tx_ring[queue];
5925 ring->launchtime_enable = enable;
5926
5927 return 0;
5928 }
5929
On Thu, May 19, 2022 at 06:15:37PM -0700, Vinicius Costa Gomes wrote: > Frame Preemption and LaunchTime cannot be enabled on the same queue. > If that situation happens, emit an error to the user, and log the > error. > > Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com> > --- > drivers/net/ethernet/intel/igc/igc_main.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c > index 69e96e9a3ec8..96ad00e33f4b 100644 > --- a/drivers/net/ethernet/intel/igc/igc_main.c > +++ b/drivers/net/ethernet/intel/igc/igc_main.c > @@ -5916,6 +5916,11 @@ static int igc_save_launchtime_params(struct igc_adapter *adapter, int queue, > if (queue < 0 || queue >= adapter->num_tx_queues) > return -EINVAL; > > + if (ring->preemptible) { > + netdev_err(adapter->netdev, "Cannot enable LaunchTime on a preemptible queue\n"); > + return -EINVAL; > + } > + > ring = adapter->tx_ring[queue]; > ring->launchtime_enable = enable; > > -- > 2.35.3 > I'm kind of concerned about this. I was thinking of adapting some scripts I had into some functional kselftests for frame preemption. I am sending 2 streams, one preemptable and one express. With SO_TXTIME I am controlling their scheduled TX times, and I am forcing collisions in the MAC merge layer by making the express packet have a scheduled TX time equal to the preemptable packet's scheduled TX time, then I gradually increase the express packet's scheduled time by small amounts (8 ns or so). I take hardware TX timestamps of both packets and I plot when the express packet is actually sent by the MAC. That is, I measure how long it takes for the MAC to preempt and to reschedule the express packet. My point is, if the LaunchTime feature cannot be enabled on preemptable queues, how can I know that the igc does something functionally valid with preemptable packets on TX, other than to reassemble the mPackets on RX? Otherwise, if there isn't any other disagreement on the UAPI, would you mind posting the iproute2 patch as well, so we could work in parallel (me on enetc + the selftest) until net-next reopens? I'd like to write a selftest that covers your hardware as well, but then again, not sure how to cover it. Do you have any sort of counters from the list in clause 30.14 Management for MAC Merge Sublayer? I see that structured ethtool counters are becoming more popular, see struct ethtool_eth_mac_stats for example.
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c index 69e96e9a3ec8..96ad00e33f4b 100644 --- a/drivers/net/ethernet/intel/igc/igc_main.c +++ b/drivers/net/ethernet/intel/igc/igc_main.c @@ -5916,6 +5916,11 @@ static int igc_save_launchtime_params(struct igc_adapter *adapter, int queue, if (queue < 0 || queue >= adapter->num_tx_queues) return -EINVAL; + if (ring->preemptible) { + netdev_err(adapter->netdev, "Cannot enable LaunchTime on a preemptible queue\n"); + return -EINVAL; + } + ring = adapter->tx_ring[queue]; ring->launchtime_enable = enable;
Frame Preemption and LaunchTime cannot be enabled on the same queue. If that situation happens, emit an error to the user, and log the error. Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com> --- drivers/net/ethernet/intel/igc/igc_main.c | 5 +++++ 1 file changed, 5 insertions(+)