We get a build error with the mlx5 driver when the ethernet
support (CONFIG_MLX5_CORE_EN) is disabled:
drivers/net/ethernet/mellanox/mlx5/core/main.c:1320:22: error: 'mlx5_devlink_eswitch_mode_set' undeclared here (not in a function)
drivers/net/ethernet/mellanox/mlx5/core/main.c:1321:22: error: 'mlx5_devlink_eswitch_mode_get' undeclared here (not in a function)
drivers/net/built-in.o:(.rodata+0x25a68): undefined reference to `mlx5_devlink_eswitch_mode_get'
drivers/net/built-in.o:(.rodata+0x25a6c): undefined reference to `mlx5_devlink_eswitch_mode_set'
There are actually two problems here, but they are closely related,
so I'm addressing them both:
- The header is included under an #ifdef, which is usually a bad idea
as it hides the function declarations, so we fail to compile even
if we don't actually use the functions in the end.
- The references to the functions are kept in the object file because
we don't check whether they are built-in or not.
As we don't want to add any useless #ifdef here, this uses an
IS_ENABLED() check to drop the mlx5_devlink_ops structure when we don't
need it, and to skip the register/unregister step.
Signed-off-by: Arnd Bergmann <redacted>
Fixes: f7856daf57b9 ("net/mlx5: Add devlink interface")
---
drivers/net/ethernet/mellanox/mlx5/core/main.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
--
2.9.0
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
The mlx5 driver fails to build on 32-bit architectures after some
references to 64-bit divisions got added:
drivers/net/built-in.o: In function `mlx5e_rx_am':
:(.text+0xf88ac): undefined reference to `__aeabi_ldivmod'
The driver even performs three division here, and it uses the
obsolete 'struct timespec' that we want to get rid of.
Using ktime_t and ktime_us_delta() replaces one of the divisions
and is mildly more efficient, aside from working across 'settimeofday'
calls and being the right type for the y2038 conversion.
Using a u32 instead of s64 to store the number of microseconds
limits the maximum time to about 71 minutes, but if we exceed that
time, we probably don't care about the result any more for the
purpose of rx coalescing.
For the number of packets, we are taking the difference between
two 'unsigned int', so the result won't ever be greater than that
either.
After those changes, the other two divisions are done as 32-bit
arithmetic operations, which are much faster.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Fixes: 3841f0b3493b ("net/mlx5e: Support adaptive RX coalescing")
---
drivers/net/ethernet/mellanox/mlx5/core/en.h | 2 +-
drivers/net/ethernet/mellanox/mlx5/core/en_rx_am.c | 12 ++++++------
2 files changed, 7 insertions(+), 7 deletions(-)
@@ -278,17 +278,17 @@ static void mlx5e_am_calc_stats(struct mlx5e_rx_am_sample *start,structmlx5e_rx_am_sample*end,structmlx5e_rx_am_stats*curr_stats){-structtimespectime=timespec_sub(end->time,start->time);-s64delta_us=timespec_to_ns(&time)/1000;-s64npkts=end->pkt_ctr-start->pkt_ctr;+/* u32 holds up to 71 minutes, should be enough */+u32delta_us=ktime_us_delta(end->time,start->time);+unsignedintnpkts=end->pkt_ctr-start->pkt_ctr;if(!delta_us){WARN_ONCE(true,"mlx5e_am_calc_stats: delta_us=0\n");return;}-curr_stats->ppms=(npkts*1000)/delta_us;-curr_stats->epms=(MLX5E_AM_NEVENTS*1000)/delta_us;+curr_stats->ppms=(npkts*USEC_PER_MSEC)/delta_us;+curr_stats->epms=(MLX5E_AM_NEVENTS*USEC_PER_MSEC)/delta_us;}voidmlx5e_rx_am_work(structwork_struct*work)
On Wed, Jun 15, 2016 at 6:27 PM, Arnd Bergmann [off-list ref] wrote:
We get a build error with the mlx5 driver when the ethernet
support (CONFIG_MLX5_CORE_EN) is disabled:
drivers/net/ethernet/mellanox/mlx5/core/main.c:1320:22: error: 'mlx5_devlink_eswitch_mode_set' undeclared here (not in a function)
drivers/net/ethernet/mellanox/mlx5/core/main.c:1321:22: error: 'mlx5_devlink_eswitch_mode_get' undeclared here (not in a function)
drivers/net/built-in.o:(.rodata+0x25a68): undefined reference to `mlx5_devlink_eswitch_mode_get'
drivers/net/built-in.o:(.rodata+0x25a6c): undefined reference to `mlx5_devlink_eswitch_mode_set'
There are actually two problems here, but they are closely related,
so I'm addressing them both:
- The header is included under an #ifdef, which is usually a bad idea
as it hides the function declarations, so we fail to compile even
if we don't actually use the functions in the end.
- The references to the functions are kept in the object file because
we don't check whether they are built-in or not.
As we don't want to add any useless #ifdef here, this uses an
IS_ENABLED() check to drop the mlx5_devlink_ops structure when we don't
need it, and to skip the register/unregister step.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Fixes: f7856daf57b9 ("net/mlx5: Add devlink interface")
Hi Arnd,
We already took care of those issues, they only apply to Leon's tree
https://git.kernel.org/cgit/linux/kernel/git/leon/linux-rdma.git/,
this tree is meant to maintain MLX5 Shared code between netdev and
linux-rdma trees prior to submission to both trees.
This patch is a non-shared code and it only exists in
https://git.kernel.org/cgit/linux/kernel/git/leon/linux-rdma.git/log/?h=topic/net-next-mlx5.
It is yet to be submitted to Dave's net/net-next tree. later on, this
patch and all the others will go through the normal submission
process.
For the future I don't see any reason to CC the whole netdev, rdma and
kernel folks.
Unless you, Dave and Doug think otherwise.
Thanks
Saeed.
Ok, I see. It would be nice if the process had a way to avoid build regressions
in linux-next, in particular if you already have a fix by the time a patch
that introduces a problem gets added.
Can you check if the fix for the second problem correctly removes the
unnecessary 64-bit division (as opposed to adding a call to div_s64()
or do_div()), and if it removes all traces of 'struct timespec' again?
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 15, 2016 at 11:50 PM, Arnd Bergmann [off-list ref] wrote:
On Wednesday, June 15, 2016 7:04:54 PM CEST Saeed Mahameed wrote:
Ok, I see. It would be nice if the process had a way to avoid build regressions
in linux-next, in particular if you already have a fix by the time a patch
that introduces a problem gets added.
The reason we added this tree is to get 0-day testing but currently it
makes some unwanted noise
so we will remove it until we figure it out.
Can you check if the fix for the second problem correctly removes the
unnecessary 64-bit division (as opposed to adding a call to div_s64()
or do_div()), and if it removes all traces of 'struct timespec' again?
Yes, same thing, already fixed, will reply to that thread.
On Friday, June 17, 2016 5:50:14 PM CEST Saeed Mahameed wrote:
On Wed, Jun 15, 2016 at 11:50 PM, Arnd Bergmann [off-list ref] wrote:
quoted
On Wednesday, June 15, 2016 7:04:54 PM CEST Saeed Mahameed wrote:
Ok, I see. It would be nice if the process had a way to avoid build regressions
in linux-next, in particular if you already have a fix by the time a patch
that introduces a problem gets added.
The reason we added this tree is to get 0-day testing but currently it
makes some unwanted noise
so we will remove it until we figure it out.
I think you can simply ask Fengguang Wu to add your git tree to the list
of trees he pulls from for the 0-day test bot.
quoted
Can you check if the fix for the second problem correctly removes the
unnecessary 64-bit division (as opposed to adding a call to div_s64()
or do_div()), and if it removes all traces of 'struct timespec' again?
Yes, same thing, already fixed, will reply to that thread.
On Wed, Jun 15, 2016 at 6:27 PM, Arnd Bergmann [off-list ref] wrote:
The mlx5 driver fails to build on 32-bit architectures after some
references to 64-bit divisions got added:
drivers/net/built-in.o: In function `mlx5e_rx_am':
:(.text+0xf88ac): undefined reference to `__aeabi_ldivmod'
The driver even performs three division here, and it uses the
obsolete 'struct timespec' that we want to get rid of.
Using ktime_t and ktime_us_delta() replaces one of the divisions
and is mildly more efficient, aside from working across 'settimeofday'
calls and being the right type for the y2038 conversion.
Using a u32 instead of s64 to store the number of microseconds
limits the maximum time to about 71 minutes, but if we exceed that
time, we probably don't care about the result any more for the
purpose of rx coalescing.
For the number of packets, we are taking the difference between
two 'unsigned int', so the result won't ever be greater than that
either.
After those changes, the other two divisions are done as 32-bit
arithmetic operations, which are much faster.
Nice catch Arnd, we originally fixed this with div_u64, but your
solution looks wiser.
does ktime_t gives time in a resolution same as timespec ?
As discussed before this patch can't be applied on net-next as
the original patch which it meant to fix is yet to be submitted,
I will CC you once we submit the fixed patch.
On Friday, June 17, 2016 6:09:00 PM CEST Saeed Mahameed wrote:
On Wed, Jun 15, 2016 at 6:27 PM, Arnd Bergmann [off-list ref] wrote:
quoted
The mlx5 driver fails to build on 32-bit architectures after some
references to 64-bit divisions got added:
drivers/net/built-in.o: In function `mlx5e_rx_am':
:(.text+0xf88ac): undefined reference to `__aeabi_ldivmod'
The driver even performs three division here, and it uses the
obsolete 'struct timespec' that we want to get rid of.
Using ktime_t and ktime_us_delta() replaces one of the divisions
and is mildly more efficient, aside from working across 'settimeofday'
calls and being the right type for the y2038 conversion.
Using a u32 instead of s64 to store the number of microseconds
limits the maximum time to about 71 minutes, but if we exceed that
time, we probably don't care about the result any more for the
purpose of rx coalescing.
For the number of packets, we are taking the difference between
two 'unsigned int', so the result won't ever be greater than that
either.
After those changes, the other two divisions are done as 32-bit
arithmetic operations, which are much faster.
Nice catch Arnd, we originally fixed this with div_u64, but your
solution looks wiser.
does ktime_t gives time in a resolution same as timespec ?
ktime_t is a 64-bit nanosecond counter, so the resolution is the same
as ktime_get_ts64(), which is the "monotonic" equivalent of
getnstimeofday().
There are also variants that have the same resolution but are
less accurate and don't set the exact lower bits in order to
get a faster reading, but the above are all as accurate as the machine
allows.
Arnd
From: Leon Romanovsky <leon@kernel.org> Date: 2016-06-17 16:11:26
On Fri, Jun 17, 2016 at 05:02:33PM +0200, Arnd Bergmann wrote:
On Friday, June 17, 2016 5:50:14 PM CEST Saeed Mahameed wrote:
quoted
On Wed, Jun 15, 2016 at 11:50 PM, Arnd Bergmann [off-list ref] wrote:
quoted
On Wednesday, June 15, 2016 7:04:54 PM CEST Saeed Mahameed wrote:
Ok, I see. It would be nice if the process had a way to avoid build regressions
in linux-next, in particular if you already have a fix by the time a patch
that introduces a problem gets added.
The reason we added this tree is to get 0-day testing but currently it
makes some unwanted noise
so we will remove it until we figure it out.
I think you can simply ask Fengguang Wu to add your git tree to the list
of trees he pulls from for the 0-day test bot.
It is not 0-day only, but linux-next too. It works flawlessly for RDMA
topics and Doug receives cleaned and fully tested patches. Sadly enough,
it didn't work well for mlx5 net part.
Till further notice, I removed mlx5 net part (submission queue) from my
tree and from linux-next.