* [PATCH] Bluetooth: remove redundant variables 'adv_set' and 'cp'
From: YueHaibing @ 2018-08-06 11:08 UTC (permalink / raw)
To: marcel, johan.hedberg
Cc: linux-kernel, netdev, davem, linux-bluetooth, YueHaibing
Variables 'adv_set' and 'cp' are being assigned but are never used hence
they are redundant and can be removed.
Cleans up clang warnings:
net/bluetooth/hci_event.c:1135:29: warning: variable 'adv_set' set but not used [-Wunused-but-set-variable]
net/bluetooth/mgmt.c:3359:39: warning: variable 'cp' set but not used [-Wunused-but-set-variable]
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
net/bluetooth/hci_event.c | 3 ---
net/bluetooth/mgmt.c | 3 ---
2 files changed, 6 deletions(-)
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 754714c..8078587 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1132,7 +1132,6 @@ static void hci_cc_le_set_ext_adv_enable(struct hci_dev *hdev,
struct sk_buff *skb)
{
struct hci_cp_le_set_ext_adv_enable *cp;
- struct hci_cp_ext_adv_set *adv_set;
__u8 status = *((__u8 *) skb->data);
BT_DBG("%s status 0x%2.2x", hdev->name, status);
@@ -1144,8 +1143,6 @@ static void hci_cc_le_set_ext_adv_enable(struct hci_dev *hdev,
if (!cp)
return;
- adv_set = (void *) cp->data;
-
hci_dev_lock(hdev);
if (cp->enable) {
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 231602f7..3bdc8f3 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -3356,7 +3356,6 @@ int mgmt_phy_configuration_changed(struct hci_dev *hdev, struct sock *skip)
static void set_default_phy_complete(struct hci_dev *hdev, u8 status,
u16 opcode, struct sk_buff *skb)
{
- struct mgmt_cp_set_phy_confguration *cp;
struct mgmt_pending_cmd *cmd;
BT_DBG("status 0x%02x", status);
@@ -3367,8 +3366,6 @@ static void set_default_phy_complete(struct hci_dev *hdev, u8 status,
if (!cmd)
goto unlock;
- cp = cmd->param;
-
if (status) {
mgmt_cmd_status(cmd->sk, hdev->id,
MGMT_OP_SET_PHY_CONFIGURATION,
--
2.7.0
^ permalink raw reply related
* Re: [stmmac][bug?] endianness of Flexible RX Parser code
From: Jose Abreu @ 2018-08-06 8:56 UTC (permalink / raw)
To: Al Viro, Jose Abreu; +Cc: David S. Miller, netdev
In-Reply-To: <20180804011952.GD30522@ZenIV.linux.org.uk>
Hi Al,
On 04-08-2018 02:19, Al Viro wrote:
> The values passed in struct tc_u32_sel ->mask and ->val are
> 32bit net-endian. Your tc_fill_entry() does this:
>
> data = sel->keys[0].val;
> mask = sel->keys[0].mask;
>
> ...
> entry->frag_ptr = frag;
> entry->val.match_en = (mask << (rem * 8)) &
> GENMASK(31, rem * 8);
> entry->val.match_data = (data << (rem * 8)) &
> GENMASK(31, rem * 8);
> entry->val.frame_offset = real_off;
> entry->prio = prio;
>
> frag->val.match_en = (mask >> (rem * 8)) &
> GENMASK(rem * 8 - 1, 0);
> frag->val.match_data = (data >> (rem * 8)) &
> GENMASK(rem * 8 - 1, 0);
> frag->val.frame_offset = real_off + 1;
> frag->prio = prio;
> frag->is_frag = true;
>
> and that looks very odd. rem here is offset modulo 4. Suppose offset is
> equal to 5, val contains {V0, V1, V2, V3} and mask - {M0, M1, M2, M3}.
> Then on little-endian host we get
> entry->val.match_en: {0, M0, M1, M2}
> entry->val.match_data: {0, V0, V1, V2}
> entry->val.frame_offset = 1;
> frag->val.match_en: {M3, 0, 0, 0}
> frag->val.match_data: {V3, 0, 0, 0}
> frag->val.frame_offset = 2;
> and on big-endian
> entry->val.match_en: {M1, M2, M3, 0}
> entry->val.match_data: {V1, V2, V3, 0}
> entry->val.frame_offset = 1;
> frag->val.match_en: {0, 0, 0, M0}
> frag->val.match_data: {0, 0, 0, V0}
> frag->val.frame_offset = 2;
>
> Little-endian variant looks like we mask octets 5, 6, 7 and 8 with
> M0..M3 resp. and want V0..V3 in those. On big-endian, though, we
> look at the octets 11, 4, 5 and 6 instead.
>
> I don't know the hardware (and it might be pulling any kind of weird
> endianness-dependent stunts), but that really smells like a bug.
There is a feature in HW that supports Byte-Invariant Big-Endian
data transfer. It's not activated by default though ...
> It looks like that code is trying to do something like
>
> data = ntohl(sel->keys[0].val);
> mask = ntohl(sel->keys[0].mask);
> shift = rem * 8;
>
> entry->val.match_en = htonl(mask >> shift);
> entry->val.match_data = htonl(data >> shift);
> entry->val.frame_offset = real_off;
> ...
> frag->val.match_en = htonl(mask << (32 - shift));
> frag->val.match_data = htonl(data << (32 - shift));
> entry->val.frame_offset = real_off + 1;
>
> Comments?
Looks good. Can you send a formal patch and a simple command that
I can use to validate this situation?
I used at the time at least two commands: one for validating
simple match by using 1 entry (i.e. plain match) and another to
validate 2 entries (i.e. two matches) and did not encounter any
problem ...
Thanks and Best Regards,
Jose Miguel Abreu
^ permalink raw reply
* [PATCH net-next-internal] net: sched: cls_flower: set correct offload data in fl_reoffload
From: Vlad Buslov @ 2018-08-06 8:27 UTC (permalink / raw)
To: netdev; +Cc: davem, jhs, xiyou.wangcong, jiri, Vlad Buslov
fl_reoffload implementation sets following members of struct
tc_cls_flower_offload incorrectly:
- masked key instead of mask
- key instead of masked key
Fix fl_reoffload to provide correct data to offload callback.
Fixes: 31533cba4327 ("net: sched: cls_flower: implement offload tcf_proto_op")
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
---
net/sched/cls_flower.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index e8bd08ba998a..cbf165c33656 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -1174,8 +1174,8 @@ static int fl_reoffload(struct tcf_proto *tp, bool add, tc_setup_cb_t *cb,
TC_CLSFLOWER_REPLACE : TC_CLSFLOWER_DESTROY;
cls_flower.cookie = (unsigned long)f;
cls_flower.dissector = &mask->dissector;
- cls_flower.mask = &f->mkey;
- cls_flower.key = &f->key;
+ cls_flower.mask = &mask->key;
+ cls_flower.key = &f->mkey;
cls_flower.exts = &f->exts;
cls_flower.classid = f->res.classid;
--
2.7.5
^ permalink raw reply related
* Re: [PATCH v8 bpf-next 00/10] veth: Driver XDP
From: Björn Töpel @ 2018-08-06 8:22 UTC (permalink / raw)
To: Jesper Dangaard Brouer, Toshiaki Makita
Cc: Alexei Starovoitov, Daniel Borkmann, netdev, Jakub Kicinski,
John Fastabend, Tariq Toukan, Björn Töpel,
intel-wired-lan
In-Reply-To: <20180803114538.382664c8@redhat.com>
On 2018-08-03 11:45, Jesper Dangaard Brouer wrote:
> On Fri, 3 Aug 2018 16:58:08 +0900
> Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> wrote:
>
>> This patch set introduces driver XDP for veth.
>> Basically this is used in conjunction with redirect action of another XDP
>> program.
>>
>> NIC -----------> veth===veth
>> (XDP) (redirect) (XDP)
>>
>
> I'm was playing with V7 on my testlab yesterday and I noticed one
> fundamental issue. You are not updating the "ifconfig" stats counters,
> when in XDP mode. This makes receive or send via XDP invisible to
> sysadm/management tools. This for-sure is going to cause confusion...
>
> I took a closer look at other driver. The ixgbe driver is doing the
> right thing. Driver i40e have a bug, where RX/TX stats are swapped
> getting (strange!).
Indeed! Thanks for finding/reporting this! I'll have look!
Björn
> The mlx5 driver is not updating the regular RX/TX
> counters, but A LOT of other ethtool stats counters (which are the ones
> I usually monitor when testing).
>
> So, given other drivers also didn't get this right, we need to have a
> discussion outside your/this patchset. Thus, I don't want to
> stop/stall this patchset, but this is something we need to fixup in a
> followup patchset to other drivers as well.
>
> Thus, I'm acking the patchset, but I request that we do a joint effort
> of fixing this as followup patches.
>
> Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>
>
^ permalink raw reply
* [net-next:master 457/471] ERROR: "__aeabi_ldivmod" [drivers/ptp/ptp_qoriq.ko] undefined!
From: kbuild test robot @ 2018-08-06 8:07 UTC (permalink / raw)
To: Yangbo Lu; +Cc: kbuild-all, netdev
[-- Attachment #1: Type: text/plain, Size: 1000 bytes --]
tree: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git master
head: 981467033a37d916649647fa3afe1fe99bba1817
commit: 91305f2812624c0cf7ccbb44133b66d3b24676e4 [457/471] ptp_qoriq: support automatic configuration for ptp timer
config: arm-allmodconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
git checkout 91305f2812624c0cf7ccbb44133b66d3b24676e4
# save the attached .config to linux build tree
GCC_VERSION=7.2.0 make.cross ARCH=arm
All errors (new ones prefixed by >>):
>> ERROR: "__aeabi_ldivmod" [drivers/ptp/ptp_qoriq.ko] undefined!
>> ERROR: "__aeabi_uldivmod" [drivers/ptp/ptp_qoriq.ko] undefined!
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 66173 bytes --]
^ permalink raw reply
* Re: [PATCH v3 net-next 5/9] net: stmmac: Add MDIO related functions for XGMAC2
From: Jose Abreu @ 2018-08-06 7:59 UTC (permalink / raw)
To: Florian Fainelli, Jose Abreu, netdev
Cc: David S. Miller, Joao Pinto, Giuseppe Cavallaro, Alexandre Torgue,
Andrew Lunn
In-Reply-To: <01fc5766-d3ad-3056-6c6c-f9a6d603f87e@gmail.com>
On 03-08-2018 20:06, Florian Fainelli wrote:
> On 08/03/2018 08:50 AM, Jose Abreu wrote:
>> Add the MDIO related funcionalities for the new IP block XGMAC2.
>>
>> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
>> Cc: David S. Miller <davem@davemloft.net>
>> Cc: Joao Pinto <jpinto@synopsys.com>
>> Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>
>> Cc: Alexandre Torgue <alexandre.torgue@st.com>
>> Cc: Andrew Lunn <andrew@lunn.ch>
>> ---
>> +satic int stmmac_xgmac2_c22_format(struct stmmac_priv *priv, int phyaddr,
>> + int phyreg, u32 *hw_addr)
>> +{
>> + unsigned int mii_data = priv->hw->mii.data;
>> + u32 tmp;
>> +
>> + /* HW does not support C22 addr >= 4 */
>> + if (phyaddr >= 4)
>> + return -ENODEV;
> It would be nice if this could be moved at probe time so you don't have
> to wait until you connect to the PHY, read its PHY OUI and find out it
> has a MDIO address >= 4. Not a blocker, but something that could be
> improved further on.
>
> In premise you could even scan the MDIO bus' device tree node, and find
> that out ahead of time.
Oh, but I use phylib ... I only provide the read/write callbacks
so I think we should avoid duplicating the code that's already in
the phylib ... No?
>
>> + /* Wait until any existing MII operation is complete */
>> + if (readl_poll_timeout(priv->ioaddr + mii_data, tmp,
>> + !(tmp & MII_XGMAC_BUSY), 100, 10000))
>> + return -EBUSY;
>> +
>> + /* Set port as Clause 22 */
>> + tmp = readl(priv->ioaddr + XGMAC_MDIO_C22P);
>> + tmp |= BIT(phyaddr);
> Since the registers are being Read/Modify/Write here, don't you need to
> clear the previous address bits as well?
>
> You probably did not encounter any problems in your testing if you had
> only one PHY on the MDIO bus, but this is not something that is
> necessarily true, e.g: if you have an Ethernet switch, several MDIO bus
> addresses are going to be responding.
>
> Your MDIO bus implementation must be able to support one transaction
> with one PHY address and the next transaction with another PHY address ,
> etc...
>
> That is something that should be easy to fix and be resubmitted as part
> of v4.
I'm not following you here... I only set/unset the bit for the
corresponding phyaddr that phylib wants to read/write. Why would
I clear the remaining addresses?
Thanks and Best Regards,
Jose Miguel Abreu
^ permalink raw reply
* Re: [PATCH v3 net-next 3/9] net: stmmac: Add DMA related callbacks for XGMAC2
From: Jose Abreu @ 2018-08-06 7:56 UTC (permalink / raw)
To: Florian Fainelli, Jose Abreu, netdev
Cc: David S. Miller, Joao Pinto, Giuseppe Cavallaro, Alexandre Torgue
In-Reply-To: <dc666664-d93e-0408-cfd7-86b271191d3e@gmail.com>
On 03-08-2018 19:58, Florian Fainelli wrote:
> On 08/03/2018 08:50 AM, Jose Abreu wrote:
>> Add the DMA related callbacks for the new IP block XGMAC2.
>>
>> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
>> Cc: David S. Miller <davem@davemloft.net>
>> Cc: Joao Pinto <jpinto@synopsys.com>
>> Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>
>> Cc: Alexandre Torgue <alexandre.torgue@st.com>
>> ---
>> + value &= ~XGMAC_RD_OSR_LMT;
>> + value |= (axi->axi_rd_osr_lmt << XGMAC_RD_OSR_LMT_SHIFT) &
>> + XGMAC_RD_OSR_LMT;
>> +
>> + for (i = 0; i < AXI_BLEN; i++) {
>> + if (axi->axi_blen[i])
>> + value &= ~XGMAC_UNDEF;
> Should not you be you clearing all XGMAC_BLEN* values since you do a
> logical or here? I am assuming this is not something that would likely
> change from one open/close but still?
Yeah, this won't change between open/close but I will add the
mask anyway.
Thanks and Best Regards,
Jose Miguel Abreu
^ permalink raw reply
* Re: [PATCH v3 net-next 1/9] net: stmmac: Add XGMAC 2.10 HWIF entry
From: Jose Abreu @ 2018-08-06 7:55 UTC (permalink / raw)
To: Florian Fainelli, Jose Abreu, netdev
Cc: David S. Miller, Joao Pinto, Giuseppe Cavallaro, Alexandre Torgue
In-Reply-To: <4f309bf8-c669-ae9d-750c-8946715f0fe1@gmail.com>
Hi Florian,
On 03-08-2018 19:54, Florian Fainelli wrote:
> On 08/03/2018 08:50 AM, Jose Abreu wrote:
>> @@ -87,6 +88,7 @@ static const struct stmmac_hwif_entry {
>> {
>> .gmac = false,
>> .gmac4 = false,
>> + .xgmac = false,
> In a future clean-up you would like want to remove this and replace this
> an enumeration which is less error prone than having to define a boolean
> for each of these previous generations only to say "this is not an xgmac".
Its a good idea! I really don't like the pattern of "if
(priv->plat->has_something)" in the code. I will think about
adding this but for now let's merge xgmac2 and I will try to
refactor the code later because I will need to change many files
and the patchset would be bigger.
Thanks and Best Regards,
Jose Miguel Abreu
^ permalink raw reply
* Re: [PATCH] net: ieee802154: 6lowpan: remove redundant pointers 'fq' and 'net'
From: Stefan Schmidt @ 2018-08-06 9:10 UTC (permalink / raw)
To: Colin King, Alexander Aring, David S . Miller, linux-wpan, netdev
Cc: kernel-janitors, linux-kernel
In-Reply-To: <20180731154507.5247-1-colin.king@canonical.com>
Hello.
On 07/31/2018 05:45 PM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> Pointers fq and net are being assigned but are never used hence they
> are redundant and can be removed.
>
> Cleans up clang warnings:
> warning: variable 'fq' set but not used [-Wunused-but-set-variable]
> warning: variable 'net' set but not used [-Wunused-but-set-variable]
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> net/ieee802154/6lowpan/reassembly.c | 5 -----
> 1 file changed, 5 deletions(-)
>
> diff --git a/net/ieee802154/6lowpan/reassembly.c b/net/ieee802154/6lowpan/reassembly.c
> index ec7a5da56129..e7857a8ac86d 100644
> --- a/net/ieee802154/6lowpan/reassembly.c
> +++ b/net/ieee802154/6lowpan/reassembly.c
> @@ -40,9 +40,6 @@ static int lowpan_frag_reasm(struct lowpan_frag_queue *fq,
> static void lowpan_frag_init(struct inet_frag_queue *q, const void *a)
> {
> const struct frag_lowpan_compare_key *key = a;
> - struct lowpan_frag_queue *fq;
> -
> - fq = container_of(q, struct lowpan_frag_queue, q);
>
> BUILD_BUG_ON(sizeof(*key) > sizeof(q->key));
> memcpy(&q->key, key, sizeof(*key));
> @@ -52,10 +49,8 @@ static void lowpan_frag_expire(struct timer_list *t)
> {
> struct inet_frag_queue *frag = from_timer(frag, t, timer);
> struct frag_queue *fq;
> - struct net *net;
>
> fq = container_of(frag, struct frag_queue, q);
> - net = container_of(fq->q.net, struct net, ieee802154_lowpan.frags);
>
> spin_lock(&fq->q.lock);
>
>
This patch has been applied to the wpan-next tree and will be
part of the next pull request to net-next. Thanks!
regards
Stefan Schmidt
^ permalink raw reply
* Re: [PATCH] ieee802154: add rx LQI from userspace
From: Stefan Schmidt @ 2018-08-06 9:07 UTC (permalink / raw)
To: Clément Péron, Romuald Cari, linux-wpan
Cc: Alexander Aring, Stefan Schmidt, David S . Miller, netdev,
linux-kernel, Clément Peron
In-Reply-To: <20180607140802.22666-1-peron.clem@gmail.com>
Hello.
On 06/07/2018 04:08 PM, Clément Péron wrote:
> From: Romuald CARI <romuald.cari@devialet.com>
>
> The Link Quality Indication data exposed by drivers could not be accessed from
> userspace. Since this data is per-datagram received, it makes sense to make it
> available to userspace application through the ancillary data mechanism in
> recvmsg rather than through ioctls. This can be activated using the socket
> option WPAN_WANTLQI under SOL_IEEE802154 protocol.
>
> This LQI data is available in the ancillary data buffer under the SOL_IEEE802154
> level as the type WPAN_LQI. The value is an unsigned byte indicating the link
> quality with values ranging 0-255.
>
> Signed-off-by: Romuald Cari <romuald.cari@devialet.com>
> Signed-off-by: Clément Peron <clement.peron@devialet.com>
> ---
> include/net/af_ieee802154.h | 1 +
> net/ieee802154/socket.c | 17 +++++++++++++++++
> 2 files changed, 18 insertions(+)
>
> diff --git a/include/net/af_ieee802154.h b/include/net/af_ieee802154.h
> index a5563d27a3eb..8003a9f6eb43 100644
> --- a/include/net/af_ieee802154.h
> +++ b/include/net/af_ieee802154.h
> @@ -56,6 +56,7 @@ struct sockaddr_ieee802154 {
> #define WPAN_WANTACK 0
> #define WPAN_SECURITY 1
> #define WPAN_SECURITY_LEVEL 2
> +#define WPAN_WANTLQI 3
>
> #define WPAN_SECURITY_DEFAULT 0
> #define WPAN_SECURITY_OFF 1
> diff --git a/net/ieee802154/socket.c b/net/ieee802154/socket.c
> index a60658c85a9a..bc6b912603f1 100644
> --- a/net/ieee802154/socket.c
> +++ b/net/ieee802154/socket.c
> @@ -25,6 +25,7 @@
> #include <linux/termios.h> /* For TIOCOUTQ/INQ */
> #include <linux/list.h>
> #include <linux/slab.h>
> +#include <linux/socket.h>
> #include <net/datalink.h>
> #include <net/psnap.h>
> #include <net/sock.h>
> @@ -452,6 +453,7 @@ struct dgram_sock {
> unsigned int bound:1;
> unsigned int connected:1;
> unsigned int want_ack:1;
> + unsigned int want_lqi:1;
> unsigned int secen:1;
> unsigned int secen_override:1;
> unsigned int seclevel:3;
> @@ -486,6 +488,7 @@ static int dgram_init(struct sock *sk)
> struct dgram_sock *ro = dgram_sk(sk);
>
> ro->want_ack = 1;
> + ro->want_lqi = 0;
> return 0;
> }
>
> @@ -713,6 +716,7 @@ static int dgram_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
> size_t copied = 0;
> int err = -EOPNOTSUPP;
> struct sk_buff *skb;
> + struct dgram_sock *ro = dgram_sk(sk);
> DECLARE_SOCKADDR(struct sockaddr_ieee802154 *, saddr, msg->msg_name);
>
> skb = skb_recv_datagram(sk, flags, noblock, &err);
> @@ -744,6 +748,13 @@ static int dgram_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
> *addr_len = sizeof(*saddr);
> }
>
> + if (ro->want_lqi) {
> + err = put_cmsg(msg, SOL_IEEE802154, WPAN_WANTLQI,
> + sizeof(uint8_t), &(mac_cb(skb)->lqi));
> + if (err)
> + goto done;
> + }
> +
> if (flags & MSG_TRUNC)
> copied = skb->len;
> done:
> @@ -847,6 +858,9 @@ static int dgram_getsockopt(struct sock *sk, int level, int optname,
> case WPAN_WANTACK:
> val = ro->want_ack;
> break;
> + case WPAN_WANTLQI:
> + val = ro->want_lqi;
> + break;
> case WPAN_SECURITY:
> if (!ro->secen_override)
> val = WPAN_SECURITY_DEFAULT;
> @@ -892,6 +906,9 @@ static int dgram_setsockopt(struct sock *sk, int level, int optname,
> case WPAN_WANTACK:
> ro->want_ack = !!val;
> break;
> + case WPAN_WANTLQI:
> + ro->want_lqi = !!val;
> + break;
> case WPAN_SECURITY:
> if (!ns_capable(net->user_ns, CAP_NET_ADMIN) &&
> !ns_capable(net->user_ns, CAP_NET_RAW)) {
>
This patch has been applied to the wpan-next tree and will be
part of the next pull request to net-next. Thanks!
regards
Stefan Schmidt
^ permalink raw reply
* [PATCH net-next 13/14] net: core: add new/replace rate estimator lock parameter
From: Vlad Buslov @ 2018-08-06 6:54 UTC (permalink / raw)
To: netdev
Cc: davem, jhs, xiyou.wangcong, jiri, pablo, kadlec, fw, ast, daniel,
edumazet, keescook, marcelo.leitner, Vlad Buslov
In-Reply-To: <1533538465-23199-1-git-send-email-vladbu@mellanox.com>
Extend rate estimator 'new' and 'replace' APIs with additional spinlock
parameter to be used by rtnl-unlocked actions to protect rate_est pointer
from concurrent modification.
Extract code that requires synchronization from gen_new_estimator into
__replace_estimator function in order to be used by both locked and
unlocked paths.
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
include/net/gen_stats.h | 2 ++
net/core/gen_estimator.c | 58 +++++++++++++++++++++++++++++++++++-----------
net/netfilter/xt_RATEEST.c | 2 +-
net/sched/act_api.c | 2 +-
net/sched/act_police.c | 2 +-
net/sched/sch_api.c | 2 ++
net/sched/sch_cbq.c | 4 ++--
net/sched/sch_drr.c | 4 ++--
net/sched/sch_hfsc.c | 4 ++--
net/sched/sch_htb.c | 4 ++--
net/sched/sch_qfq.c | 4 ++--
11 files changed, 61 insertions(+), 27 deletions(-)
diff --git a/include/net/gen_stats.h b/include/net/gen_stats.h
index 0304ba2ae353..d1ef63d16abc 100644
--- a/include/net/gen_stats.h
+++ b/include/net/gen_stats.h
@@ -59,12 +59,14 @@ int gnet_stats_finish_copy(struct gnet_dump *d);
int gen_new_estimator(struct gnet_stats_basic_packed *bstats,
struct gnet_stats_basic_cpu __percpu *cpu_bstats,
struct net_rate_estimator __rcu **rate_est,
+ spinlock_t *rate_est_lock,
spinlock_t *stats_lock,
seqcount_t *running, struct nlattr *opt);
void gen_kill_estimator(struct net_rate_estimator __rcu **ptr);
int gen_replace_estimator(struct gnet_stats_basic_packed *bstats,
struct gnet_stats_basic_cpu __percpu *cpu_bstats,
struct net_rate_estimator __rcu **ptr,
+ spinlock_t *rate_est_lock,
spinlock_t *stats_lock,
seqcount_t *running, struct nlattr *opt);
bool gen_estimator_active(struct net_rate_estimator __rcu **ptr);
diff --git a/net/core/gen_estimator.c b/net/core/gen_estimator.c
index 98fd12721221..012e37011147 100644
--- a/net/core/gen_estimator.c
+++ b/net/core/gen_estimator.c
@@ -107,11 +107,43 @@ static void est_timer(struct timer_list *t)
mod_timer(&est->timer, est->next_jiffies);
}
+static void __replace_estimator(struct net_rate_estimator __rcu **rate_est,
+ struct net_rate_estimator *new)
+{
+ struct net_rate_estimator *old = rcu_dereference_protected(*rate_est,
+ 1);
+
+ if (old) {
+ del_timer_sync(&old->timer);
+ new->avbps = old->avbps;
+ new->avpps = old->avpps;
+ }
+
+ new->next_jiffies = jiffies + ((HZ / 4) << new->intvl_log);
+ timer_setup(&new->timer, est_timer, 0);
+ mod_timer(&new->timer, new->next_jiffies);
+
+ rcu_assign_pointer(*rate_est, new);
+
+ if (old)
+ kfree_rcu(old, rcu);
+}
+
+static void replace_estimator(struct net_rate_estimator __rcu **rate_est,
+ struct net_rate_estimator *new,
+ spinlock_t *rate_est_lock)
+{
+ spin_lock_bh(rate_est_lock);
+ __replace_estimator(rate_est, new);
+ spin_unlock_bh(rate_est_lock);
+}
+
/**
* gen_new_estimator - create a new rate estimator
* @bstats: basic statistics
* @cpu_bstats: bstats per cpu
* @rate_est: rate estimator statistics
+ * @rate_est_lock: rate_est lock (might be NULL)
* @stats_lock: statistics lock
* @running: qdisc running seqcount
* @opt: rate estimator configuration TLV
@@ -128,12 +160,13 @@ static void est_timer(struct timer_list *t)
int gen_new_estimator(struct gnet_stats_basic_packed *bstats,
struct gnet_stats_basic_cpu __percpu *cpu_bstats,
struct net_rate_estimator __rcu **rate_est,
+ spinlock_t *rate_est_lock,
spinlock_t *stats_lock,
seqcount_t *running,
struct nlattr *opt)
{
struct gnet_estimator *parm = nla_data(opt);
- struct net_rate_estimator *old, *est;
+ struct net_rate_estimator *est;
struct gnet_stats_basic_packed b;
int intvl_log;
@@ -167,20 +200,15 @@ int gen_new_estimator(struct gnet_stats_basic_packed *bstats,
local_bh_enable();
est->last_bytes = b.bytes;
est->last_packets = b.packets;
- old = rcu_dereference_protected(*rate_est, 1);
- if (old) {
- del_timer_sync(&old->timer);
- est->avbps = old->avbps;
- est->avpps = old->avpps;
- }
- est->next_jiffies = jiffies + ((HZ/4) << intvl_log);
- timer_setup(&est->timer, est_timer, 0);
- mod_timer(&est->timer, est->next_jiffies);
+ if (rate_est_lock)
+ replace_estimator(rate_est, est, rate_est_lock);
+ else
+ /* If no spinlock argument provided,
+ * then assume that caller is already synchronized.
+ */
+ __replace_estimator(rate_est, est);
- rcu_assign_pointer(*rate_est, est);
- if (old)
- kfree_rcu(old, rcu);
return 0;
}
EXPORT_SYMBOL(gen_new_estimator);
@@ -209,6 +237,7 @@ EXPORT_SYMBOL(gen_kill_estimator);
* @bstats: basic statistics
* @cpu_bstats: bstats per cpu
* @rate_est: rate estimator statistics
+ * @rate_est_lock: rate_est lock (might be NULL)
* @stats_lock: statistics lock
* @running: qdisc running seqcount (might be NULL)
* @opt: rate estimator configuration TLV
@@ -221,10 +250,11 @@ EXPORT_SYMBOL(gen_kill_estimator);
int gen_replace_estimator(struct gnet_stats_basic_packed *bstats,
struct gnet_stats_basic_cpu __percpu *cpu_bstats,
struct net_rate_estimator __rcu **rate_est,
+ spinlock_t *rate_est_lock,
spinlock_t *stats_lock,
seqcount_t *running, struct nlattr *opt)
{
- return gen_new_estimator(bstats, cpu_bstats, rate_est,
+ return gen_new_estimator(bstats, cpu_bstats, rate_est, rate_est_lock,
stats_lock, running, opt);
}
EXPORT_SYMBOL(gen_replace_estimator);
diff --git a/net/netfilter/xt_RATEEST.c b/net/netfilter/xt_RATEEST.c
index dec843cadf46..8e79bd50bc0b 100644
--- a/net/netfilter/xt_RATEEST.c
+++ b/net/netfilter/xt_RATEEST.c
@@ -154,7 +154,7 @@ static int xt_rateest_tg_checkentry(const struct xt_tgchk_param *par)
cfg.est.interval = info->interval;
cfg.est.ewma_log = info->ewma_log;
- ret = gen_new_estimator(&est->bstats, NULL, &est->rate_est,
+ ret = gen_new_estimator(&est->bstats, NULL, &est->rate_est, NULL,
&est->lock, NULL, &cfg.opt);
if (ret < 0)
goto err2;
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 229d63c99be2..28c2d41f0cd2 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -401,7 +401,7 @@ int tcf_idr_create(struct tc_action_net *tn, u32 index, struct nlattr *est,
p->tcfa_tm.firstuse = 0;
if (est) {
err = gen_new_estimator(&p->tcfa_bstats, p->cpu_bstats,
- &p->tcfa_rate_est,
+ &p->tcfa_rate_est, NULL,
&p->tcfa_lock, NULL, est);
if (err)
goto err3;
diff --git a/net/sched/act_police.c b/net/sched/act_police.c
index 1f3192ea8df7..3eb5fe60c62c 100644
--- a/net/sched/act_police.c
+++ b/net/sched/act_police.c
@@ -138,7 +138,7 @@ static int tcf_act_police_init(struct net *net, struct nlattr *nla,
if (est) {
err = gen_replace_estimator(&police->tcf_bstats, NULL,
- &police->tcf_rate_est,
+ &police->tcf_rate_est, NULL,
&police->tcf_lock,
NULL, est);
if (err)
diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index 98541c6399db..57cdc105ce30 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -1185,6 +1185,7 @@ static struct Qdisc *qdisc_create(struct net_device *dev,
sch->cpu_bstats,
&sch->rate_est,
NULL,
+ NULL,
running,
tca[TCA_RATE]);
if (err) {
@@ -1260,6 +1261,7 @@ static int qdisc_change(struct Qdisc *sch, struct nlattr **tca,
sch->cpu_bstats,
&sch->rate_est,
NULL,
+ NULL,
qdisc_root_sleeping_running(sch),
tca[TCA_RATE]);
}
diff --git a/net/sched/sch_cbq.c b/net/sched/sch_cbq.c
index f42025d53cfe..2a7ff53b4e7a 100644
--- a/net/sched/sch_cbq.c
+++ b/net/sched/sch_cbq.c
@@ -1503,7 +1503,7 @@ cbq_change_class(struct Qdisc *sch, u32 classid, u32 parentid, struct nlattr **t
if (tca[TCA_RATE]) {
err = gen_replace_estimator(&cl->bstats, NULL,
- &cl->rate_est,
+ &cl->rate_est, NULL,
NULL,
qdisc_root_sleeping_running(sch),
tca[TCA_RATE]);
@@ -1605,7 +1605,7 @@ cbq_change_class(struct Qdisc *sch, u32 classid, u32 parentid, struct nlattr **t
if (tca[TCA_RATE]) {
err = gen_new_estimator(&cl->bstats, NULL, &cl->rate_est,
- NULL,
+ NULL, NULL,
qdisc_root_sleeping_running(sch),
tca[TCA_RATE]);
if (err) {
diff --git a/net/sched/sch_drr.c b/net/sched/sch_drr.c
index e0b0cf8a9939..0896e239063b 100644
--- a/net/sched/sch_drr.c
+++ b/net/sched/sch_drr.c
@@ -95,7 +95,7 @@ static int drr_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
if (cl != NULL) {
if (tca[TCA_RATE]) {
err = gen_replace_estimator(&cl->bstats, NULL,
- &cl->rate_est,
+ &cl->rate_est, NULL,
NULL,
qdisc_root_sleeping_running(sch),
tca[TCA_RATE]);
@@ -129,7 +129,7 @@ static int drr_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
if (tca[TCA_RATE]) {
err = gen_replace_estimator(&cl->bstats, NULL, &cl->rate_est,
- NULL,
+ NULL, NULL,
qdisc_root_sleeping_running(sch),
tca[TCA_RATE]);
if (err) {
diff --git a/net/sched/sch_hfsc.c b/net/sched/sch_hfsc.c
index 3278a76f6861..be4ead89b50e 100644
--- a/net/sched/sch_hfsc.c
+++ b/net/sched/sch_hfsc.c
@@ -972,7 +972,7 @@ hfsc_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
if (tca[TCA_RATE]) {
err = gen_replace_estimator(&cl->bstats, NULL,
- &cl->rate_est,
+ &cl->rate_est, NULL,
NULL,
qdisc_root_sleeping_running(sch),
tca[TCA_RATE]);
@@ -1042,7 +1042,7 @@ hfsc_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
if (tca[TCA_RATE]) {
err = gen_new_estimator(&cl->bstats, NULL, &cl->rate_est,
- NULL,
+ NULL, NULL,
qdisc_root_sleeping_running(sch),
tca[TCA_RATE]);
if (err) {
diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
index 43c4bfe625a9..f782a1d31264 100644
--- a/net/sched/sch_htb.c
+++ b/net/sched/sch_htb.c
@@ -1395,7 +1395,7 @@ static int htb_change_class(struct Qdisc *sch, u32 classid,
if (htb_rate_est || tca[TCA_RATE]) {
err = gen_new_estimator(&cl->bstats, NULL,
&cl->rate_est,
- NULL,
+ NULL, NULL,
qdisc_root_sleeping_running(sch),
tca[TCA_RATE] ? : &est.nla);
if (err) {
@@ -1460,7 +1460,7 @@ static int htb_change_class(struct Qdisc *sch, u32 classid,
} else {
if (tca[TCA_RATE]) {
err = gen_replace_estimator(&cl->bstats, NULL,
- &cl->rate_est,
+ &cl->rate_est, NULL,
NULL,
qdisc_root_sleeping_running(sch),
tca[TCA_RATE]);
diff --git a/net/sched/sch_qfq.c b/net/sched/sch_qfq.c
index bb1a9c11fc54..8026c1eebd79 100644
--- a/net/sched/sch_qfq.c
+++ b/net/sched/sch_qfq.c
@@ -461,7 +461,7 @@ static int qfq_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
if (cl != NULL) { /* modify existing class */
if (tca[TCA_RATE]) {
err = gen_replace_estimator(&cl->bstats, NULL,
- &cl->rate_est,
+ &cl->rate_est, NULL,
NULL,
qdisc_root_sleeping_running(sch),
tca[TCA_RATE]);
@@ -488,7 +488,7 @@ static int qfq_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
if (tca[TCA_RATE]) {
err = gen_new_estimator(&cl->bstats, NULL,
&cl->rate_est,
- NULL,
+ NULL, NULL,
qdisc_root_sleeping_running(sch),
tca[TCA_RATE]);
if (err)
--
2.7.5
^ permalink raw reply related
* [PATCH net-next 12/14] net: sched: act_mirred: remove dependency on rtnl lock
From: Vlad Buslov @ 2018-08-06 6:54 UTC (permalink / raw)
To: netdev
Cc: davem, jhs, xiyou.wangcong, jiri, pablo, kadlec, fw, ast, daniel,
edumazet, keescook, marcelo.leitner, Vlad Buslov
In-Reply-To: <1533538465-23199-1-git-send-email-vladbu@mellanox.com>
Re-introduce mirred list spinlock, that was removed some time ago, in order
to protect it from concurrent modifications, instead of relying on rtnl
lock.
Use tcf spinlock to protect mirred action private data from concurrent
modification in init and dump. Rearrange access to mirred data in order to
be performed only while holding the lock.
Rearrange net dev access to always hold reference while working with it,
instead of relying on rntl lock. Change get dev function to increment net
device reference before returning it to caller, instead of assuming that
caller is protected with rtnl lock.
Provide rcu version of mirred dev and tunnel info access functions. (to be
used by unlocked drivers)
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
include/net/tc_act/tc_mirred.h | 5 +++
include/net/tc_act/tc_tunnel_key.h | 33 ++++++++++++---
net/sched/act_mirred.c | 82 +++++++++++++++++++++++++-------------
net/sched/cls_api.c | 1 +
4 files changed, 88 insertions(+), 33 deletions(-)
diff --git a/include/net/tc_act/tc_mirred.h b/include/net/tc_act/tc_mirred.h
index a2e9cbca5c9e..cb30be55e444 100644
--- a/include/net/tc_act/tc_mirred.h
+++ b/include/net/tc_act/tc_mirred.h
@@ -37,4 +37,9 @@ static inline struct net_device *tcf_mirred_dev(const struct tc_action *a)
return rtnl_dereference(to_mirred(a)->tcfm_dev);
}
+static inline struct net_device *tcf_mirred_dev_rcu(const struct tc_action *a)
+{
+ return rcu_dereference(to_mirred(a)->tcfm_dev);
+}
+
#endif /* __NET_TC_MIR_H */
diff --git a/include/net/tc_act/tc_tunnel_key.h b/include/net/tc_act/tc_tunnel_key.h
index 46b8c7f1c8d5..e6e475d788c6 100644
--- a/include/net/tc_act/tc_tunnel_key.h
+++ b/include/net/tc_act/tc_tunnel_key.h
@@ -30,26 +30,47 @@ struct tcf_tunnel_key {
static inline bool is_tcf_tunnel_set(const struct tc_action *a)
{
+ bool ret = false;
#ifdef CONFIG_NET_CLS_ACT
struct tcf_tunnel_key *t = to_tunnel_key(a);
- struct tcf_tunnel_key_params *params = rtnl_dereference(t->params);
+ struct tcf_tunnel_key_params *params;
+ rcu_read_lock();
+ params = rcu_dereference(t->params);
if (a->ops && a->ops->type == TCA_ACT_TUNNEL_KEY)
- return params->tcft_action == TCA_TUNNEL_KEY_ACT_SET;
+ ret = params->tcft_action == TCA_TUNNEL_KEY_ACT_SET;
+ rcu_read_unlock();
#endif
- return false;
+ return ret;
}
static inline bool is_tcf_tunnel_release(const struct tc_action *a)
{
+ bool ret = false;
#ifdef CONFIG_NET_CLS_ACT
struct tcf_tunnel_key *t = to_tunnel_key(a);
- struct tcf_tunnel_key_params *params = rtnl_dereference(t->params);
+ struct tcf_tunnel_key_params *params;
+ rcu_read_lock();
+ params = rcu_dereference(t->params);
if (a->ops && a->ops->type == TCA_ACT_TUNNEL_KEY)
- return params->tcft_action == TCA_TUNNEL_KEY_ACT_RELEASE;
+ ret = params->tcft_action == TCA_TUNNEL_KEY_ACT_RELEASE;
+ rcu_read_unlock();
+#endif
+ return ret;
+}
+
+static inline
+struct ip_tunnel_info *tcf_tunnel_info_rcu(const struct tc_action *a)
+{
+#ifdef CONFIG_NET_CLS_ACT
+ struct tcf_tunnel_key *t = to_tunnel_key(a);
+ struct tcf_tunnel_key_params *params = rcu_dereference(t->params);
+
+ return ¶ms->tcft_enc_metadata->u.tun_info;
+#else
+ return NULL;
#endif
- return false;
}
static inline struct ip_tunnel_info *tcf_tunnel_info(const struct tc_action *a)
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index b26d060da08e..9f622114f5a5 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -30,6 +30,7 @@
#include <net/tc_act/tc_mirred.h>
static LIST_HEAD(mirred_list);
+static DEFINE_SPINLOCK(mirred_list_lock);
static bool tcf_mirred_is_act_redirect(int action)
{
@@ -62,13 +63,23 @@ static bool tcf_mirred_can_reinsert(int action)
return false;
}
+static struct net_device *tcf_mirred_dev_dereference(struct tcf_mirred *m)
+{
+ return rcu_dereference_protected(m->tcfm_dev,
+ lockdep_is_held(&m->tcf_lock));
+}
+
static void tcf_mirred_release(struct tc_action *a)
{
struct tcf_mirred *m = to_mirred(a);
struct net_device *dev;
+ spin_lock(&mirred_list_lock);
list_del(&m->tcfm_list);
- dev = rtnl_dereference(m->tcfm_dev);
+ spin_unlock(&mirred_list_lock);
+
+ /* last reference to action, no need to lock */
+ dev = rcu_dereference_protected(m->tcfm_dev, 1);
if (dev)
dev_put(dev);
}
@@ -128,22 +139,9 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
NL_SET_ERR_MSG_MOD(extack, "Unknown mirred option");
return -EINVAL;
}
- if (parm->ifindex) {
- dev = __dev_get_by_index(net, parm->ifindex);
- if (dev == NULL) {
- if (exists)
- tcf_idr_release(*a, bind);
- else
- tcf_idr_cleanup(tn, parm->index);
- return -ENODEV;
- }
- mac_header_xmit = dev_is_mac_header_xmit(dev);
- } else {
- dev = NULL;
- }
if (!exists) {
- if (!dev) {
+ if (!parm->ifindex) {
tcf_idr_cleanup(tn, parm->index);
NL_SET_ERR_MSG_MOD(extack, "Specified device does not exist");
return -EINVAL;
@@ -161,19 +159,31 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
}
m = to_mirred(*a);
- ASSERT_RTNL();
+ spin_lock(&m->tcf_lock);
m->tcf_action = parm->action;
m->tcfm_eaction = parm->eaction;
- if (dev != NULL) {
- if (ret != ACT_P_CREATED)
- dev_put(rcu_dereference_protected(m->tcfm_dev, 1));
- dev_hold(dev);
- rcu_assign_pointer(m->tcfm_dev, dev);
+
+ if (parm->ifindex) {
+ dev = dev_get_by_index(net, parm->ifindex);
+ if (!dev) {
+ spin_unlock(&m->tcf_lock);
+ tcf_idr_release(*a, bind);
+ return -ENODEV;
+ }
+ mac_header_xmit = dev_is_mac_header_xmit(dev);
+ rcu_swap_protected(m->tcfm_dev, dev,
+ lockdep_is_held(&m->tcf_lock));
+ if (dev)
+ dev_put(dev);
m->tcfm_mac_header_xmit = mac_header_xmit;
}
+ spin_unlock(&m->tcf_lock);
if (ret == ACT_P_CREATED) {
+ spin_lock(&mirred_list_lock);
list_add(&m->tcfm_list, &mirred_list);
+ spin_unlock(&mirred_list_lock);
+
tcf_idr_insert(tn, *a);
}
@@ -287,26 +297,33 @@ static int tcf_mirred_dump(struct sk_buff *skb, struct tc_action *a, int bind,
{
unsigned char *b = skb_tail_pointer(skb);
struct tcf_mirred *m = to_mirred(a);
- struct net_device *dev = rtnl_dereference(m->tcfm_dev);
struct tc_mirred opt = {
.index = m->tcf_index,
- .action = m->tcf_action,
.refcnt = refcount_read(&m->tcf_refcnt) - ref,
.bindcnt = atomic_read(&m->tcf_bindcnt) - bind,
- .eaction = m->tcfm_eaction,
- .ifindex = dev ? dev->ifindex : 0,
};
+ struct net_device *dev;
struct tcf_t t;
+ spin_lock(&m->tcf_lock);
+ opt.action = m->tcf_action;
+ opt.eaction = m->tcfm_eaction;
+ dev = tcf_mirred_dev_dereference(m);
+ if (dev)
+ opt.ifindex = dev->ifindex;
+
if (nla_put(skb, TCA_MIRRED_PARMS, sizeof(opt), &opt))
goto nla_put_failure;
tcf_tm_dump(&t, &m->tcf_tm);
if (nla_put_64bit(skb, TCA_MIRRED_TM, sizeof(t), &t, TCA_MIRRED_PAD))
goto nla_put_failure;
+ spin_unlock(&m->tcf_lock);
+
return skb->len;
nla_put_failure:
+ spin_unlock(&m->tcf_lock);
nlmsg_trim(skb, b);
return -1;
}
@@ -337,15 +354,19 @@ static int mirred_device_event(struct notifier_block *unused,
ASSERT_RTNL();
if (event == NETDEV_UNREGISTER) {
+ spin_lock(&mirred_list_lock);
list_for_each_entry(m, &mirred_list, tcfm_list) {
- if (rcu_access_pointer(m->tcfm_dev) == dev) {
+ spin_lock(&m->tcf_lock);
+ if (tcf_mirred_dev_dereference(m) == dev) {
dev_put(dev);
/* Note : no rcu grace period necessary, as
* net_device are already rcu protected.
*/
RCU_INIT_POINTER(m->tcfm_dev, NULL);
}
+ spin_unlock(&m->tcf_lock);
}
+ spin_unlock(&mirred_list_lock);
}
return NOTIFY_DONE;
@@ -358,8 +379,15 @@ static struct notifier_block mirred_device_notifier = {
static struct net_device *tcf_mirred_get_dev(const struct tc_action *a)
{
struct tcf_mirred *m = to_mirred(a);
+ struct net_device *dev;
+
+ rcu_read_lock();
+ dev = rcu_dereference(m->tcfm_dev);
+ if (dev)
+ dev_hold(dev);
+ rcu_read_unlock();
- return rtnl_dereference(m->tcfm_dev);
+ return dev;
}
static int tcf_mirred_delete(struct net *net, u32 index)
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index e8b0bbd0883f..0cce0eadc28b 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -2167,6 +2167,7 @@ static int tc_exts_setup_cb_egdev_call(struct tcf_exts *exts,
if (!dev)
continue;
ret = tc_setup_cb_egdev_call(dev, type, type_data, err_stop);
+ dev_put(dev);
if (ret < 0)
return ret;
ok_count += ret;
--
2.7.5
^ permalink raw reply related
* [PATCH net-next 14/14] net: sched: act_police: remove dependency on rtnl lock
From: Vlad Buslov @ 2018-08-06 6:54 UTC (permalink / raw)
To: netdev
Cc: davem, jhs, xiyou.wangcong, jiri, pablo, kadlec, fw, ast, daniel,
edumazet, keescook, marcelo.leitner, Vlad Buslov
In-Reply-To: <1533538465-23199-1-git-send-email-vladbu@mellanox.com>
Use tcf spinlock to protect police action private data from concurrent
modification during dump. (init already uses tcf spinlock when changing
police action state)
Pass tcf spinlock as estimator lock argument to gen_replace_estimator()
during action init.
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
net/sched/act_police.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/net/sched/act_police.c b/net/sched/act_police.c
index 3eb5fe60c62c..4777da7caadf 100644
--- a/net/sched/act_police.c
+++ b/net/sched/act_police.c
@@ -138,7 +138,8 @@ static int tcf_act_police_init(struct net *net, struct nlattr *nla,
if (est) {
err = gen_replace_estimator(&police->tcf_bstats, NULL,
- &police->tcf_rate_est, NULL,
+ &police->tcf_rate_est,
+ &police->tcf_lock,
&police->tcf_lock,
NULL, est);
if (err)
@@ -274,14 +275,15 @@ static int tcf_act_police_dump(struct sk_buff *skb, struct tc_action *a,
struct tcf_police *police = to_police(a);
struct tc_police opt = {
.index = police->tcf_index,
- .action = police->tcf_action,
- .mtu = police->tcfp_mtu,
- .burst = PSCHED_NS2TICKS(police->tcfp_burst),
.refcnt = refcount_read(&police->tcf_refcnt) - ref,
.bindcnt = atomic_read(&police->tcf_bindcnt) - bind,
};
struct tcf_t t;
+ spin_lock_bh(&police->tcf_lock);
+ opt.action = police->tcf_action;
+ opt.mtu = police->tcfp_mtu;
+ opt.burst = PSCHED_NS2TICKS(police->tcfp_burst);
if (police->rate_present)
psched_ratecfg_getrate(&opt.rate, &police->rate);
if (police->peak_present)
@@ -301,10 +303,12 @@ static int tcf_act_police_dump(struct sk_buff *skb, struct tc_action *a,
t.expires = jiffies_to_clock_t(police->tcf_tm.expires);
if (nla_put_64bit(skb, TCA_POLICE_TM, sizeof(t), &t, TCA_POLICE_PAD))
goto nla_put_failure;
+ spin_unlock_bh(&police->tcf_lock);
return skb->len;
nla_put_failure:
+ spin_unlock_bh(&police->tcf_lock);
nlmsg_trim(skb, b);
return -1;
}
--
2.7.5
^ permalink raw reply related
* [PATCH net-next 06/14] net: sched: act_pedit: remove dependency on rtnl lock
From: Vlad Buslov @ 2018-08-06 6:54 UTC (permalink / raw)
To: netdev
Cc: davem, jhs, xiyou.wangcong, jiri, pablo, kadlec, fw, ast, daniel,
edumazet, keescook, marcelo.leitner, Vlad Buslov
In-Reply-To: <1533538465-23199-1-git-send-email-vladbu@mellanox.com>
Rearrange pedit init code to only access pedit action data while holding
tcf spinlock. Change keys allocation type to atomic to allow it to execute
while holding tcf spinlock. Take tcf spinlock in dump function when
accessing pedit action data.
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
net/sched/act_pedit.c | 40 ++++++++++++++++++++--------------------
1 file changed, 20 insertions(+), 20 deletions(-)
diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index 43ba999b2d23..3f62da72ab6a 100644
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -187,44 +187,38 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
tcf_idr_cleanup(tn, parm->index);
goto out_free;
}
- p = to_pedit(*a);
- keys = kmalloc(ksize, GFP_KERNEL);
- if (!keys) {
- tcf_idr_release(*a, bind);
- ret = -ENOMEM;
- goto out_free;
- }
ret = ACT_P_CREATED;
} else if (err > 0) {
if (bind)
goto out_free;
if (!ovr) {
- tcf_idr_release(*a, bind);
ret = -EEXIST;
- goto out_free;
- }
- p = to_pedit(*a);
- if (p->tcfp_nkeys && p->tcfp_nkeys != parm->nkeys) {
- keys = kmalloc(ksize, GFP_KERNEL);
- if (!keys) {
- ret = -ENOMEM;
- goto out_free;
- }
+ goto out_release;
}
} else {
return err;
}
+ p = to_pedit(*a);
spin_lock_bh(&p->tcf_lock);
- p->tcfp_flags = parm->flags;
- p->tcf_action = parm->action;
- if (keys) {
+
+ if (ret == ACT_P_CREATED ||
+ (p->tcfp_nkeys && p->tcfp_nkeys != parm->nkeys)) {
+ keys = kmalloc(ksize, GFP_ATOMIC);
+ if (!keys) {
+ spin_unlock_bh(&p->tcf_lock);
+ ret = -ENOMEM;
+ goto out_release;
+ }
kfree(p->tcfp_keys);
p->tcfp_keys = keys;
p->tcfp_nkeys = parm->nkeys;
}
memcpy(p->tcfp_keys, parm->keys, ksize);
+ p->tcfp_flags = parm->flags;
+ p->tcf_action = parm->action;
+
kfree(p->tcfp_keys_ex);
p->tcfp_keys_ex = keys_ex;
@@ -232,6 +226,9 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
if (ret == ACT_P_CREATED)
tcf_idr_insert(tn, *a);
return ret;
+
+out_release:
+ tcf_idr_release(*a, bind);
out_free:
kfree(keys_ex);
return ret;
@@ -410,6 +407,7 @@ static int tcf_pedit_dump(struct sk_buff *skb, struct tc_action *a,
if (unlikely(!opt))
return -ENOBUFS;
+ spin_lock_bh(&p->tcf_lock);
memcpy(opt->keys, p->tcfp_keys,
p->tcfp_nkeys * sizeof(struct tc_pedit_key));
opt->index = p->tcf_index;
@@ -432,11 +430,13 @@ static int tcf_pedit_dump(struct sk_buff *skb, struct tc_action *a,
tcf_tm_dump(&t, &p->tcf_tm);
if (nla_put_64bit(skb, TCA_PEDIT_TM, sizeof(t), &t, TCA_PEDIT_PAD))
goto nla_put_failure;
+ spin_unlock_bh(&p->tcf_lock);
kfree(opt);
return skb->len;
nla_put_failure:
+ spin_unlock_bh(&p->tcf_lock);
nlmsg_trim(skb, b);
kfree(opt);
return -1;
--
2.7.5
^ permalink raw reply related
* [PATCH net-next 05/14] net: sched: act_ipt: remove dependency on rtnl lock
From: Vlad Buslov @ 2018-08-06 6:54 UTC (permalink / raw)
To: netdev
Cc: davem, jhs, xiyou.wangcong, jiri, pablo, kadlec, fw, ast, daniel,
edumazet, keescook, marcelo.leitner, Vlad Buslov
In-Reply-To: <1533538465-23199-1-git-send-email-vladbu@mellanox.com>
Use tcf spinlock to protect ipt action private data from concurrent
modification during dump. Ipt init already takes tcf spinlock when
modifying ipt state.
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
net/sched/act_ipt.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/net/sched/act_ipt.c b/net/sched/act_ipt.c
index 0dc787a57798..e149f0e66cb6 100644
--- a/net/sched/act_ipt.c
+++ b/net/sched/act_ipt.c
@@ -288,6 +288,7 @@ static int tcf_ipt_dump(struct sk_buff *skb, struct tc_action *a, int bind,
* for foolproof you need to not assume this
*/
+ spin_lock_bh(&ipt->tcf_lock);
t = kmemdup(ipt->tcfi_t, ipt->tcfi_t->u.user.target_size, GFP_ATOMIC);
if (unlikely(!t))
goto nla_put_failure;
@@ -307,10 +308,12 @@ static int tcf_ipt_dump(struct sk_buff *skb, struct tc_action *a, int bind,
if (nla_put_64bit(skb, TCA_IPT_TM, sizeof(tm), &tm, TCA_IPT_PAD))
goto nla_put_failure;
+ spin_unlock_bh(&ipt->tcf_lock);
kfree(t);
return skb->len;
nla_put_failure:
+ spin_unlock_bh(&ipt->tcf_lock);
nlmsg_trim(skb, b);
kfree(t);
return -1;
--
2.7.5
^ permalink raw reply related
* [PATCH net-next 02/14] net: sched: act_csum: remove dependency on rtnl lock
From: Vlad Buslov @ 2018-08-06 6:54 UTC (permalink / raw)
To: netdev
Cc: davem, jhs, xiyou.wangcong, jiri, pablo, kadlec, fw, ast, daniel,
edumazet, keescook, marcelo.leitner, Vlad Buslov
In-Reply-To: <1533538465-23199-1-git-send-email-vladbu@mellanox.com>
Use tcf lock to protect csum action struct private data from concurrent
modification in init and dump. Use rcu swap operation to reassign params
pointer under protection of tcf lock. (old params value is not used by
init, so there is no need of standalone rcu dereference step)
Remove rtnl assertion that is no longer necessary.
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
net/sched/act_csum.c | 24 +++++++++++++++---------
1 file changed, 15 insertions(+), 9 deletions(-)
diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
index 648a3a35b720..f01c59ba6d12 100644
--- a/net/sched/act_csum.c
+++ b/net/sched/act_csum.c
@@ -50,7 +50,7 @@ static int tcf_csum_init(struct net *net, struct nlattr *nla,
struct netlink_ext_ack *extack)
{
struct tc_action_net *tn = net_generic(net, csum_net_id);
- struct tcf_csum_params *params_old, *params_new;
+ struct tcf_csum_params *params_new;
struct nlattr *tb[TCA_CSUM_MAX + 1];
struct tc_csum *parm;
struct tcf_csum *p;
@@ -88,20 +88,22 @@ static int tcf_csum_init(struct net *net, struct nlattr *nla,
}
p = to_tcf_csum(*a);
- ASSERT_RTNL();
params_new = kzalloc(sizeof(*params_new), GFP_KERNEL);
if (unlikely(!params_new)) {
tcf_idr_release(*a, bind);
return -ENOMEM;
}
- params_old = rtnl_dereference(p->params);
+ params_new->update_flags = parm->update_flags;
+ spin_lock(&p->tcf_lock);
p->tcf_action = parm->action;
- params_new->update_flags = parm->update_flags;
- rcu_assign_pointer(p->params, params_new);
- if (params_old)
- kfree_rcu(params_old, rcu);
+ rcu_swap_protected(p->params, params_new,
+ lockdep_is_held(&p->tcf_lock));
+ spin_unlock(&p->tcf_lock);
+
+ if (params_new)
+ kfree_rcu(params_new, rcu);
if (ret == ACT_P_CREATED)
tcf_idr_insert(tn, *a);
@@ -599,11 +601,13 @@ static int tcf_csum_dump(struct sk_buff *skb, struct tc_action *a, int bind,
.index = p->tcf_index,
.refcnt = refcount_read(&p->tcf_refcnt) - ref,
.bindcnt = atomic_read(&p->tcf_bindcnt) - bind,
- .action = p->tcf_action,
};
struct tcf_t t;
- params = rtnl_dereference(p->params);
+ spin_lock(&p->tcf_lock);
+ params = rcu_dereference_protected(p->params,
+ lockdep_is_held(&p->tcf_lock));
+ opt.action = p->tcf_action;
opt.update_flags = params->update_flags;
if (nla_put(skb, TCA_CSUM_PARMS, sizeof(opt), &opt))
@@ -612,10 +616,12 @@ static int tcf_csum_dump(struct sk_buff *skb, struct tc_action *a, int bind,
tcf_tm_dump(&t, &p->tcf_tm);
if (nla_put_64bit(skb, TCA_CSUM_TM, sizeof(t), &t, TCA_CSUM_PAD))
goto nla_put_failure;
+ spin_unlock(&p->tcf_lock);
return skb->len;
nla_put_failure:
+ spin_unlock(&p->tcf_lock);
nlmsg_trim(skb, b);
return -1;
}
--
2.7.5
^ permalink raw reply related
* [PATCH net-next 09/14] net: sched: act_skbmod: remove dependency on rtnl lock
From: Vlad Buslov @ 2018-08-06 6:54 UTC (permalink / raw)
To: netdev
Cc: davem, jhs, xiyou.wangcong, jiri, pablo, kadlec, fw, ast, daniel,
edumazet, keescook, marcelo.leitner, Vlad Buslov
In-Reply-To: <1533538465-23199-1-git-send-email-vladbu@mellanox.com>
Move read of skbmod_p rcu pointer to be protected by tcf spinlock. Use tcf
spinlock to protect private skbmod data from concurrent modification during
dump.
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
net/sched/act_skbmod.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/net/sched/act_skbmod.c b/net/sched/act_skbmod.c
index c437c6d51a71..e9c86ade3b40 100644
--- a/net/sched/act_skbmod.c
+++ b/net/sched/act_skbmod.c
@@ -156,7 +156,6 @@ static int tcf_skbmod_init(struct net *net, struct nlattr *nla,
d = to_skbmod(*a);
- ASSERT_RTNL();
p = kzalloc(sizeof(struct tcf_skbmod_params), GFP_KERNEL);
if (unlikely(!p)) {
tcf_idr_release(*a, bind);
@@ -166,10 +165,10 @@ static int tcf_skbmod_init(struct net *net, struct nlattr *nla,
p->flags = lflags;
d->tcf_action = parm->action;
- p_old = rtnl_dereference(d->skbmod_p);
-
if (ovr)
spin_lock_bh(&d->tcf_lock);
+ /* Protected by tcf_lock if overwriting existing action. */
+ p_old = rcu_dereference_protected(d->skbmod_p, 1);
if (lflags & SKBMOD_F_DMAC)
ether_addr_copy(p->eth_dst, daddr);
@@ -205,15 +204,18 @@ static int tcf_skbmod_dump(struct sk_buff *skb, struct tc_action *a,
{
struct tcf_skbmod *d = to_skbmod(a);
unsigned char *b = skb_tail_pointer(skb);
- struct tcf_skbmod_params *p = rtnl_dereference(d->skbmod_p);
+ struct tcf_skbmod_params *p;
struct tc_skbmod opt = {
.index = d->tcf_index,
.refcnt = refcount_read(&d->tcf_refcnt) - ref,
.bindcnt = atomic_read(&d->tcf_bindcnt) - bind,
- .action = d->tcf_action,
};
struct tcf_t t;
+ spin_lock_bh(&d->tcf_lock);
+ opt.action = d->tcf_action;
+ p = rcu_dereference_protected(d->skbmod_p,
+ lockdep_is_held(&d->tcf_lock));
opt.flags = p->flags;
if (nla_put(skb, TCA_SKBMOD_PARMS, sizeof(opt), &opt))
goto nla_put_failure;
@@ -231,8 +233,10 @@ static int tcf_skbmod_dump(struct sk_buff *skb, struct tc_action *a,
if (nla_put_64bit(skb, TCA_SKBMOD_TM, sizeof(t), &t, TCA_SKBMOD_PAD))
goto nla_put_failure;
+ spin_unlock_bh(&d->tcf_lock);
return skb->len;
nla_put_failure:
+ spin_unlock_bh(&d->tcf_lock);
nlmsg_trim(skb, b);
return -1;
}
--
2.7.5
^ permalink raw reply related
* [PATCH net-next 04/14] net: sched: act_ife: remove dependency on rtnl lock
From: Vlad Buslov @ 2018-08-06 6:54 UTC (permalink / raw)
To: netdev
Cc: davem, jhs, xiyou.wangcong, jiri, pablo, kadlec, fw, ast, daniel,
edumazet, keescook, marcelo.leitner, Vlad Buslov
In-Reply-To: <1533538465-23199-1-git-send-email-vladbu@mellanox.com>
Use tcf spinlock and rcu to protect params pointer from concurrent
modification during dump and init. Use rcu swap operation to reassign
params pointer under protection of tcf lock. (old params value is not used
by init, so there is no need of standalone rcu dereference step)
Ife action has meta-actions that are compiled as standalone modules. Rtnl
mutex must be released while loading a kernel module. In order to support
execution without rtnl mutex, propagate 'rtnl_held' argument to meta action
loading functions. When requesting meta action module, conditionally
release rtnl lock depending on 'rtnl_held' argument.
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
net/sched/act_ife.c | 40 +++++++++++++++++++++++++---------------
1 file changed, 25 insertions(+), 15 deletions(-)
diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c
index df4060e32d43..5d200495e467 100644
--- a/net/sched/act_ife.c
+++ b/net/sched/act_ife.c
@@ -268,7 +268,8 @@ static const char *ife_meta_id2name(u32 metaid)
* under ife->tcf_lock for existing action
*/
static int load_metaops_and_vet(struct tcf_ife_info *ife, u32 metaid,
- void *val, int len, bool exists)
+ void *val, int len, bool exists,
+ bool rtnl_held)
{
struct tcf_meta_ops *ops = find_ife_oplist(metaid);
int ret = 0;
@@ -278,9 +279,11 @@ static int load_metaops_and_vet(struct tcf_ife_info *ife, u32 metaid,
#ifdef CONFIG_MODULES
if (exists)
spin_unlock_bh(&ife->tcf_lock);
- rtnl_unlock();
+ if (rtnl_held)
+ rtnl_unlock();
request_module("ife-meta-%s", ife_meta_id2name(metaid));
- rtnl_lock();
+ if (rtnl_held)
+ rtnl_lock();
if (exists)
spin_lock_bh(&ife->tcf_lock);
ops = find_ife_oplist(metaid);
@@ -421,7 +424,7 @@ static void tcf_ife_cleanup(struct tc_action *a)
/* under ife->tcf_lock for existing action */
static int populate_metalist(struct tcf_ife_info *ife, struct nlattr **tb,
- bool exists)
+ bool exists, bool rtnl_held)
{
int len = 0;
int rc = 0;
@@ -433,7 +436,8 @@ static int populate_metalist(struct tcf_ife_info *ife, struct nlattr **tb,
val = nla_data(tb[i]);
len = nla_len(tb[i]);
- rc = load_metaops_and_vet(ife, i, val, len, exists);
+ rc = load_metaops_and_vet(ife, i, val, len, exists,
+ rtnl_held);
if (rc != 0)
return rc;
@@ -454,7 +458,7 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla,
struct tc_action_net *tn = net_generic(net, ife_net_id);
struct nlattr *tb[TCA_IFE_MAX + 1];
struct nlattr *tb2[IFE_META_MAX + 1];
- struct tcf_ife_params *p, *p_old;
+ struct tcf_ife_params *p;
struct tcf_ife_info *ife;
u16 ife_type = ETH_P_IFE;
struct tc_ife *parm;
@@ -558,7 +562,7 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla,
return err;
}
- err = populate_metalist(ife, tb2, exists);
+ err = populate_metalist(ife, tb2, exists, rtnl_held);
if (err)
goto metadata_parse_err;
@@ -581,13 +585,13 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla,
}
ife->tcf_action = parm->action;
+ /* protected by tcf_lock when modifying existing action */
+ rcu_swap_protected(ife->params, p, 1);
+
if (exists)
spin_unlock_bh(&ife->tcf_lock);
-
- p_old = rtnl_dereference(ife->params);
- rcu_assign_pointer(ife->params, p);
- if (p_old)
- kfree_rcu(p_old, rcu);
+ if (p)
+ kfree_rcu(p, rcu);
if (ret == ACT_P_CREATED)
tcf_idr_insert(tn, *a);
@@ -600,16 +604,20 @@ static int tcf_ife_dump(struct sk_buff *skb, struct tc_action *a, int bind,
{
unsigned char *b = skb_tail_pointer(skb);
struct tcf_ife_info *ife = to_ife(a);
- struct tcf_ife_params *p = rtnl_dereference(ife->params);
+ struct tcf_ife_params *p;
struct tc_ife opt = {
.index = ife->tcf_index,
.refcnt = refcount_read(&ife->tcf_refcnt) - ref,
.bindcnt = atomic_read(&ife->tcf_bindcnt) - bind,
- .action = ife->tcf_action,
- .flags = p->flags,
};
struct tcf_t t;
+ spin_lock_bh(&ife->tcf_lock);
+ opt.action = ife->tcf_action;
+ p = rcu_dereference_protected(ife->params,
+ lockdep_is_held(&ife->tcf_lock));
+ opt.flags = p->flags;
+
if (nla_put(skb, TCA_IFE_PARMS, sizeof(opt), &opt))
goto nla_put_failure;
@@ -635,9 +643,11 @@ static int tcf_ife_dump(struct sk_buff *skb, struct tc_action *a, int bind,
pr_info("Failed to dump metalist\n");
}
+ spin_unlock_bh(&ife->tcf_lock);
return skb->len;
nla_put_failure:
+ spin_unlock_bh(&ife->tcf_lock);
nlmsg_trim(skb, b);
return -1;
}
--
2.7.5
^ permalink raw reply related
* [PATCH net-next 00/14] Remove rtnl lock dependency from all action implementations
From: Vlad Buslov @ 2018-08-06 6:54 UTC (permalink / raw)
To: netdev
Cc: davem, jhs, xiyou.wangcong, jiri, pablo, kadlec, fw, ast, daniel,
edumazet, keescook, marcelo.leitner, Vlad Buslov
Currently, all netlink protocol handlers for updating rules, actions and
qdiscs are protected with single global rtnl lock which removes any
possibility for parallelism. This patch set is a second step to remove
rtnl lock dependency from TC rules update path.
Recently, new rtnl registration flag RTNL_FLAG_DOIT_UNLOCKED was added.
Handlers registered with this flag are called without RTNL taken. End
goal is to have rule update handlers(RTM_NEWTFILTER, RTM_DELTFILTER,
etc.) to be registered with UNLOCKED flag to allow parallel execution.
However, there is no intention to completely remove or split rtnl lock
itself. This patch set addresses specific problems in implementation of
tc actions that prevent their control path from being executed
concurrently. Additional changes are required to refactor classifiers
API and individual classifiers for parallel execution. This patch set
lays groundwork to eventually register rule update handlers as
rtnl-unlocked.
Action API is already prepared for parallel execution with previous
patch set, which means that action ops that use action API for their
implementation do not require additional modifications. (delete, search,
etc.) Action API implements concurrency-safe reference counting and
guarantees that cleanup/delete is called only once, after last reference
to action is released.
The goal of this change is to update specific actions APIs that access
action private state directly, in order to be independent from external
locking. General approach is to re-use existing tcf_lock spinlock (used
by some action implementation to synchronize control path with data
path) to protect action private state from concurrent modification. If
action has rcu-protected pointer, tcf spinlock is used to protect its
update code, instead of relying on rtnl lock.
Some actions need to determine rtnl mutex status in order to release it.
For example, ife action can load additional kernel modules(meta ops) and
must make sure that no locks are held during module load. In such cases
'rtnl_held' argument is used to conditionally release rtnl mutex.
Vlad Buslov (14):
net: sched: act_bpf: remove dependency on rtnl lock
net: sched: act_csum: remove dependency on rtnl lock
net: sched: act_gact: remove dependency on rtnl lock
net: sched: act_ife: remove dependency on rtnl lock
net: sched: act_ipt: remove dependency on rtnl lock
net: sched: act_pedit: remove dependency on rtnl lock
net: sched: act_sample: remove dependency on rtnl lock
net: sched: act_simple: remove dependency on rtnl lock
net: sched: act_skbmod: remove dependency on rtnl lock
net: sched: act_tunnel_key: remove dependency on rtnl lock
net: sched: act_vlan: remove dependency on rtnl lock
net: sched: act_mirred: remove dependency on rtnl lock
net: core: add new/replace rate estimator lock parameter
net: sched: act_police: remove dependency on rtnl lock
include/net/gen_stats.h | 2 +
include/net/tc_act/tc_mirred.h | 5 +++
include/net/tc_act/tc_tunnel_key.h | 33 ++++++++++++---
net/core/gen_estimator.c | 58 ++++++++++++++++++++-------
net/netfilter/xt_RATEEST.c | 2 +-
net/sched/act_api.c | 2 +-
net/sched/act_bpf.c | 10 +++--
net/sched/act_csum.c | 24 ++++++-----
net/sched/act_gact.c | 10 ++++-
net/sched/act_ife.c | 40 ++++++++++++-------
net/sched/act_ipt.c | 3 ++
net/sched/act_mirred.c | 82 +++++++++++++++++++++++++-------------
net/sched/act_pedit.c | 40 +++++++++----------
net/sched/act_police.c | 10 +++--
net/sched/act_sample.c | 12 +++++-
net/sched/act_simple.c | 6 ++-
net/sched/act_skbmod.c | 14 ++++---
net/sched/act_tunnel_key.c | 26 ++++++------
net/sched/act_vlan.c | 27 +++++++------
net/sched/cls_api.c | 1 +
net/sched/sch_api.c | 2 +
net/sched/sch_cbq.c | 4 +-
net/sched/sch_drr.c | 4 +-
net/sched/sch_hfsc.c | 4 +-
net/sched/sch_htb.c | 4 +-
net/sched/sch_qfq.c | 4 +-
26 files changed, 285 insertions(+), 144 deletions(-)
--
2.7.5
^ permalink raw reply
* [PATCH net-next 11/14] net: sched: act_vlan: remove dependency on rtnl lock
From: Vlad Buslov @ 2018-08-06 6:54 UTC (permalink / raw)
To: netdev
Cc: davem, jhs, xiyou.wangcong, jiri, pablo, kadlec, fw, ast, daniel,
edumazet, keescook, marcelo.leitner, Vlad Buslov
In-Reply-To: <1533538465-23199-1-git-send-email-vladbu@mellanox.com>
Use tcf spinlock to protect vlan action private data from concurrent
modification during dump and init. Use rcu swap operation to reassign
params pointer under protection of tcf lock. (old params value is not used
by init, so there is no need of standalone rcu dereference step)
Remove rtnl assertion that is no longer necessary.
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
net/sched/act_vlan.c | 27 +++++++++++++++------------
1 file changed, 15 insertions(+), 12 deletions(-)
diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c
index 15a0ee214c9c..5bde17fe3608 100644
--- a/net/sched/act_vlan.c
+++ b/net/sched/act_vlan.c
@@ -109,7 +109,7 @@ static int tcf_vlan_init(struct net *net, struct nlattr *nla,
{
struct tc_action_net *tn = net_generic(net, vlan_net_id);
struct nlattr *tb[TCA_VLAN_MAX + 1];
- struct tcf_vlan_params *p, *p_old;
+ struct tcf_vlan_params *p;
struct tc_vlan *parm;
struct tcf_vlan *v;
int action;
@@ -202,26 +202,24 @@ static int tcf_vlan_init(struct net *net, struct nlattr *nla,
v = to_vlan(*a);
- ASSERT_RTNL();
p = kzalloc(sizeof(*p), GFP_KERNEL);
if (!p) {
tcf_idr_release(*a, bind);
return -ENOMEM;
}
- v->tcf_action = parm->action;
-
- p_old = rtnl_dereference(v->vlan_p);
-
p->tcfv_action = action;
p->tcfv_push_vid = push_vid;
p->tcfv_push_prio = push_prio;
p->tcfv_push_proto = push_proto;
- rcu_assign_pointer(v->vlan_p, p);
+ spin_lock(&v->tcf_lock);
+ v->tcf_action = parm->action;
+ rcu_swap_protected(v->vlan_p, p, lockdep_is_held(&v->tcf_lock));
+ spin_unlock(&v->tcf_lock);
- if (p_old)
- kfree_rcu(p_old, rcu);
+ if (p)
+ kfree_rcu(p, rcu);
if (ret == ACT_P_CREATED)
tcf_idr_insert(tn, *a);
@@ -243,16 +241,18 @@ static int tcf_vlan_dump(struct sk_buff *skb, struct tc_action *a,
{
unsigned char *b = skb_tail_pointer(skb);
struct tcf_vlan *v = to_vlan(a);
- struct tcf_vlan_params *p = rtnl_dereference(v->vlan_p);
+ struct tcf_vlan_params *p;
struct tc_vlan opt = {
.index = v->tcf_index,
.refcnt = refcount_read(&v->tcf_refcnt) - ref,
.bindcnt = atomic_read(&v->tcf_bindcnt) - bind,
- .action = v->tcf_action,
- .v_action = p->tcfv_action,
};
struct tcf_t t;
+ spin_lock(&v->tcf_lock);
+ opt.action = v->tcf_action;
+ p = rcu_dereference_protected(v->vlan_p, lockdep_is_held(&v->tcf_lock));
+ opt.v_action = p->tcfv_action;
if (nla_put(skb, TCA_VLAN_PARMS, sizeof(opt), &opt))
goto nla_put_failure;
@@ -268,9 +268,12 @@ static int tcf_vlan_dump(struct sk_buff *skb, struct tc_action *a,
tcf_tm_dump(&t, &v->tcf_tm);
if (nla_put_64bit(skb, TCA_VLAN_TM, sizeof(t), &t, TCA_VLAN_PAD))
goto nla_put_failure;
+ spin_unlock(&v->tcf_lock);
+
return skb->len;
nla_put_failure:
+ spin_unlock(&v->tcf_lock);
nlmsg_trim(skb, b);
return -1;
}
--
2.7.5
^ permalink raw reply related
* [PATCH net-next 07/14] net: sched: act_sample: remove dependency on rtnl lock
From: Vlad Buslov @ 2018-08-06 6:54 UTC (permalink / raw)
To: netdev
Cc: davem, jhs, xiyou.wangcong, jiri, pablo, kadlec, fw, ast, daniel,
edumazet, keescook, marcelo.leitner, Vlad Buslov
In-Reply-To: <1533538465-23199-1-git-send-email-vladbu@mellanox.com>
Use tcf spinlock to protect private sample action data from concurrent
modification during dump and init.
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
net/sched/act_sample.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/net/sched/act_sample.c b/net/sched/act_sample.c
index 2608ccc83e5e..81071afe1b43 100644
--- a/net/sched/act_sample.c
+++ b/net/sched/act_sample.c
@@ -80,11 +80,13 @@ static int tcf_sample_init(struct net *net, struct nlattr *nla,
}
s = to_sample(*a);
+ spin_lock(&s->tcf_lock);
s->tcf_action = parm->action;
s->rate = nla_get_u32(tb[TCA_SAMPLE_RATE]);
s->psample_group_num = nla_get_u32(tb[TCA_SAMPLE_PSAMPLE_GROUP]);
psample_group = psample_group_get(net, s->psample_group_num);
if (!psample_group) {
+ spin_unlock(&s->tcf_lock);
tcf_idr_release(*a, bind);
return -ENOMEM;
}
@@ -94,6 +96,7 @@ static int tcf_sample_init(struct net *net, struct nlattr *nla,
s->truncate = true;
s->trunc_size = nla_get_u32(tb[TCA_SAMPLE_TRUNC_SIZE]);
}
+ spin_unlock(&s->tcf_lock);
if (ret == ACT_P_CREATED)
tcf_idr_insert(tn, *a);
@@ -105,7 +108,8 @@ static void tcf_sample_cleanup(struct tc_action *a)
struct tcf_sample *s = to_sample(a);
struct psample_group *psample_group;
- psample_group = rtnl_dereference(s->psample_group);
+ /* last reference to action, no need to lock */
+ psample_group = rcu_dereference_protected(s->psample_group, 1);
RCU_INIT_POINTER(s->psample_group, NULL);
if (psample_group)
psample_group_put(psample_group);
@@ -174,12 +178,13 @@ static int tcf_sample_dump(struct sk_buff *skb, struct tc_action *a,
struct tcf_sample *s = to_sample(a);
struct tc_sample opt = {
.index = s->tcf_index,
- .action = s->tcf_action,
.refcnt = refcount_read(&s->tcf_refcnt) - ref,
.bindcnt = atomic_read(&s->tcf_bindcnt) - bind,
};
struct tcf_t t;
+ spin_lock(&s->tcf_lock);
+ opt.action = s->tcf_action;
if (nla_put(skb, TCA_SAMPLE_PARMS, sizeof(opt), &opt))
goto nla_put_failure;
@@ -196,9 +201,12 @@ static int tcf_sample_dump(struct sk_buff *skb, struct tc_action *a,
if (nla_put_u32(skb, TCA_SAMPLE_PSAMPLE_GROUP, s->psample_group_num))
goto nla_put_failure;
+ spin_unlock(&s->tcf_lock);
+
return skb->len;
nla_put_failure:
+ spin_unlock(&s->tcf_lock);
nlmsg_trim(skb, b);
return -1;
}
--
2.7.5
^ permalink raw reply related
* [PATCH net-next 08/14] net: sched: act_simple: remove dependency on rtnl lock
From: Vlad Buslov @ 2018-08-06 6:54 UTC (permalink / raw)
To: netdev
Cc: davem, jhs, xiyou.wangcong, jiri, pablo, kadlec, fw, ast, daniel,
edumazet, keescook, marcelo.leitner, Vlad Buslov
In-Reply-To: <1533538465-23199-1-git-send-email-vladbu@mellanox.com>
Use tcf spinlock to protect private simple action data from concurrent
modification during dump. (simple init already uses tcf spinlock when
changing action state)
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
net/sched/act_simple.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/net/sched/act_simple.c b/net/sched/act_simple.c
index aa51152e0066..18e4452574cd 100644
--- a/net/sched/act_simple.c
+++ b/net/sched/act_simple.c
@@ -156,10 +156,11 @@ static int tcf_simp_dump(struct sk_buff *skb, struct tc_action *a,
.index = d->tcf_index,
.refcnt = refcount_read(&d->tcf_refcnt) - ref,
.bindcnt = atomic_read(&d->tcf_bindcnt) - bind,
- .action = d->tcf_action,
};
struct tcf_t t;
+ spin_lock_bh(&d->tcf_lock);
+ opt.action = d->tcf_action;
if (nla_put(skb, TCA_DEF_PARMS, sizeof(opt), &opt) ||
nla_put_string(skb, TCA_DEF_DATA, d->tcfd_defdata))
goto nla_put_failure;
@@ -167,9 +168,12 @@ static int tcf_simp_dump(struct sk_buff *skb, struct tc_action *a,
tcf_tm_dump(&t, &d->tcf_tm);
if (nla_put_64bit(skb, TCA_DEF_TM, sizeof(t), &t, TCA_DEF_PAD))
goto nla_put_failure;
+ spin_unlock_bh(&d->tcf_lock);
+
return skb->len;
nla_put_failure:
+ spin_unlock_bh(&d->tcf_lock);
nlmsg_trim(skb, b);
return -1;
}
--
2.7.5
^ permalink raw reply related
* [PATCH net-next 01/14] net: sched: act_bpf: remove dependency on rtnl lock
From: Vlad Buslov @ 2018-08-06 6:54 UTC (permalink / raw)
To: netdev
Cc: davem, jhs, xiyou.wangcong, jiri, pablo, kadlec, fw, ast, daniel,
edumazet, keescook, marcelo.leitner, Vlad Buslov
In-Reply-To: <1533538465-23199-1-git-send-email-vladbu@mellanox.com>
Use tcf spinlock to protect bpf action private data from concurrent
modification during dump and init. Remove rtnl lock assertion that is no
longer necessary.
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
net/sched/act_bpf.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/net/sched/act_bpf.c b/net/sched/act_bpf.c
index 6203eb075c9a..9e8a33f9fee3 100644
--- a/net/sched/act_bpf.c
+++ b/net/sched/act_bpf.c
@@ -143,11 +143,12 @@ static int tcf_bpf_dump(struct sk_buff *skb, struct tc_action *act,
.index = prog->tcf_index,
.refcnt = refcount_read(&prog->tcf_refcnt) - ref,
.bindcnt = atomic_read(&prog->tcf_bindcnt) - bind,
- .action = prog->tcf_action,
};
struct tcf_t tm;
int ret;
+ spin_lock(&prog->tcf_lock);
+ opt.action = prog->tcf_action;
if (nla_put(skb, TCA_ACT_BPF_PARMS, sizeof(opt), &opt))
goto nla_put_failure;
@@ -163,9 +164,11 @@ static int tcf_bpf_dump(struct sk_buff *skb, struct tc_action *act,
TCA_ACT_BPF_PAD))
goto nla_put_failure;
+ spin_unlock(&prog->tcf_lock);
return skb->len;
nla_put_failure:
+ spin_unlock(&prog->tcf_lock);
nlmsg_trim(skb, tp);
return -1;
}
@@ -264,7 +267,7 @@ static void tcf_bpf_prog_fill_cfg(const struct tcf_bpf *prog,
{
cfg->is_ebpf = tcf_bpf_is_ebpf(prog);
/* updates to prog->filter are prevented, since it's called either
- * with rtnl lock or during final cleanup in rcu callback
+ * with tcf lock or during final cleanup in rcu callback
*/
cfg->filter = rcu_dereference_protected(prog->filter, 1);
@@ -336,8 +339,8 @@ static int tcf_bpf_init(struct net *net, struct nlattr *nla,
goto out;
prog = to_bpf(*act);
- ASSERT_RTNL();
+ spin_lock(&prog->tcf_lock);
if (res != ACT_P_CREATED)
tcf_bpf_prog_fill_cfg(prog, &old);
@@ -349,6 +352,7 @@ static int tcf_bpf_init(struct net *net, struct nlattr *nla,
prog->tcf_action = parm->action;
rcu_assign_pointer(prog->filter, cfg.filter);
+ spin_unlock(&prog->tcf_lock);
if (res == ACT_P_CREATED) {
tcf_idr_insert(tn, *act);
--
2.7.5
^ permalink raw reply related
* [PATCH net-next 03/14] net: sched: act_gact: remove dependency on rtnl lock
From: Vlad Buslov @ 2018-08-06 6:54 UTC (permalink / raw)
To: netdev
Cc: davem, jhs, xiyou.wangcong, jiri, pablo, kadlec, fw, ast, daniel,
edumazet, keescook, marcelo.leitner, Vlad Buslov
In-Reply-To: <1533538465-23199-1-git-send-email-vladbu@mellanox.com>
Use tcf spinlock to protect gact action private state from concurrent
modification during dump and init. Remove rtnl assertion that is no longer
necessary.
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
net/sched/act_gact.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/net/sched/act_gact.c b/net/sched/act_gact.c
index 661b72b9147d..bfccd34a3968 100644
--- a/net/sched/act_gact.c
+++ b/net/sched/act_gact.c
@@ -113,7 +113,7 @@ static int tcf_gact_init(struct net *net, struct nlattr *nla,
gact = to_gact(*a);
- ASSERT_RTNL();
+ spin_lock(&gact->tcf_lock);
gact->tcf_action = parm->action;
#ifdef CONFIG_GACT_PROB
if (p_parm) {
@@ -126,6 +126,8 @@ static int tcf_gact_init(struct net *net, struct nlattr *nla,
gact->tcfg_ptype = p_parm->ptype;
}
#endif
+ spin_unlock(&gact->tcf_lock);
+
if (ret == ACT_P_CREATED)
tcf_idr_insert(tn, *a);
return ret;
@@ -178,10 +180,11 @@ static int tcf_gact_dump(struct sk_buff *skb, struct tc_action *a,
.index = gact->tcf_index,
.refcnt = refcount_read(&gact->tcf_refcnt) - ref,
.bindcnt = atomic_read(&gact->tcf_bindcnt) - bind,
- .action = gact->tcf_action,
};
struct tcf_t t;
+ spin_lock(&gact->tcf_lock);
+ opt.action = gact->tcf_action;
if (nla_put(skb, TCA_GACT_PARMS, sizeof(opt), &opt))
goto nla_put_failure;
#ifdef CONFIG_GACT_PROB
@@ -199,9 +202,12 @@ static int tcf_gact_dump(struct sk_buff *skb, struct tc_action *a,
tcf_tm_dump(&t, &gact->tcf_tm);
if (nla_put_64bit(skb, TCA_GACT_TM, sizeof(t), &t, TCA_GACT_PAD))
goto nla_put_failure;
+ spin_unlock(&gact->tcf_lock);
+
return skb->len;
nla_put_failure:
+ spin_unlock(&gact->tcf_lock);
nlmsg_trim(skb, b);
return -1;
}
--
2.7.5
^ permalink raw reply related
* [PATCH net-next 10/14] net: sched: act_tunnel_key: remove dependency on rtnl lock
From: Vlad Buslov @ 2018-08-06 6:54 UTC (permalink / raw)
To: netdev
Cc: davem, jhs, xiyou.wangcong, jiri, pablo, kadlec, fw, ast, daniel,
edumazet, keescook, marcelo.leitner, Vlad Buslov
In-Reply-To: <1533538465-23199-1-git-send-email-vladbu@mellanox.com>
Use tcf lock to protect tunnel key action struct private data from
concurrent modification in init and dump. Use rcu swap operation to
reassign params pointer under protection of tcf lock. (old params value is
not used by init, so there is no need of standalone rcu dereference step)
Remove rtnl lock assertion that is no longer required.
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
net/sched/act_tunnel_key.c | 26 +++++++++++++-------------
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/net/sched/act_tunnel_key.c b/net/sched/act_tunnel_key.c
index d42d9e112789..ba2ae9f75ef5 100644
--- a/net/sched/act_tunnel_key.c
+++ b/net/sched/act_tunnel_key.c
@@ -204,7 +204,6 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
{
struct tc_action_net *tn = net_generic(net, tunnel_key_net_id);
struct nlattr *tb[TCA_TUNNEL_KEY_MAX + 1];
- struct tcf_tunnel_key_params *params_old;
struct tcf_tunnel_key_params *params_new;
struct metadata_dst *metadata = NULL;
struct tc_tunnel_key *parm;
@@ -346,24 +345,22 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
t = to_tunnel_key(*a);
- ASSERT_RTNL();
params_new = kzalloc(sizeof(*params_new), GFP_KERNEL);
if (unlikely(!params_new)) {
tcf_idr_release(*a, bind);
NL_SET_ERR_MSG(extack, "Cannot allocate tunnel key parameters");
return -ENOMEM;
}
-
- params_old = rtnl_dereference(t->params);
-
- t->tcf_action = parm->action;
params_new->tcft_action = parm->t_action;
params_new->tcft_enc_metadata = metadata;
- rcu_assign_pointer(t->params, params_new);
-
- if (params_old)
- kfree_rcu(params_old, rcu);
+ spin_lock(&t->tcf_lock);
+ t->tcf_action = parm->action;
+ rcu_swap_protected(t->params, params_new,
+ lockdep_is_held(&t->tcf_lock));
+ spin_unlock(&t->tcf_lock);
+ if (params_new)
+ kfree_rcu(params_new, rcu);
if (ret == ACT_P_CREATED)
tcf_idr_insert(tn, *a);
@@ -485,12 +482,13 @@ static int tunnel_key_dump(struct sk_buff *skb, struct tc_action *a,
.index = t->tcf_index,
.refcnt = refcount_read(&t->tcf_refcnt) - ref,
.bindcnt = atomic_read(&t->tcf_bindcnt) - bind,
- .action = t->tcf_action,
};
struct tcf_t tm;
- params = rtnl_dereference(t->params);
-
+ spin_lock(&t->tcf_lock);
+ params = rcu_dereference_protected(t->params,
+ lockdep_is_held(&t->tcf_lock));
+ opt.action = t->tcf_action;
opt.t_action = params->tcft_action;
if (nla_put(skb, TCA_TUNNEL_KEY_PARMS, sizeof(opt), &opt))
@@ -522,10 +520,12 @@ static int tunnel_key_dump(struct sk_buff *skb, struct tc_action *a,
if (nla_put_64bit(skb, TCA_TUNNEL_KEY_TM, sizeof(tm),
&tm, TCA_TUNNEL_KEY_PAD))
goto nla_put_failure;
+ spin_unlock(&t->tcf_lock);
return skb->len;
nla_put_failure:
+ spin_unlock(&t->tcf_lock);
nlmsg_trim(skb, b);
return -1;
}
--
2.7.5
^ permalink raw reply related
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