* Re: [PATCH] sch_gred: kzalloc needs null check
From: Eric Dumazet @ 2019-07-19 1:42 UTC (permalink / raw)
To: Navid Emamdoost
Cc: emamd001, kjlu, smccaman, Jamal Hadi Salim, Cong Wang, Jiri Pirko,
David S. Miller, netdev, linux-kernel
In-Reply-To: <20190719013026.24297-1-navid.emamdoost@gmail.com>
On 7/19/19 3:30 AM, Navid Emamdoost wrote:
> call to kzalloc may fail and return null. So the result should be checked
> against null. Added the check to cover kzalloc failure case.
>
> Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
> ---
> net/sched/sch_gred.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/net/sched/sch_gred.c b/net/sched/sch_gred.c
> index 8599c6f31b05..5cd0859f0274 100644
> --- a/net/sched/sch_gred.c
> +++ b/net/sched/sch_gred.c
> @@ -697,6 +697,9 @@ static int gred_change(struct Qdisc *sch, struct nlattr *opt,
> }
>
> prealloc = kzalloc(sizeof(*prealloc), GFP_KERNEL);
> + if (!prealloc)
> + return -ENOMEM;
> +
> sch_tree_lock(sch);
>
> err = gred_change_vq(sch, ctl->DP, ctl, prio, stab, max_P, &prealloc,
>
This seems not needed.
The case is handled later in gred_change_vq()
^ permalink raw reply
* [PATCH] sch_gred: kzalloc needs null check
From: Navid Emamdoost @ 2019-07-19 1:30 UTC (permalink / raw)
Cc: emamd001, kjlu, smccaman, Navid Emamdoost, Jamal Hadi Salim,
Cong Wang, Jiri Pirko, David S. Miller, netdev, linux-kernel
call to kzalloc may fail and return null. So the result should be checked
against null. Added the check to cover kzalloc failure case.
Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
---
net/sched/sch_gred.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/net/sched/sch_gred.c b/net/sched/sch_gred.c
index 8599c6f31b05..5cd0859f0274 100644
--- a/net/sched/sch_gred.c
+++ b/net/sched/sch_gred.c
@@ -697,6 +697,9 @@ static int gred_change(struct Qdisc *sch, struct nlattr *opt,
}
prealloc = kzalloc(sizeof(*prealloc), GFP_KERNEL);
+ if (!prealloc)
+ return -ENOMEM;
+
sch_tree_lock(sch);
err = gred_change_vq(sch, ctl->DP, ctl, prio, stab, max_P, &prealloc,
--
2.17.1
^ permalink raw reply related
* Re: [PATCH bpf] libbpf: fix missing __WORDSIZE definition
From: Arnaldo Carvalho de Melo @ 2019-07-19 1:16 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Arnaldo Carvalho de Melo, Andrii Nakryiko, bpf, Networking,
Daniel Borkmann, Alexei Starovoitov, Kernel Team, Jiri Olsa,
Namhyung Kim
In-Reply-To: <CAEf4BzburdiRTYSJUSpSFAxKmf6ELpvEeNW502eKskzyyMaUxQ@mail.gmail.com>
Em Thu, Jul 18, 2019 at 02:16:29PM -0700, Andrii Nakryiko escreveu:
> On Thu, Jul 18, 2019 at 12:14 PM Arnaldo Carvalho de Melo
> <arnaldo.melo@gmail.com> wrote:
> >
> > Em Thu, Jul 18, 2019 at 03:56:19PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > I'll stop and replace my patch with yours to see if it survives all the
> > > test builds...
> >
> > So, Alpine:3.4, the first image for this distro I did when I started
> > these builds, survives the 6 builds with gcc and clang with your patch:
> >
> >
> > [perfbuilder@quaco linux-perf-tools-build]$ export PERF_TARBALL=http://192.168.124.1/perf/perf-5.2.0.tar.xz
> > [perfbuilder@quaco linux-perf-tools-build]$ dm
> > 1 alpine:3.4 : Ok gcc (Alpine 5.3.0) 5.3.0, clang version 3.8.0 (tags/RELEASE_380/final)
> >
> >
> > [perfbuilder@quaco linux-perf-tools-build]$ grep "+ make" dm.log/alpine\:3.4
> > + make ARCH= CROSS_COMPILE= EXTRA_CFLAGS= -C /git/linux/tools/perf O=/tmp/build/perf
> > + make ARCH= CROSS_COMPILE= EXTRA_CFLAGS= NO_LIBELF=1 -C /git/linux/tools/perf O=/tmp/build/perf
> > + make ARCH= CROSS_COMPILE= EXTRA_CFLAGS= -C /git/linux/tools/perf O=/tmp/build/perf CC=clang
> > + make ARCH= CROSS_COMPILE= EXTRA_CFLAGS= NO_LIBELF=1 -C /git/linux/tools/perf O=/tmp/build/perf CC=clang
> > + make ARCH= CROSS_COMPILE= EXTRA_CFLAGS= LIBCLANGLLVM=1 -C /git/linux/tools/perf O=/tmp/build/perf CC=clang
> > + make ARCH= CROSS_COMPILE= EXTRA_CFLAGS= LIBCLANGLLVM=1 -C /git/linux/tools/perf O=/tmp/build/perf
> > [perfbuilder@quaco linux-perf-tools-build]$
> >
> > Probably all the rest will go well, will let you know.
> >
> > Daniel, do you mind if I carry this one in my perf/core branch? Its
> > small and shouldn't clash with other patches, I think. It should go
> > upstream soon:
> >
> > Andrii, there are these others:
>
> I took a look at them, but I think it would be better, if you could
> post them as proper patches to
> bpf@vger.kernel.org/netdev@vger.kernel.org, so that others can check
> and comment, if necessary.
>
> One nit for all three of them: we typically prefix subject with just
> "libbpf: " instead of "tools lib libbpf".
Sure, that was mechanic, I do it like that for the patches I upstream,
and that was like that in the beginning:
[acme@quaco perf]$ git log --oneline tools/lib/bpf | grep lib | tail
9d759a9b4ac2 tools lib bpf: Collect map definition in bpf_object
d8ad6a15cc3a tools lib bpf: Don't do a feature check when cleaning
6371ca3b541c bpf tools: Improve libbpf error reporting
0c77c04aa9c2 tools lib bpf: Change FEATURE-DUMP to FEATURE-DUMP.libbpf
715f8db9102f tools lib bpf: Fix compiler warning on CentOS 6
7c422f557266 tools build: Build fixdep helper from perf and basic libs
65f041bee783 tools lib bpf: Use FEATURE_USER to allow building in the same dir as perf
20517cd9c593 tools lib bpf: Fix up FEATURE_{TESTS,DISPLAY} usage
cc4228d57c4c bpf tools: Check endianness and make libbpf fail early
1b76c13e4b36 bpf tools: Introduce 'bpf' library and add bpf feature check
[acme@quaco perf]$
Anyway, I'll resubmit the patches that you acked to bpf@vger and will
let my container tests fail for those cases, sticking a warning so that
Ingo knows that this is being dealt with and those problems will get
fixed soon when the bpf tree merges upstream.
> >
> > 8dfb6ed300bf tools lib bpf: Avoid designated initializers for unnamed union members
>
> + attr.sample_period = attr.wakeup_events = 1;
>
> let's instead
>
> + attr.sample_period = 1;
> + attr.wakeup_events = 1;
>
> I don't like multi-assignments.
Meh, what's wrong with it? :)
> Also, if we are doing explicit assignment, let's do it for all the
> fields, not split initialization like that.
If that is what takes to get it to build everywhere, no problem. In
tools/perf I'm used to doing it, documents that this is an oddity to
support more systems :)
> > 80f7f8f21441 tools lib bpf: Avoid using 'link' as it shadows a global definition in some systems
>
> For this one I'm confused. What compiler/system you are getting it on?
> I tried to reproduce it with this example (trying both global
> variable, as well as function):
>
> #include <stdio.h>
>
> //int link = 1;
> void link() {}
>
> int f(int link) {
> return link;
> }
> int main() {
> printf("%d\n", f(123));
> return 0;
> }
>
> I haven't gotten any errors nor warnings. I'm certainly liking
> existing naming better, but my main concern is that we'll probably add
> more code like this, and we'll forget about this problem and will
> re-introduce.
yeah, this happens from time to time with centos:6 and IIRC
amazonlinux:1, oraclelinux:6.
I still remember when I got reports from the twitter guys when something
broke on rhel:5, that was the main reason to get these container tests
in place, you never know where people are using this, and since before
upstreaming I do the tests, fixing those became second nature 8-)
> So I'd rather figure out why it's happening and some rare system and
> see if we can mitigate that without all the renames.
>
>
> > d93fe741e291 tools lib bpf: Fix endianness macro usage for some compilers
>
> This one looks good to me, thanks!
>
> Acked-by: Andrii Nakryiko <andriin@fb.com>
Ok, will collect this, change the prefix to: "libbpf: ..." and send to
bpf@vger
>
> >
> > If you could take a look at them at my tmp.perf/core branch at:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/log/?h=tmp.perf/core
> >
> > I'm force pushing it now to replace my __WORDSIZE patch with yours, the
> > SHAs should be the 3 above and the one below.
> >
> > And to build cleanly I had to cherry pick this one:
> >
> > 3091dafc1884 (HEAD -> perf/core) libbpf: fix ptr to u64 conversion warning on 32-bit platforms
> >
> > Alpine:3.5 just finished:
> >
> > 2 alpine:3.5 : Ok gcc (Alpine 6.2.1) 6.2.1 20160822, clang version 3.8.1 (tags/RELEASE_381/final)
> >
> > - Arnaldo
--
- Arnaldo
^ permalink raw reply
* [PATCH v2] ag71xx: fix return value check in ag71xx_probe()
From: Wei Yongjun @ 2019-07-19 1:22 UTC (permalink / raw)
To: Jay Cliburn, Chris Snook, Oleksij Rempel
Cc: Wei Yongjun, netdev, kernel-janitors
In-Reply-To: <20190717115225.23047-1-weiyongjun1@huawei.com>
In case of error, the function of_get_mac_address() returns ERR_PTR()
and never returns NULL. The NULL test in the return value check should
be replaced with IS_ERR().
Fixes: d51b6ce441d3 ("net: ethernet: add ag71xx driver")
Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
Reviewed-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
v1 -> v2: fix subsystem prefix
---
drivers/net/ethernet/atheros/ag71xx.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/atheros/ag71xx.c b/drivers/net/ethernet/atheros/ag71xx.c
index 72a57c6cd254..3088a43e6436 100644
--- a/drivers/net/ethernet/atheros/ag71xx.c
+++ b/drivers/net/ethernet/atheros/ag71xx.c
@@ -1732,9 +1732,9 @@ static int ag71xx_probe(struct platform_device *pdev)
ag->stop_desc->next = (u32)ag->stop_desc_dma;
mac_addr = of_get_mac_address(np);
- if (mac_addr)
+ if (!IS_ERR(mac_addr))
memcpy(ndev->dev_addr, mac_addr, ETH_ALEN);
- if (!mac_addr || !is_valid_ether_addr(ndev->dev_addr)) {
+ if (IS_ERR(mac_addr) || !is_valid_ether_addr(ndev->dev_addr)) {
netif_err(ag, probe, ndev, "invalid MAC address, using random address\n");
eth_random_addr(ndev->dev_addr);
}
^ permalink raw reply related
* [PATCH v2] ag71xx: fix error return code in ag71xx_probe()
From: Wei Yongjun @ 2019-07-19 1:21 UTC (permalink / raw)
To: Jay Cliburn, Chris Snook, Oleksij Rempel
Cc: Wei Yongjun, netdev, kernel-janitors
In-Reply-To: <20190717115215.22965-1-weiyongjun1@huawei.com>
Fix to return error code -ENOMEM from the dmam_alloc_coherent() error
handling case instead of 0, as done elsewhere in this function.
Fixes: d51b6ce441d3 ("net: ethernet: add ag71xx driver")
Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
Reviewed-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
v1 -> v2: fix subsystem prefix
---
drivers/net/ethernet/atheros/ag71xx.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/atheros/ag71xx.c b/drivers/net/ethernet/atheros/ag71xx.c
index 72a57c6cd254..446d62e93439 100644
--- a/drivers/net/ethernet/atheros/ag71xx.c
+++ b/drivers/net/ethernet/atheros/ag71xx.c
@@ -1724,8 +1724,10 @@ static int ag71xx_probe(struct platform_device *pdev)
ag->stop_desc = dmam_alloc_coherent(&pdev->dev,
sizeof(struct ag71xx_desc),
&ag->stop_desc_dma, GFP_KERNEL);
- if (!ag->stop_desc)
+ if (!ag->stop_desc) {
+ err = -ENOMEM;
goto err_free;
+ }
ag->stop_desc->data = 0;
ag->stop_desc->ctrl = 0;
^ permalink raw reply related
* Re: [PATCH] net: bcmgenet: use promisc for unsupported filters
From: Doug Berger @ 2019-07-19 0:51 UTC (permalink / raw)
To: justinpopo6, netdev
Cc: linux-kernel, bcm-kernel-feedback-list, davem, f.fainelli
In-Reply-To: <1563400733-39451-1-git-send-email-justinpopo6@gmail.com>
On 7/17/19 2:58 PM, justinpopo6@gmail.com wrote:
> From: Justin Chen <justinpopo6@gmail.com>
>
> Currently we silently ignore filters if we cannot meet the filter
> requirements. This will lead to the MAC dropping packets that are
> expected to pass. A better solution would be to set the NIC to promisc
> mode when the required filters cannot be met.
>
> Also correct the number of MDF filters supported. It should be 17,
> not 16.
>
> Signed-off-by: Justin Chen <justinpopo6@gmail.com>
Acked-by: Doug Berger <opendmb@gmail.com>
Thanks Justin :)
Doug
^ permalink raw reply
* Re: [PATCH v3 net-next 13/19] ionic: Add initial ethtool support
From: Shannon Nelson @ 2019-07-19 0:12 UTC (permalink / raw)
To: Andrew Lunn; +Cc: netdev
In-Reply-To: <20190718032814.GH6962@lunn.ch>
On 7/17/19 8:28 PM, Andrew Lunn wrote:
> On Fri, Jul 12, 2019 at 10:16:31PM -0700, Shannon Nelson wrote:
>> On 7/8/19 7:14 PM, Andrew Lunn wrote:
>>>> +static int ionic_set_pauseparam(struct net_device *netdev,
>>>> + struct ethtool_pauseparam *pause)
>>>> +{
>>>> + struct lif *lif = netdev_priv(netdev);
>>>> + struct ionic *ionic = lif->ionic;
>>>> + struct ionic_dev *idev = &lif->ionic->idev;
>>>> +
>>>> + u32 requested_pause;
>>>> + u32 cur_autoneg;
>>>> + int err;
>>>> +
>>>> + cur_autoneg = idev->port_info->config.an_enable ? AUTONEG_ENABLE :
>>>> + AUTONEG_DISABLE;
>>>> + if (pause->autoneg != cur_autoneg) {
>>>> + netdev_info(netdev, "Please use 'ethtool -s ...' to change autoneg\n");
>>>> + return -EOPNOTSUPP;
>>>> + }
>>>> +
>>>> + /* change both at the same time */
>>>> + requested_pause = PORT_PAUSE_TYPE_LINK;
>>>> + if (pause->rx_pause)
>>>> + requested_pause |= IONIC_PAUSE_F_RX;
>>>> + if (pause->tx_pause)
>>>> + requested_pause |= IONIC_PAUSE_F_TX;
>>>> +
>>>> + if (requested_pause == idev->port_info->config.pause_type)
>>>> + return 0;
>>>> +
>>>> + idev->port_info->config.pause_type = requested_pause;
>>>> +
>>>> + mutex_lock(&ionic->dev_cmd_lock);
>>>> + ionic_dev_cmd_port_pause(idev, requested_pause);
>>>> + err = ionic_dev_cmd_wait(ionic, devcmd_timeout);
>>>> + mutex_unlock(&ionic->dev_cmd_lock);
>>>> + if (err)
>>>> + return err;
>>> Hi Shannon
>>>
>>> I've no idea what the firmware black box is doing, but this looks
>>> wrong.
>>>
>>> pause->autoneg is about if the results of auto-neg should be used or
>>> not. If false, just configure the MAC with the pause settings and you
>>> are done. If the interface is being forced, so autoneg in general is
>>> disabled, just configure the MAC and you are done.
>>>
>>> If pause->autoneg is true and the interface is using auto-neg as a
>>> whole, you pass the pause values to the PHY for it to advertise and
>>> trigger an auto-neg. Once autoneg has completed, and the resolved
>>> settings are available, the MAC is configured with the resolved
>>> values.
>>>
>>> Looking at this code, i don't see any difference between configuring
>>> the MAC or configuring the PHY. I would expect pause->autoneg to be
>>> part of requested_pause somehow, so the firmware knows what is should
>>> do.
>>>
>>> Andrew
>> In this device there's actually very little the driver can do to directly
>> configure the mac or phy besides passing through to the firmware what the
>> user has requested - that happens here for the pause values, and in
>> ionic_set_link_ksettings() for autoneg. The firmware is managing the port
>> based on these requests with the help of internally configured rules defined
>> in a customer setting.
> I get that. But the firmware needs to conform to what Linux
> expects. And what i see here does not conform. That is why i gave a
> bit of detail in my reply.
>
> What exactly does the firmware do? Once we know that, we can figure
> out when the driver should return -EOPNOTSUPP because of firmware
> limitations, and what it can configure and should return 0.
Because this is fairly smart FW, it handles this as expected. I can add
this as another comment in the code.
sln
^ permalink raw reply
* Re: [PATCH] net: ethernet: mediatek: Add MT7628/88 SoC support
From: David Miller @ 2019-07-18 23:36 UTC (permalink / raw)
To: sr; +Cc: netdev, linux-mediatek, opensource, sean.wang, nbd, john
In-Reply-To: <20190717110243.14240-1-sr@denx.de>
From: Stefan Roese <sr@denx.de>
Date: Wed, 17 Jul 2019 13:02:43 +0200
> This patch adds support for the MediaTek MT7628/88 SoCs to the common
> MediaTek ethernet driver. Some minor changes are needed for this and
> a bigger change, as the MT7628 does not support QDMA (only PDMA).
>
> Signed-off-by: Stefan Roese <sr@denx.de>
Besides the feedback you've received, this kind of change is only appropriate
for the net-next tree at this time.
If you wish to keep sending versions for review until the net-next tree opens
back up, clearly indicate in your Subject line by saying "[PATCH RFC ...]" or
similar.
Thank you.
^ permalink raw reply
* Re: [PATCH] usb: qmi_wwan: add D-Link DWM-222 A2 device ID
From: David Miller @ 2019-07-18 23:34 UTC (permalink / raw)
To: rogan; +Cc: bjorn, netdev, linux-usb
In-Reply-To: <20190717091433.GA5325@lisa.dawes.za.net>
From: Rogan Dawes <rogan@dawes.za.net>
Date: Wed, 17 Jul 2019 11:14:33 +0200
> Signed-off-by: Rogan Dawes <rogan@dawes.za.net>
Applied.
^ permalink raw reply
* Re: [PATCH net] bnxt_en: Fix VNIC accounting when enabling aRFS on 57500 chips.
From: David Miller @ 2019-07-18 23:34 UTC (permalink / raw)
To: michael.chan; +Cc: netdev
In-Reply-To: <1563347243-8100-1-git-send-email-michael.chan@broadcom.com>
From: Michael Chan <michael.chan@broadcom.com>
Date: Wed, 17 Jul 2019 03:07:23 -0400
> Unlike legacy chips, 57500 chips don't need additional VNIC resources
> for aRFS/ntuple. Fix the code accordingly so that we don't reserve
> and allocate additional VNICs on 57500 chips. Without this patch,
> the driver is failing to initialize when it tries to allocate extra
> VNICs.
>
> Fixes: ac33906c67e2 ("bnxt_en: Add support for aRFS on 57500 chips.")
> Signed-off-by: Michael Chan <michael.chan@broadcom.com>
Applied.
> Please queue this for 5.2 -stable also. Thanks.
Queued up.
^ permalink raw reply
* Re: [PATCH] net: dsa: sja1105: Fix missing unlock on error in sk_buff()
From: David Miller @ 2019-07-18 23:33 UTC (permalink / raw)
To: weiyongjun1
Cc: andrew, vivien.didelot, f.fainelli, olteanv, netdev,
kernel-janitors
In-Reply-To: <20190717062956.127446-1-weiyongjun1@huawei.com>
From: Wei Yongjun <weiyongjun1@huawei.com>
Date: Wed, 17 Jul 2019 06:29:56 +0000
> Add the missing unlock before return from function sk_buff()
> in the error handling case.
>
> Fixes: f3097be21bf1 ("net: dsa: sja1105: Add a state machine for RX timestamping")
> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
Applied.
^ permalink raw reply
* Re: [PATCH] gve: replace kfree with kvfree
From: David Miller @ 2019-07-18 23:32 UTC (permalink / raw)
To: chuhongyuan; +Cc: csully, netdev, linux-kernel
In-Reply-To: <MWHPR11MB1757F23147F85B59BCF1628BAFC90@MWHPR11MB1757.namprd11.prod.outlook.com>
From: Chuhong YUAN <chuhongyuan@outlook.com>
Date: Wed, 17 Jul 2019 00:59:02 +0000
> Variables allocated by kvzalloc should not be freed by kfree.
> Because they may be allocated by vmalloc.
> So we replace kfree with kvfree here.
>
> Signed-off-by: Chuhong Yuan <chuhongyuan@outlook.com>
Applied, thanks Chuhong.
GVE maintainers, you are upstream now and have to stay on top of review
of changes like this. Otherwise I'll just review it myself and apply
it unless I find problems, and that may not be what you want :)
^ permalink raw reply
* Re: [PATCH] bnx2x: Prevent load reordering in tx completion processing
From: David Miller @ 2019-07-18 23:30 UTC (permalink / raw)
To: brking; +Cc: GR-everest-linux-l2, skalluru, aelior, netdev
In-Reply-To: <1563226910-21660-1-git-send-email-brking@linux.vnet.ibm.com>
From: Brian King <brking@linux.vnet.ibm.com>
Date: Mon, 15 Jul 2019 16:41:50 -0500
> This patch fixes an issue seen on Power systems with bnx2x which results
> in the skb is NULL WARN_ON in bnx2x_free_tx_pkt firing due to the skb
> pointer getting loaded in bnx2x_free_tx_pkt prior to the hw_cons
> load in bnx2x_tx_int. Adding a read memory barrier resolves the issue.
>
> Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
I agree with Brian's explanation of how the reordering can happen, and why
the crash doesn't show up in the prefetch() call.
I'll give the Marvell folks one more day to give a proper ACK.
Thanks.
^ permalink raw reply
* Re: [PATCH] be2net: fix adapter->big_page_size miscaculation
From: David Miller @ 2019-07-18 23:29 UTC (permalink / raw)
To: cai
Cc: morbo, ndesaulniers, jyknight, sathya.perla, ajit.khaparde,
sriharsha.basavapatna, somnath.kotur, arnd, dhowells, hpa, netdev,
linux-arch, linux-kernel, natechancellor
In-Reply-To: <75B428FC-734C-4B15-B1A7-A3FC5F9F2FE5@lca.pw>
From: Qian Cai <cai@lca.pw>
Date: Thu, 18 Jul 2019 19:26:47 -0400
>
>
>> On Jul 18, 2019, at 5:21 PM, Bill Wendling <morbo@google.com> wrote:
>>
>> [My previous response was marked as spam...]
>>
>> Top-of-tree clang says that it's const:
>>
>> $ gcc a.c -O2 && ./a.out
>> a is a const.
>>
>> $ clang a.c -O2 && ./a.out
>> a is a const.
>
>
> I used clang-7.0.1. So, this is getting worse where both GCC and clang will start to suffer the
> same problem.
Then rewrite the module parameter macros such that the non-constness
is evident to all compilers regardless of version.
That is the place to fix this, otherwise we will just be adding hacks
all over the place rather than in just one spot.
Thanks.
^ permalink raw reply
* Re: [PATCH] be2net: fix adapter->big_page_size miscaculation
From: Qian Cai @ 2019-07-18 23:26 UTC (permalink / raw)
To: Bill Wendling
Cc: Nick Desaulniers, James Y Knight, David Miller, sathya.perla,
ajit.khaparde, sriharsha.basavapatna, somnath.kotur,
Arnd Bergmann, David Howells, H. Peter Anvin, netdev, linux-arch,
LKML, Nathan Chancellor
In-Reply-To: <CAGG=3QUvdwJs1wW1w+5Mord-qFLa=_WkjTsiZuwGfcjkoEJGNQ@mail.gmail.com>
> On Jul 18, 2019, at 5:21 PM, Bill Wendling <morbo@google.com> wrote:
>
> [My previous response was marked as spam...]
>
> Top-of-tree clang says that it's const:
>
> $ gcc a.c -O2 && ./a.out
> a is a const.
>
> $ clang a.c -O2 && ./a.out
> a is a const.
I used clang-7.0.1. So, this is getting worse where both GCC and clang will start to suffer the
same problem.
>
>
> On Thu, Jul 18, 2019 at 2:10 PM Nick Desaulniers
> <ndesaulniers@google.com> wrote:
>>
>> On Thu, Jul 18, 2019 at 2:01 PM Qian Cai <cai@lca.pw> wrote:
>>>
>>>
>>>
>>>> On Jul 12, 2019, at 8:50 PM, David Miller <davem@davemloft.net> wrote:
>>>>
>>>> From: Qian Cai <cai@lca.pw>
>>>> Date: Fri, 12 Jul 2019 20:27:09 -0400
>>>>
>>>>> Actually, GCC would consider it a const with -O2 optimized level because it found that it was never modified and it does not understand it is a module parameter. Considering the following code.
>>>>>
>>>>> # cat const.c
>>>>> #include <stdio.h>
>>>>>
>>>>> static int a = 1;
>>>>>
>>>>> int main(void)
>>>>> {
>>>>> if (__builtin_constant_p(a))
>>>>> printf("a is a const.\n");
>>>>>
>>>>> return 0;
>>>>> }
>>>>>
>>>>> # gcc -O2 const.c -o const
>>>>
>>>> That's not a complete test case, and with a proper test case that
>>>> shows the externalization of the address of &a done by the module
>>>> parameter macros, gcc should not make this optimization or we should
>>>> define the module parameter macros in a way that makes this properly
>>>> clear to the compiler.
>>>>
>>>> It makes no sense to hack around this locally in drivers and other
>>>> modules.
>>>
>>> If you see the warning in the original patch,
>>>
>>> https://lore.kernel.org/netdev/1562959401-19815-1-git-send-email-cai@lca.pw/
>>>
>>> GCC definitely optimize rx_frag_size to be a constant while I just confirmed clang
>>> -O2 does not. The problem is that I have no clue about how to let GCC not to
>>> optimize a module parameter.
>>>
>>> Though, I have added a few people who might know more of compilers than myself.
>>
>> + Bill and James, who probably knows more than they'd like to about
>> __builtin_constant_p and more than other LLVM folks at this point.
>>
>> --
>> Thanks,
>> ~Nick Desaulniers
^ permalink raw reply
* Re: [PATCH] net: ag71xx: fix return value check in ag71xx_probe()
From: David Miller @ 2019-07-18 23:21 UTC (permalink / raw)
To: weiyongjun1; +Cc: jcliburn, chris.snook, o.rempel, netdev, kernel-janitors
In-Reply-To: <20190717115225.23047-1-weiyongjun1@huawei.com>
Please resubmit these two ag71xx patches, you use a different subsystem
prefix in the Subject lines, so you should make them consistent.
Using just "ag71xx: " is perfectly fine.
Thank you.
^ permalink raw reply
* Re: [PATCH iproute2 net-next v5 1/5] etf: Add skip_sock_check
From: David Ahern @ 2019-07-18 22:48 UTC (permalink / raw)
To: Vedang Patel, netdev
Cc: jhs, xiyou.wangcong, jiri, stephen, vinicius.gomes,
leandro.maciel.dorileo, jakub.kicinski, m-karicheri2
In-Reply-To: <1563479743-8371-1-git-send-email-vedang.patel@intel.com>
On 7/18/19 1:55 PM, Vedang Patel wrote:
> ETF Qdisc currently checks for a socket with SO_TXTIME socket option. If
> either is not present, the packet is dropped. In the future commits, we
> want other Qdiscs to add packet with launchtime to the ETF Qdisc. Also,
> there are some packets (e.g. ICMP packets) which may not have a socket
> associated with them. So, add an option to skip this check.
>
> Signed-off-by: Vedang Patel <vedang.patel@intel.com>
> ---
> tc/q_etf.c | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
applied all to iproute2-next. Thanks
^ permalink raw reply
* Re: [PATCH net-next iproute2 v2 0/3] net/sched: Introduce tc connection tracking
From: David Ahern @ 2019-07-18 22:42 UTC (permalink / raw)
To: Paul Blakey, Jiri Pirko, Roi Dayan, Yossi Kuperman, Oz Shlomo,
Marcelo Ricardo Leitner, netdev, David Miller, Aaron Conole,
Zhike Wang
Cc: Rony Efraim, nst-kernel, John Hurley, Simon Horman, Justin Pettit
In-Reply-To: <1562832867-32347-1-git-send-email-paulb@mellanox.com>
On 7/11/19 2:14 AM, Paul Blakey wrote:
> Hi,
>
> This patch series add connection tracking capabilities in tc.
> It does so via a new tc action, called act_ct, and new tc flower classifier matching.
> Act ct and relevant flower matches, are still under review in net-next mailing list.
>
...
>
> Paul Blakey (3):
> tc: add NLA_F_NESTED flag to all actions options nested block
> tc: Introduce tc ct action
> tc: flower: Add matching on conntrack info
>
applied to iproute2-next. Thanks
^ permalink raw reply
* Re: [PATCH] openvswitch: Fix a possible memory leak on dst_cache
From: Gregory Rose @ 2019-07-18 22:12 UTC (permalink / raw)
To: Haishuang Yan, Pravin B Shelar, David S. Miller; +Cc: netdev, linux-kernel
In-Reply-To: <1563466028-2531-1-git-send-email-yanhaishuang@cmss.chinamobile.com>
On 7/18/2019 9:07 AM, Haishuang Yan wrote:
> dst_cache should be destroyed when fail to add flow actions.
>
> Fixes: d71785ffc7e7 ("net: add dst_cache to ovs vxlan lwtunnel")
> Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com>
> ---
> net/openvswitch/flow_netlink.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
> index d7559c6..1fd1cdd 100644
> --- a/net/openvswitch/flow_netlink.c
> +++ b/net/openvswitch/flow_netlink.c
> @@ -2608,6 +2608,7 @@ static int validate_and_copy_set_tun(const struct nlattr *attr,
> sizeof(*ovs_tun), log);
> if (IS_ERR(a)) {
> dst_release((struct dst_entry *)tun_dst);
> + dst_cache_destroy(&tun_dst->u.tun_info.dst_cache);
> return PTR_ERR(a);
> }
>
Nack.
dst_release will decrement the ref count and will
call_rcu(&dst->rcu_head, dst_destroy_rcu) if the ref count is zero. No
other net drivers call dst_destroy SFAICT.
Haishuang,
are you trying to fix some specific problem here?
Thanks,
- Greg
^ permalink raw reply
* Re: [PATCH ghak90 V6 02/10] audit: add container id
From: Paul Moore @ 2019-07-18 21:52 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: Serge E. Hallyn, Tycho Andersen, containers, linux-api,
Linux-Audit Mailing List, linux-fsdevel, LKML, netdev,
netfilter-devel, sgrubb, omosnace, dhowells, simo, Eric Paris,
ebiederm, nhorman
In-Reply-To: <20190718005145.eshekqfr3navqqiy@madcap2.tricolour.ca>
On Wed, Jul 17, 2019 at 8:52 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2019-07-16 19:30, Paul Moore wrote:
...
> > We can trust capable(CAP_AUDIT_CONTROL) for enforcing audit container
> > ID policy, we can not trust ns_capable(CAP_AUDIT_CONTROL).
>
> Ok. So does a process in a non-init user namespace have two (or more)
> sets of capabilities stored in creds, one in the init_user_ns, and one
> in current_user_ns? Or does it get stripped of all its capabilities in
> init_user_ns once it has its own set in current_user_ns? If the former,
> then we can use capable(). If the latter, we need another mechanism, as
> you have suggested might be needed.
Unfortunately I think the problem is that ultimately we need to allow
any container orchestrator that has been given privileges to manage
the audit container ID to also grant that privilege to any of the
child process/containers it manages. I don't believe we can do that
with capabilities based on the code I've looked at, and the
discussions I've had, but if you find a way I would leave to hear it.
> If some random unprivileged user wants to fire up a container
> orchestrator/engine in his own user namespace, then audit needs to be
> namespaced. Can we safely discard this scenario for now?
I think the only time we want to allow a container orchestrator to
manage the audit container ID is if it has been granted that privilege
by someone who has that privilege already. In the zero-container, or
single-level of containers, case this is relatively easy, and we can
accomplish it using CAP_AUDIT_CONTROL as the privilege. If we start
nesting container orchestrators it becomes more complicated as we need
to be able to support granting and inheriting this privilege in a
manner; this is why I suggested a new mechanism *may* be necessary.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH] be2net: fix adapter->big_page_size miscaculation
From: Bill Wendling @ 2019-07-18 21:28 UTC (permalink / raw)
To: Nick Desaulniers
Cc: Qian Cai, James Y Knight, David Miller, sathya.perla,
ajit.khaparde, sriharsha.basavapatna, somnath.kotur,
Arnd Bergmann, David Howells, H. Peter Anvin, netdev, linux-arch,
LKML, Nathan Chancellor
In-Reply-To: <CAKwvOd=B=Lj-hTtbe88bo89wLxJrDAsm3fJisSMD=hKkRHf6zw@mail.gmail.com>
Possibly. I'd need to ask him. :-)
On Thu, Jul 18, 2019 at 2:22 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Thu, Jul 18, 2019 at 2:18 PM Bill Wendling <morbo@google.com> wrote:
> >
> > Top-of-tree clang says that it's const:
> >
> > $ gcc a.c -O2 && ./a.out
> > a is a const.
> >
> > $ clang a.c -O2 && ./a.out
> > a is a const.
>
> Right, so I know you (Bill) did a lot of work to refactor
> __builtin_constant_p handling in Clang and LLVM in the
> pre-llvm-9-release timeframe. I suspect Qian might not be using
> clang-9 built from source (as clang-8 is the current release) and thus
> observing differences.
>
> >
> > On Thu, Jul 18, 2019 at 2:10 PM Nick Desaulniers <ndesaulniers@google.com> wrote:
> >>
> >> On Thu, Jul 18, 2019 at 2:01 PM Qian Cai <cai@lca.pw> wrote:
> >> >
> >> >
> >> >
> >> > > On Jul 12, 2019, at 8:50 PM, David Miller <davem@davemloft.net> wrote:
> >> > >
> >> > > From: Qian Cai <cai@lca.pw>
> >> > > Date: Fri, 12 Jul 2019 20:27:09 -0400
> >> > >
> >> > >> Actually, GCC would consider it a const with -O2 optimized level because it found that it was never modified and it does not understand it is a module parameter. Considering the following code.
> >> > >>
> >> > >> # cat const.c
> >> > >> #include <stdio.h>
> >> > >>
> >> > >> static int a = 1;
> >> > >>
> >> > >> int main(void)
> >> > >> {
> >> > >> if (__builtin_constant_p(a))
> >> > >> printf("a is a const.\n");
> >> > >>
> >> > >> return 0;
> >> > >> }
> >> > >>
> >> > >> # gcc -O2 const.c -o const
> >> > >
> >> > > That's not a complete test case, and with a proper test case that
> >> > > shows the externalization of the address of &a done by the module
> >> > > parameter macros, gcc should not make this optimization or we should
> >> > > define the module parameter macros in a way that makes this properly
> >> > > clear to the compiler.
> >> > >
> >> > > It makes no sense to hack around this locally in drivers and other
> >> > > modules.
> >> >
> >> > If you see the warning in the original patch,
> >> >
> >> > https://lore.kernel.org/netdev/1562959401-19815-1-git-send-email-cai@lca.pw/
> >> >
> >> > GCC definitely optimize rx_frag_size to be a constant while I just confirmed clang
> >> > -O2 does not. The problem is that I have no clue about how to let GCC not to
> >> > optimize a module parameter.
> >> >
> >> > Though, I have added a few people who might know more of compilers than myself.
> >>
> >> + Bill and James, who probably knows more than they'd like to about
> >> __builtin_constant_p and more than other LLVM folks at this point.
> >>
> >> --
> >> Thanks,
> >> ~Nick Desaulniers
>
>
>
> --
> Thanks,
> ~Nick Desaulniers
^ permalink raw reply
* Re: [PATCH] be2net: fix adapter->big_page_size miscaculation
From: Nick Desaulniers @ 2019-07-18 21:22 UTC (permalink / raw)
To: Bill Wendling
Cc: Qian Cai, James Y Knight, David Miller, sathya.perla,
ajit.khaparde, sriharsha.basavapatna, somnath.kotur,
Arnd Bergmann, David Howells, H. Peter Anvin, netdev, linux-arch,
LKML, Nathan Chancellor
In-Reply-To: <CAGG=3QWkgm+YhC=TWEWwt585Lbm8ZPG-uFre-kBRv+roPzZFbA@mail.gmail.com>
On Thu, Jul 18, 2019 at 2:18 PM Bill Wendling <morbo@google.com> wrote:
>
> Top-of-tree clang says that it's const:
>
> $ gcc a.c -O2 && ./a.out
> a is a const.
>
> $ clang a.c -O2 && ./a.out
> a is a const.
Right, so I know you (Bill) did a lot of work to refactor
__builtin_constant_p handling in Clang and LLVM in the
pre-llvm-9-release timeframe. I suspect Qian might not be using
clang-9 built from source (as clang-8 is the current release) and thus
observing differences.
>
> On Thu, Jul 18, 2019 at 2:10 PM Nick Desaulniers <ndesaulniers@google.com> wrote:
>>
>> On Thu, Jul 18, 2019 at 2:01 PM Qian Cai <cai@lca.pw> wrote:
>> >
>> >
>> >
>> > > On Jul 12, 2019, at 8:50 PM, David Miller <davem@davemloft.net> wrote:
>> > >
>> > > From: Qian Cai <cai@lca.pw>
>> > > Date: Fri, 12 Jul 2019 20:27:09 -0400
>> > >
>> > >> Actually, GCC would consider it a const with -O2 optimized level because it found that it was never modified and it does not understand it is a module parameter. Considering the following code.
>> > >>
>> > >> # cat const.c
>> > >> #include <stdio.h>
>> > >>
>> > >> static int a = 1;
>> > >>
>> > >> int main(void)
>> > >> {
>> > >> if (__builtin_constant_p(a))
>> > >> printf("a is a const.\n");
>> > >>
>> > >> return 0;
>> > >> }
>> > >>
>> > >> # gcc -O2 const.c -o const
>> > >
>> > > That's not a complete test case, and with a proper test case that
>> > > shows the externalization of the address of &a done by the module
>> > > parameter macros, gcc should not make this optimization or we should
>> > > define the module parameter macros in a way that makes this properly
>> > > clear to the compiler.
>> > >
>> > > It makes no sense to hack around this locally in drivers and other
>> > > modules.
>> >
>> > If you see the warning in the original patch,
>> >
>> > https://lore.kernel.org/netdev/1562959401-19815-1-git-send-email-cai@lca.pw/
>> >
>> > GCC definitely optimize rx_frag_size to be a constant while I just confirmed clang
>> > -O2 does not. The problem is that I have no clue about how to let GCC not to
>> > optimize a module parameter.
>> >
>> > Though, I have added a few people who might know more of compilers than myself.
>>
>> + Bill and James, who probably knows more than they'd like to about
>> __builtin_constant_p and more than other LLVM folks at this point.
>>
>> --
>> Thanks,
>> ~Nick Desaulniers
--
Thanks,
~Nick Desaulniers
^ permalink raw reply
* Re: [PATCH] be2net: fix adapter->big_page_size miscaculation
From: Bill Wendling @ 2019-07-18 21:21 UTC (permalink / raw)
To: Nick Desaulniers
Cc: Qian Cai, James Y Knight, David Miller, sathya.perla,
ajit.khaparde, sriharsha.basavapatna, somnath.kotur,
Arnd Bergmann, David Howells, H. Peter Anvin, netdev, linux-arch,
LKML, Nathan Chancellor
In-Reply-To: <CAKwvOdkCfqfpJYYX+iu2nLCUUkeDorDdVP3e7koB9NYsRwgCNw@mail.gmail.com>
[My previous response was marked as spam...]
Top-of-tree clang says that it's const:
$ gcc a.c -O2 && ./a.out
a is a const.
$ clang a.c -O2 && ./a.out
a is a const.
On Thu, Jul 18, 2019 at 2:10 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Thu, Jul 18, 2019 at 2:01 PM Qian Cai <cai@lca.pw> wrote:
> >
> >
> >
> > > On Jul 12, 2019, at 8:50 PM, David Miller <davem@davemloft.net> wrote:
> > >
> > > From: Qian Cai <cai@lca.pw>
> > > Date: Fri, 12 Jul 2019 20:27:09 -0400
> > >
> > >> Actually, GCC would consider it a const with -O2 optimized level because it found that it was never modified and it does not understand it is a module parameter. Considering the following code.
> > >>
> > >> # cat const.c
> > >> #include <stdio.h>
> > >>
> > >> static int a = 1;
> > >>
> > >> int main(void)
> > >> {
> > >> if (__builtin_constant_p(a))
> > >> printf("a is a const.\n");
> > >>
> > >> return 0;
> > >> }
> > >>
> > >> # gcc -O2 const.c -o const
> > >
> > > That's not a complete test case, and with a proper test case that
> > > shows the externalization of the address of &a done by the module
> > > parameter macros, gcc should not make this optimization or we should
> > > define the module parameter macros in a way that makes this properly
> > > clear to the compiler.
> > >
> > > It makes no sense to hack around this locally in drivers and other
> > > modules.
> >
> > If you see the warning in the original patch,
> >
> > https://lore.kernel.org/netdev/1562959401-19815-1-git-send-email-cai@lca.pw/
> >
> > GCC definitely optimize rx_frag_size to be a constant while I just confirmed clang
> > -O2 does not. The problem is that I have no clue about how to let GCC not to
> > optimize a module parameter.
> >
> > Though, I have added a few people who might know more of compilers than myself.
>
> + Bill and James, who probably knows more than they'd like to about
> __builtin_constant_p and more than other LLVM folks at this point.
>
> --
> Thanks,
> ~Nick Desaulniers
^ permalink raw reply
* Re: [PATCH bpf] libbpf: fix missing __WORDSIZE definition
From: Andrii Nakryiko @ 2019-07-18 21:16 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Andrii Nakryiko, bpf, Networking, Daniel Borkmann,
Alexei Starovoitov, Kernel Team, Jiri Olsa, Namhyung Kim
In-Reply-To: <20190718191452.GM3624@kernel.org>
On Thu, Jul 18, 2019 at 12:14 PM Arnaldo Carvalho de Melo
<arnaldo.melo@gmail.com> wrote:
>
> Em Thu, Jul 18, 2019 at 03:56:19PM -0300, Arnaldo Carvalho de Melo escreveu:
> > I'll stop and replace my patch with yours to see if it survives all the
> > test builds...
>
> So, Alpine:3.4, the first image for this distro I did when I started
> these builds, survives the 6 builds with gcc and clang with your patch:
>
>
> [perfbuilder@quaco linux-perf-tools-build]$ export PERF_TARBALL=http://192.168.124.1/perf/perf-5.2.0.tar.xz
> [perfbuilder@quaco linux-perf-tools-build]$ dm
> 1 alpine:3.4 : Ok gcc (Alpine 5.3.0) 5.3.0, clang version 3.8.0 (tags/RELEASE_380/final)
>
>
> [perfbuilder@quaco linux-perf-tools-build]$ grep "+ make" dm.log/alpine\:3.4
> + make ARCH= CROSS_COMPILE= EXTRA_CFLAGS= -C /git/linux/tools/perf O=/tmp/build/perf
> + make ARCH= CROSS_COMPILE= EXTRA_CFLAGS= NO_LIBELF=1 -C /git/linux/tools/perf O=/tmp/build/perf
> + make ARCH= CROSS_COMPILE= EXTRA_CFLAGS= -C /git/linux/tools/perf O=/tmp/build/perf CC=clang
> + make ARCH= CROSS_COMPILE= EXTRA_CFLAGS= NO_LIBELF=1 -C /git/linux/tools/perf O=/tmp/build/perf CC=clang
> + make ARCH= CROSS_COMPILE= EXTRA_CFLAGS= LIBCLANGLLVM=1 -C /git/linux/tools/perf O=/tmp/build/perf CC=clang
> + make ARCH= CROSS_COMPILE= EXTRA_CFLAGS= LIBCLANGLLVM=1 -C /git/linux/tools/perf O=/tmp/build/perf
> [perfbuilder@quaco linux-perf-tools-build]$
>
> Probably all the rest will go well, will let you know.
>
> Daniel, do you mind if I carry this one in my perf/core branch? Its
> small and shouldn't clash with other patches, I think. It should go
> upstream soon:
>
> Andrii, there are these others:
I took a look at them, but I think it would be better, if you could
post them as proper patches to
bpf@vger.kernel.org/netdev@vger.kernel.org, so that others can check
and comment, if necessary.
One nit for all three of them: we typically prefix subject with just
"libbpf: " instead of "tools lib libbpf".
>
> 8dfb6ed300bf tools lib bpf: Avoid designated initializers for unnamed union members
+ attr.sample_period = attr.wakeup_events = 1;
let's instead
+ attr.sample_period = 1;
+ attr.wakeup_events = 1;
I don't like multi-assignments.
Also, if we are doing explicit assignment, let's do it for all the
fields, not split initialization like that.
> 80f7f8f21441 tools lib bpf: Avoid using 'link' as it shadows a global definition in some systems
For this one I'm confused. What compiler/system you are getting it on?
I tried to reproduce it with this example (trying both global
variable, as well as function):
#include <stdio.h>
//int link = 1;
void link() {}
int f(int link) {
return link;
}
int main() {
printf("%d\n", f(123));
return 0;
}
I haven't gotten any errors nor warnings. I'm certainly liking
existing naming better, but my main concern is that we'll probably add
more code like this, and we'll forget about this problem and will
re-introduce.
So I'd rather figure out why it's happening and some rare system and
see if we can mitigate that without all the renames.
> d93fe741e291 tools lib bpf: Fix endianness macro usage for some compilers
This one looks good to me, thanks!
Acked-by: Andrii Nakryiko <andriin@fb.com>
>
> If you could take a look at them at my tmp.perf/core branch at:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/log/?h=tmp.perf/core
>
> I'm force pushing it now to replace my __WORDSIZE patch with yours, the
> SHAs should be the 3 above and the one below.
>
> And to build cleanly I had to cherry pick this one:
>
> 3091dafc1884 (HEAD -> perf/core) libbpf: fix ptr to u64 conversion warning on 32-bit platforms
>
> Alpine:3.5 just finished:
>
> 2 alpine:3.5 : Ok gcc (Alpine 6.2.1) 6.2.1 20160822, clang version 3.8.1 (tags/RELEASE_381/final)
>
> - Arnaldo
^ permalink raw reply
* Re: [PATCH] be2net: fix adapter->big_page_size miscaculation
From: Nick Desaulniers @ 2019-07-18 21:10 UTC (permalink / raw)
To: Qian Cai, Bill Wendling, James Y Knight
Cc: David Miller, sathya.perla, ajit.khaparde, sriharsha.basavapatna,
somnath.kotur, Arnd Bergmann, David Howells, H. Peter Anvin,
netdev, linux-arch, LKML, Nathan Chancellor
In-Reply-To: <D7E57421-A6F4-4453-878A-8F173A856296@lca.pw>
On Thu, Jul 18, 2019 at 2:01 PM Qian Cai <cai@lca.pw> wrote:
>
>
>
> > On Jul 12, 2019, at 8:50 PM, David Miller <davem@davemloft.net> wrote:
> >
> > From: Qian Cai <cai@lca.pw>
> > Date: Fri, 12 Jul 2019 20:27:09 -0400
> >
> >> Actually, GCC would consider it a const with -O2 optimized level because it found that it was never modified and it does not understand it is a module parameter. Considering the following code.
> >>
> >> # cat const.c
> >> #include <stdio.h>
> >>
> >> static int a = 1;
> >>
> >> int main(void)
> >> {
> >> if (__builtin_constant_p(a))
> >> printf("a is a const.\n");
> >>
> >> return 0;
> >> }
> >>
> >> # gcc -O2 const.c -o const
> >
> > That's not a complete test case, and with a proper test case that
> > shows the externalization of the address of &a done by the module
> > parameter macros, gcc should not make this optimization or we should
> > define the module parameter macros in a way that makes this properly
> > clear to the compiler.
> >
> > It makes no sense to hack around this locally in drivers and other
> > modules.
>
> If you see the warning in the original patch,
>
> https://lore.kernel.org/netdev/1562959401-19815-1-git-send-email-cai@lca.pw/
>
> GCC definitely optimize rx_frag_size to be a constant while I just confirmed clang
> -O2 does not. The problem is that I have no clue about how to let GCC not to
> optimize a module parameter.
>
> Though, I have added a few people who might know more of compilers than myself.
+ Bill and James, who probably knows more than they'd like to about
__builtin_constant_p and more than other LLVM folks at this point.
--
Thanks,
~Nick Desaulniers
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox