Netdev List
 help / color / mirror / Atom feed
* [PATCH net 2/2] ibmvnic: Update driver queues after change in ring size support
From: Thomas Falcon @ 2018-11-21 17:17 UTC (permalink / raw)
  To: netdev, linuxppc-dev; +Cc: mwb, julietk, tyreld, Thomas Falcon
In-Reply-To: <1542820679-30184-1-git-send-email-tlfalcon@linux.ibm.com>

During device reset, queue memory is not being updated to accommodate
changes in ring buffer sizes supported by backing hardware. Track
any differences in ring buffer sizes following the reset and update
queue memory when possible.

Signed-off-by: Thomas Falcon <tlfalcon@linux.ibm.com>
---
 drivers/net/ethernet/ibm/ibmvnic.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index 066897a..c0203a0 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -1737,6 +1737,7 @@ static int do_reset(struct ibmvnic_adapter *adapter,
 		    struct ibmvnic_rwi *rwi, u32 reset_state)
 {
 	u64 old_num_rx_queues, old_num_tx_queues;
+	u64 old_num_rx_slots, old_num_tx_slots;
 	struct net_device *netdev = adapter->netdev;
 	int i, rc;
 
@@ -1748,6 +1749,8 @@ static int do_reset(struct ibmvnic_adapter *adapter,
 
 	old_num_rx_queues = adapter->req_rx_queues;
 	old_num_tx_queues = adapter->req_tx_queues;
+	old_num_rx_slots = adapter->req_rx_add_entries_per_subcrq;
+	old_num_tx_slots = adapter->req_tx_entries_per_subcrq;
 
 	ibmvnic_cleanup(netdev);
 
@@ -1810,7 +1813,11 @@ static int do_reset(struct ibmvnic_adapter *adapter,
 			if (rc)
 				return rc;
 		} else if (adapter->req_rx_queues != old_num_rx_queues ||
-			   adapter->req_tx_queues != old_num_tx_queues) {
+			   adapter->req_tx_queues != old_num_tx_queues ||
+			   adapter->req_rx_add_entries_per_subcrq !=
+							old_num_rx_slots ||
+			   adapter->req_tx_entries_per_subcrq !=
+							old_num_tx_slots) {
 			release_rx_pools(adapter);
 			release_tx_pools(adapter);
 			release_napi(adapter);
-- 
1.8.3.1

^ permalink raw reply related

* [PATCH net 1/2] ibmvnic: Fix RX queue buffer cleanup
From: Thomas Falcon @ 2018-11-21 17:17 UTC (permalink / raw)
  To: netdev, linuxppc-dev; +Cc: mwb, julietk, tyreld, Thomas Falcon
In-Reply-To: <1542820679-30184-1-git-send-email-tlfalcon@linux.ibm.com>

The wrong index is used when cleaning up RX buffer objects during release
of RX queues. Update to use the correct index counter.

Signed-off-by: Thomas Falcon <tlfalcon@linux.ibm.com>
---
 drivers/net/ethernet/ibm/ibmvnic.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index 27a6df3..066897a 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -485,8 +485,8 @@ static void release_rx_pools(struct ibmvnic_adapter *adapter)
 
 		for (j = 0; j < rx_pool->size; j++) {
 			if (rx_pool->rx_buff[j].skb) {
-				dev_kfree_skb_any(rx_pool->rx_buff[i].skb);
-				rx_pool->rx_buff[i].skb = NULL;
+				dev_kfree_skb_any(rx_pool->rx_buff[j].skb);
+				rx_pool->rx_buff[j].skb = NULL;
 			}
 		}
 
-- 
1.8.3.1

^ permalink raw reply related

* [PATCH net 0/2] ibmvnic: Fix queue and buffer accounting errors
From: Thomas Falcon @ 2018-11-21 17:17 UTC (permalink / raw)
  To: netdev, linuxppc-dev; +Cc: mwb, julietk, tyreld, Thomas Falcon

This series includes two small fixes. The first resolves a typo bug
in the code to clean up unused RX buffers during device queue removal.
The second ensures that device queue memory is updated to reflect new
supported queue ring sizes after migration to other backing hardware.


Thomas Falcon (2):
  ibmvnic: Fix RX queue buffer cleanup
  ibmvnic: Update driver queues after change in ring size support

 drivers/net/ethernet/ibm/ibmvnic.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

-- 
1.8.3.1

^ permalink raw reply

* RE: [PATCH v1 net] lan743x: fix return value for lan743x_tx_napi_poll
From: Bryan.Whitehead @ 2018-11-21 16:47 UTC (permalink / raw)
  To: Tristram.Ha; +Cc: davem, netdev, UNGLinuxDriver, andrew, f.fainelli
In-Reply-To: <93AF473E2DA327428DE3D46B72B1E9FD411F1362@CHN-SV-EXMX02.mchp-main.com>

> I notice 2 problems in the driver:
> 
> 1. netif_napi_add is used instead of netif_tx_napi_add.
> 2. In all other drivers that use netif_tx_napi_add most do not call
> napi_complete_done.
> They all call napi_complete directly and return 0.
> 
> freescale/gianfar.c
> rocker/rocker_main.c
> ti/cpsw.c
> 
> virtio_net.c does use napi_complete_done but it also passes 0 as a
> parameter.
> 
Tristram,

Thanks for the tips. I will make a new version which uses
  netif_tx_napi_add, and napi_complete

Regards,
Bryan

^ permalink raw reply

* RE: [PATCH v1 net] lan743x: fix return value for lan743x_tx_napi_poll
From: Bryan.Whitehead @ 2018-11-21 16:44 UTC (permalink / raw)
  To: f.fainelli, andrew; +Cc: davem, netdev, UNGLinuxDriver
In-Reply-To: <27c637a7-a988-1acb-b967-70f5719306a1@gmail.com>

> Did you look at the output of "perf top" or something along those lines to
> figure out if your lan743x driver is indeed responsible for that by not being
> scheduler friendly? What is likely happening is that you do not reclaim
> "weight" packets and instead keep looping into NAPI context, which
> prevents the system from making further progress.
> 
I'm having trouble installing "perf" for my kernel. But I have used GPIO's and a scope
To make sure my poll routine is called and returns in a timely manner. I've never seen
A problem with it "not being scheduler friendly"

> Calling napi_complete_done() for the TX path is not necessary AFAICT, what
> you really want to do is call napi_complete() and make sure you:
> 
> - reclaim/free as many TX buffers as possible, without looking at the NAPI
> weight which becomes irrelevant
> - if you have been able to reclaim enough descriptors, wake-up the transmit
> queue
> 
> So ignoring the NAPI weight like you do is correct, but calling
> napi_complete_done() with a 0 argument does not sound correct to me.
> --
> Florian

I see that napi_complete just maps to napi_complete_done with a 0 argument anyway.
So they are currently functionally identical, but I will make a new version that uses
napi_complete as you suggested.

But it seems you would definitely agree that the previous version of lan743x driver
Which called napi_complete_done with 'weight' as the argument is wrong,
And therefor this fix is correct.

^ permalink raw reply

* Re: [PATCH v2 RESEND update 1/2] mm/page_alloc: free order-0 pages through PCP in page_frag_free()
From: Andrew Morton @ 2018-11-22  3:15 UTC (permalink / raw)
  To: Aaron Lu
  Cc: linux-mm, linux-kernel, netdev, Paweł Staszewski,
	Jesper Dangaard Brouer, Eric Dumazet, Tariq Toukan,
	Ilias Apalodimas, Yoel Caspersen, Mel Gorman, Saeed Mahameed,
	Michal Hocko, Vlastimil Babka, Dave Hansen, Alexander Duyck,
	Ian Kumlien
In-Reply-To: <20181120014544.GB10657@intel.com>

On Tue, 20 Nov 2018 09:45:44 +0800 Aaron Lu <aaron.lu@intel.com> wrote:

> page_frag_free() calls __free_pages_ok() to free the page back to
> Buddy. This is OK for high order page, but for order-0 pages, it
> misses the optimization opportunity of using Per-Cpu-Pages and can
> cause zone lock contention when called frequently.
> 

Looks nice to me.  Let's tell our readers why we're doing this.

--- a/mm/page_alloc.c~mm-page_alloc-free-order-0-pages-through-pcp-in-page_frag_free-fix
+++ a/mm/page_alloc.c
@@ -4684,7 +4684,7 @@ void page_frag_free(void *addr)
 	if (unlikely(put_page_testzero(page))) {
 		unsigned int order = compound_order(page);
 
-		if (order == 0)
+		if (order == 0)		/* Via pcp? */
 			free_unref_page(page);
 		else
 			__free_pages_ok(page, order);
_

^ permalink raw reply

* Re: [RFCv3 PATCH 1/6] uacce: Add documents for WarpDrive/uacce
From: Jason Gunthorpe @ 2018-11-22  2:58 UTC (permalink / raw)
  To: Kenneth Lee
  Cc: Leon Romanovsky, Kenneth Lee, Tim Sell, linux-doc,
	Alexander Shishkin, Zaibo Xu, zhangfei.gao, linuxarm,
	haojian.zhuang, Christoph Lameter, Hao Fang, Gavin Schenk,
	RDMA mailing list, Zhou Wang, Doug Ledford, Uwe Kleine-König,
	David Kershner, Johan Hovold, Cyrille Pitchen, Sagar Dharia
In-Reply-To: <20181121060805.GJ157308@Turing-Arch-b>

On Wed, Nov 21, 2018 at 02:08:05PM +0800, Kenneth Lee wrote:

> > But considering Jean's SVA stuff seems based on mmu notifiers, I have
> > a hard time believing that it has any different behavior from RDMA's
> > ODP, and if it does have different behavior, then it is probably just
> > a bug in the ODP implementation.
> 
> As Jean has explained, his solution is based on page table sharing. I think ODP
> should also consider this new feature.

Shared page tables would require the HW to walk the page table format
of the CPU directly, not sure how that would be possible for ODP?

Presumably the implementation for ARM relies on the IOMMU hardware
doing this?

> > > > If all your driver needs is to mmap some PCI bar space, route
> > > > interrupts and do DMA mapping then mediated VFIO is probably a good
> > > > choice. 
> > > 
> > > Yes. That is what is done in our RFCv1/v2. But we accepted Jerome's opinion and
> > > try not to add complexity to the mm subsystem.
> > 
> > Why would a mediated VFIO driver touch the mm subsystem? Sounds like
> > you don't have a VFIO driver if it needs to do stuff like that...
> 
> VFIO has no ODP-like solution, and if we want to solve the fork problem, we have
> to make some change to iommu and the fork procedure. Further, VFIO takes every
> queue as a independent device. This create a lot of trouble on resource
> management. For example, you will need a manager process to withdraw the unused
> device and you need to let the user process know about PASID of the queue, and
> so on.

Well, I would think you'd add SVA support to the VFIO driver as a
generic capability - it seems pretty useful for any VFIO user as it
avoids all the kernel upcalls to do memory pinning and DMA address
translation.

Once the VFIO driver knows about this as a generic capability then the
device it exposes to userspace would use CPU addresses instead of DMA
addresses.

The question is if your driver needs much more than the device
agnostic generic services VFIO provides.

I'm not sure what you have in mind with resource management.. It is
hard to revoke resources from userspace, unless you are doing
kernel syscalls, but then why do all this?

Jason

^ permalink raw reply

* [PATCH] samples: bpf: fix: error handling regarding kprobe_events
From: Daniel T. Lee @ 2018-11-21 16:07 UTC (permalink / raw)
  To: Daniel Borkmann, Alexei Starovoitov; +Cc: netdev

Currently, kprobe_events failure won't be handled properly.
Due to calling system() indirectly to write to kprobe_events,
it can't be identified whether an error is derived from kprobe or system.

    // buf = "echo '%c:%s %s' >> /s/k/d/t/kprobe_events"
    err = system(buf);
    if (err < 0) {
        printf("failed to create kprobe ..");
        return -1;
    }

For example, running ./tracex7 sample in ext4 partition,
"echo p:open_ctree open_ctree >> /s/k/d/t/kprobe_events"
gets 256 error code system() failure.
=> The error comes from kprobe, but it's not handled correctly.

According to man of system(3), it's return value
just passes the termination status of the child shell
rather than treating the error as -1. (don't care success)

Which means, currently it's not working as desired.
(According to the upper code snippet)

    ex) running ./tracex7 with ext4 env.
    # Current Output
    sh: echo: I/O error
    failed to open event open_ctree

    # Desired Output
    failed to create kprobe 'open_ctree' error 'No such file or directory'

The problem is, error can't be verified whether from child ps or system.

But using write() directly can verify the command failure,
and it will treat all error as -1.

So I suggest using write() directly to 'kprobe_events'
rather than calling system().

Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
---
 samples/bpf/bpf_load.c | 33 ++++++++++++++++++++++++---------
 1 file changed, 24 insertions(+), 9 deletions(-)

diff --git a/samples/bpf/bpf_load.c b/samples/bpf/bpf_load.c
index e6d7e0fe155b..3e34d733f5f8 100644
--- a/samples/bpf/bpf_load.c
+++ b/samples/bpf/bpf_load.c
@@ -54,6 +54,23 @@ static int populate_prog_array(const char *event, int prog_fd)
 	return 0;
 }
 
+static int write_kprobe_events(const char *val)
+{
+	int ret, flags;
+
+	if ((val != NULL) && (val[0] == '\0'))
+		flags = O_WRONLY | O_TRUNC;
+	else
+		flags = O_WRONLY | O_APPEND;
+
+	int fd = open("/sys/kernel/debug/tracing/kprobe_events", flags);
+
+	ret = write(fd, val, strlen(val));
+	close(fd);
+
+	return ret;
+}
+
 static int load_and_attach(const char *event, struct bpf_insn *prog, int size)
 {
 	bool is_socket = strncmp(event, "socket", 6) == 0;
@@ -165,10 +182,9 @@ static int load_and_attach(const char *event, struct bpf_insn *prog, int size)
 
 #ifdef __x86_64__
 		if (strncmp(event, "sys_", 4) == 0) {
-			snprintf(buf, sizeof(buf),
-				 "echo '%c:__x64_%s __x64_%s' >> /sys/kernel/debug/tracing/kprobe_events",
-				 is_kprobe ? 'p' : 'r', event, event);
-			err = system(buf);
+			snprintf(buf, sizeof(buf), "%c:__x64_%s __x64_%s",
+				is_kprobe ? 'p' : 'r', event, event);
+			err = write_kprobe_events(buf);
 			if (err >= 0) {
 				need_normal_check = false;
 				event_prefix = "__x64_";
@@ -176,10 +192,9 @@ static int load_and_attach(const char *event, struct bpf_insn *prog, int size)
 		}
 #endif
 		if (need_normal_check) {
-			snprintf(buf, sizeof(buf),
-				 "echo '%c:%s %s' >> /sys/kernel/debug/tracing/kprobe_events",
-				 is_kprobe ? 'p' : 'r', event, event);
-			err = system(buf);
+			snprintf(buf, sizeof(buf), "%c:%s %s",
+				is_kprobe ? 'p' : 'r', event, event);
+			err = write_kprobe_events(buf);
 			if (err < 0) {
 				printf("failed to create kprobe '%s' error '%s'\n",
 				       event, strerror(errno));
@@ -519,7 +534,7 @@ static int do_load_bpf_file(const char *path, fixup_map_cb fixup_map)
 		return 1;
 
 	/* clear all kprobes */
-	i = system("echo \"\" > /sys/kernel/debug/tracing/kprobe_events");
+	i = write_kprobe_events("");
 
 	/* scan over all elf sections to get license and map info */
 	for (i = 1; i < ehdr.e_shnum; i++) {
-- 
2.17.1

^ permalink raw reply related

* [PATCH net] net: thunderx: set xdp_prog to NULL if bpf_prog_add fails
From: Lorenzo Bianconi @ 2018-11-21 15:32 UTC (permalink / raw)
  To: netdev; +Cc: Sunil.Goutham
In-Reply-To: <cover.1542809524.git.lorenzo.bianconi@redhat.com>

Set xdp_prog pointer to NULL if bpf_prog_add fails since that routine
reports the error code instead of NULL in case of failure and xdp_prog
pointer value is used in the driver to verify if XDP is currently
enabled.
Moreover report the error code to userspace if nicvf_xdp_setup fails

Fixes: 05c773f52b96 ("net: thunderx: Add basic XDP support")
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
 drivers/net/ethernet/cavium/thunder/nicvf_main.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
index 768f584f8392..88f8a8fa93cd 100644
--- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
+++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
@@ -1784,6 +1784,7 @@ static int nicvf_xdp_setup(struct nicvf *nic, struct bpf_prog *prog)
 	bool if_up = netif_running(nic->netdev);
 	struct bpf_prog *old_prog;
 	bool bpf_attached = false;
+	int ret = 0;
 
 	/* For now just support only the usual MTU sized frames */
 	if (prog && (dev->mtu > 1500)) {
@@ -1817,8 +1818,12 @@ static int nicvf_xdp_setup(struct nicvf *nic, struct bpf_prog *prog)
 	if (nic->xdp_prog) {
 		/* Attach BPF program */
 		nic->xdp_prog = bpf_prog_add(nic->xdp_prog, nic->rx_queues - 1);
-		if (!IS_ERR(nic->xdp_prog))
+		if (!IS_ERR(nic->xdp_prog)) {
 			bpf_attached = true;
+		} else {
+			ret = PTR_ERR(nic->xdp_prog);
+			nic->xdp_prog = NULL;
+		}
 	}
 
 	/* Calculate Tx queues needed for XDP and network stack */
@@ -1830,7 +1835,7 @@ static int nicvf_xdp_setup(struct nicvf *nic, struct bpf_prog *prog)
 		netif_trans_update(nic->netdev);
 	}
 
-	return 0;
+	return ret;
 }
 
 static int nicvf_xdp(struct net_device *netdev, struct netdev_bpf *xdp)
-- 
2.19.1

^ permalink raw reply related

* [PATCH net-next] {net,IB}/mlx4: Initialize CQ buffers in the driver when possible
From: Tariq Toukan @ 2018-11-21 15:12 UTC (permalink / raw)
  To: David S. Miller
  Cc: netdev, Eran Ben Elisha, Daniel Jurgens, Jack Morgenstein,
	Tariq Toukan

From: Daniel Jurgens <danielj@mellanox.com>

Perform CQ initialization in the driver when the capability is supported
by the FW.  When passing the CQ to HW indicate that the CQ buffer has
been pre-initialized.

Doing so decreases CQ creation time.  Testing on P8 showed a single 2048
entry CQ creation time was reduced from ~395us to ~170us, which is
2.3x faster.

Signed-off-by: Daniel Jurgens <danielj@mellanox.com>
Signed-off-by: Jack Morgenstein <jackm@dev.mellanox.co.il>
Signed-off-by: Tariq Toukan <tariqt@mellanox.com>
---
 drivers/infiniband/hw/mlx4/cq.c            |  9 +++-
 drivers/net/ethernet/mellanox/mlx4/cq.c    | 71 ++++++++++++++++++++++++++++--
 drivers/net/ethernet/mellanox/mlx4/en_cq.c |  2 +-
 drivers/net/ethernet/mellanox/mlx4/fw.c    |  3 ++
 include/linux/mlx4/device.h                |  4 +-
 5 files changed, 82 insertions(+), 7 deletions(-)

diff --git a/drivers/infiniband/hw/mlx4/cq.c b/drivers/infiniband/hw/mlx4/cq.c
index 82adc0d1d30e..43512347b4f0 100644
--- a/drivers/infiniband/hw/mlx4/cq.c
+++ b/drivers/infiniband/hw/mlx4/cq.c
@@ -181,6 +181,7 @@ struct ib_cq *mlx4_ib_create_cq(struct ib_device *ibdev,
 	struct mlx4_ib_dev *dev = to_mdev(ibdev);
 	struct mlx4_ib_cq *cq;
 	struct mlx4_uar *uar;
+	void *buf_addr;
 	int err;
 
 	if (entries < 1 || entries > dev->dev->caps.max_cqes)
@@ -211,6 +212,8 @@ struct ib_cq *mlx4_ib_create_cq(struct ib_device *ibdev,
 			goto err_cq;
 		}
 
+		buf_addr = (void *)(unsigned long)ucmd.buf_addr;
+
 		err = mlx4_ib_get_cq_umem(dev, context, &cq->buf, &cq->umem,
 					  ucmd.buf_addr, entries);
 		if (err)
@@ -237,6 +240,8 @@ struct ib_cq *mlx4_ib_create_cq(struct ib_device *ibdev,
 		if (err)
 			goto err_db;
 
+		buf_addr = &cq->buf.buf;
+
 		uar = &dev->priv_uar;
 		cq->mcq.usage = MLX4_RES_USAGE_DRIVER;
 	}
@@ -246,7 +251,9 @@ struct ib_cq *mlx4_ib_create_cq(struct ib_device *ibdev,
 
 	err = mlx4_cq_alloc(dev->dev, entries, &cq->buf.mtt, uar,
 			    cq->db.dma, &cq->mcq, vector, 0,
-			    !!(cq->create_flags & IB_UVERBS_CQ_FLAGS_TIMESTAMP_COMPLETION));
+			    !!(cq->create_flags &
+			       IB_UVERBS_CQ_FLAGS_TIMESTAMP_COMPLETION),
+			    buf_addr, !!context);
 	if (err)
 		goto err_dbmap;
 
diff --git a/drivers/net/ethernet/mellanox/mlx4/cq.c b/drivers/net/ethernet/mellanox/mlx4/cq.c
index d8e9a323122e..db909b6069b5 100644
--- a/drivers/net/ethernet/mellanox/mlx4/cq.c
+++ b/drivers/net/ethernet/mellanox/mlx4/cq.c
@@ -144,9 +144,9 @@ void mlx4_cq_event(struct mlx4_dev *dev, u32 cqn, int event_type)
 }
 
 static int mlx4_SW2HW_CQ(struct mlx4_dev *dev, struct mlx4_cmd_mailbox *mailbox,
-			 int cq_num)
+			 int cq_num, u8 opmod)
 {
-	return mlx4_cmd(dev, mailbox->dma, cq_num, 0,
+	return mlx4_cmd(dev, mailbox->dma, cq_num, opmod,
 			MLX4_CMD_SW2HW_CQ, MLX4_CMD_TIME_CLASS_A,
 			MLX4_CMD_WRAPPED);
 }
@@ -287,11 +287,61 @@ static void mlx4_cq_free_icm(struct mlx4_dev *dev, int cqn)
 		__mlx4_cq_free_icm(dev, cqn);
 }
 
+static int mlx4_init_user_cqes(void *buf, int entries, int cqe_size)
+{
+	int entries_per_copy = PAGE_SIZE / cqe_size;
+	void *init_ents;
+	int err = 0;
+	int i;
+
+	init_ents = kmalloc(PAGE_SIZE, GFP_KERNEL);
+	if (!init_ents)
+		return -ENOMEM;
+
+	/* Populate a list of CQ entries to reduce the number of
+	 * copy_to_user calls. 0xcc is the initialization value
+	 * required by the FW.
+	 */
+	memset(init_ents, 0xcc, PAGE_SIZE);
+
+	if (entries_per_copy < entries) {
+		for (i = 0; i < entries / entries_per_copy; i++) {
+			err = copy_to_user(buf, init_ents, PAGE_SIZE);
+			if (err)
+				goto out;
+
+			buf += PAGE_SIZE;
+		}
+	} else {
+		err = copy_to_user(buf, init_ents, entries * cqe_size);
+	}
+
+out:
+	kfree(init_ents);
+
+	return err;
+}
+
+static void mlx4_init_kernel_cqes(struct mlx4_buf *buf,
+				  int entries,
+				  int cqe_size)
+{
+	int i;
+
+	if (buf->nbufs == 1)
+		memset(buf->direct.buf, 0xcc, entries * cqe_size);
+	else
+		for (i = 0; i < buf->npages; i++)
+			memset(buf->page_list[i].buf, 0xcc,
+			       1UL << buf->page_shift);
+}
+
 int mlx4_cq_alloc(struct mlx4_dev *dev, int nent,
 		  struct mlx4_mtt *mtt, struct mlx4_uar *uar, u64 db_rec,
 		  struct mlx4_cq *cq, unsigned vector, int collapsed,
-		  int timestamp_en)
+		  int timestamp_en, void *buf_addr, bool user_cq)
 {
+	bool sw_cq_init = dev->caps.flags2 & MLX4_DEV_CAP_FLAG2_SW_CQ_INIT;
 	struct mlx4_priv *priv = mlx4_priv(dev);
 	struct mlx4_cq_table *cq_table = &priv->cq_table;
 	struct mlx4_cmd_mailbox *mailbox;
@@ -336,7 +386,20 @@ int mlx4_cq_alloc(struct mlx4_dev *dev, int nent,
 	cq_context->mtt_base_addr_l = cpu_to_be32(mtt_addr & 0xffffffff);
 	cq_context->db_rec_addr     = cpu_to_be64(db_rec);
 
-	err = mlx4_SW2HW_CQ(dev, mailbox, cq->cqn);
+	if (sw_cq_init) {
+		if (user_cq) {
+			err = mlx4_init_user_cqes(buf_addr, nent,
+						  dev->caps.cqe_size);
+			if (err)
+				sw_cq_init = false;
+		} else {
+			mlx4_init_kernel_cqes(buf_addr, nent,
+					      dev->caps.cqe_size);
+		}
+	}
+
+	err = mlx4_SW2HW_CQ(dev, mailbox, cq->cqn, sw_cq_init);
+
 	mlx4_free_cmd_mailbox(dev, mailbox);
 	if (err)
 		goto err_radix;
diff --git a/drivers/net/ethernet/mellanox/mlx4/en_cq.c b/drivers/net/ethernet/mellanox/mlx4/en_cq.c
index 1e487acb4667..062a88fcc5d6 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_cq.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_cq.c
@@ -143,7 +143,7 @@ int mlx4_en_activate_cq(struct mlx4_en_priv *priv, struct mlx4_en_cq *cq,
 	cq->mcq.usage = MLX4_RES_USAGE_DRIVER;
 	err = mlx4_cq_alloc(mdev->dev, cq->size, &cq->wqres.mtt,
 			    &mdev->priv_uar, cq->wqres.db.dma, &cq->mcq,
-			    cq->vector, 0, timestamp_en);
+			    cq->vector, 0, timestamp_en, &cq->wqres.buf, false);
 	if (err)
 		goto free_eq;
 
diff --git a/drivers/net/ethernet/mellanox/mlx4/fw.c b/drivers/net/ethernet/mellanox/mlx4/fw.c
index babcfd9c0571..7df728f1e5b5 100644
--- a/drivers/net/ethernet/mellanox/mlx4/fw.c
+++ b/drivers/net/ethernet/mellanox/mlx4/fw.c
@@ -166,6 +166,7 @@ static void dump_dev_cap_flags2(struct mlx4_dev *dev, u64 flags)
 		[37] = "sl to vl mapping table change event support",
 		[38] = "user MAC support",
 		[39] = "Report driver version to FW support",
+		[40] = "SW CQ initialization support",
 	};
 	int i;
 
@@ -1098,6 +1099,8 @@ int mlx4_QUERY_DEV_CAP(struct mlx4_dev *dev, struct mlx4_dev_cap *dev_cap)
 		dev_cap->flags2 |= MLX4_DEV_CAP_FLAG2_FSM;
 	if (field32 & (1 << 21))
 		dev_cap->flags2 |= MLX4_DEV_CAP_FLAG2_80_VFS;
+	if (field32 & (1 << 23))
+		dev_cap->flags2 |= MLX4_DEV_CAP_FLAG2_SW_CQ_INIT;
 
 	for (i = 1; i <= dev_cap->num_ports; i++) {
 		err = mlx4_QUERY_PORT(dev, i, dev_cap->port_cap + i);
diff --git a/include/linux/mlx4/device.h b/include/linux/mlx4/device.h
index dca6ab4eaa99..36e412c3d657 100644
--- a/include/linux/mlx4/device.h
+++ b/include/linux/mlx4/device.h
@@ -226,6 +226,7 @@ enum {
 	MLX4_DEV_CAP_FLAG2_SL_TO_VL_CHANGE_EVENT = 1ULL << 37,
 	MLX4_DEV_CAP_FLAG2_USER_MAC_EN		= 1ULL << 38,
 	MLX4_DEV_CAP_FLAG2_DRIVER_VERSION_TO_FW = 1ULL << 39,
+	MLX4_DEV_CAP_FLAG2_SW_CQ_INIT           = 1ULL << 40,
 };
 
 enum {
@@ -1136,7 +1137,8 @@ void mlx4_free_hwq_res(struct mlx4_dev *mdev, struct mlx4_hwq_resources *wqres,
 
 int mlx4_cq_alloc(struct mlx4_dev *dev, int nent, struct mlx4_mtt *mtt,
 		  struct mlx4_uar *uar, u64 db_rec, struct mlx4_cq *cq,
-		  unsigned vector, int collapsed, int timestamp_en);
+		  unsigned int vector, int collapsed, int timestamp_en,
+		  void *buf_addr, bool user_cq);
 void mlx4_cq_free(struct mlx4_dev *dev, struct mlx4_cq *cq);
 int mlx4_qp_reserve_range(struct mlx4_dev *dev, int cnt, int align,
 			  int *base, u8 flags, u8 usage);
-- 
1.8.3.1

^ permalink raw reply related

* RE: [PATCH] can: flexcan: Implement CAN Runtime PM
From: Joakim Zhang @ 2018-11-22  1:39 UTC (permalink / raw)
  To: Marc Kleine-Budde, wg@grandegger.com, davem@davemloft.net
  Cc: linux-can@vger.kernel.org, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org, dl-linux-imx, Aisheng DONG
In-Reply-To: <24a43a20-7302-c449-06b3-5ee8f6c051a0@pengutronix.de>

Hi Marc,

Best Regards,
Joakim Zhang

> -----Original Message-----
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: 2018年11月21日 22:36
> To: Joakim Zhang <qiangqing.zhang@nxp.com>; wg@grandegger.com;
> davem@davemloft.net
> Cc: linux-can@vger.kernel.org; netdev@vger.kernel.org;
> linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; Aisheng
> DONG <aisheng.dong@nxp.com>
> Subject: Re: [PATCH] can: flexcan: Implement CAN Runtime PM
> 
> On 10/19/18 11:45 AM, Joakim Zhang wrote:
> > From: Dong Aisheng <aisheng.dong@nxp.com>
> >
> > Flexcan will be disabled during suspend if no wakeup function required
> > and enabled after resume accordingly. During this period, we could
> > explicitly disable clocks.
> >
> > Implement Runtime PM which will:
> > 1) Keep device in suspend state (clocks disabled) if it's not openned
> > 2) Make Power Domain framework be able to shutdown the corresponding
> > power domain of this device.
> 
> Without CONFIG_PM the device fails to probe:
> 
> > [  214.420606] flexcan 2090000.flexcan: 2090000.flexcan supply xceiver
> > not found, using dummy regulator [  214.445952] flexcan
> > 2090000.flexcan: Linked as a consumer to regulator.0 [  214.453448]
> > flexcan 2090000.flexcan: registering netdev failed [  214.459666]
> > flexcan 2090000.flexcan: Dropping the link to regulator.0 [
> > 214.472066] flexcan: probe of 2090000.flexcan failed with error -110

I will firstly send the patch to do FlexCAN wakeup upstream, you could review the patch "can: flexcan: add self wakeup support" when you are free.
After you have reviewed, then I will send patch to do FlexCAN runtime PM upstream. It will be convenient for you to review the patch.

Thanks a lot for your review.

Best Regards,
Joakim Zhang

> Marc
> 
> --
> Pengutronix e.K.                  | Marc Kleine-Budde           |
> Industrial Linux Solutions        | Phone: +49-231-2826-924     |
> Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
> Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


^ permalink raw reply

* Re: [PATCH] mm: convert totalram_pages, totalhigh_pages and managed_pages to atomic.
From: Guo Ren @ 2018-11-22  1:33 UTC (permalink / raw)
  To: Arun KS
  Cc: Mike Snitzer, Benjamin Herrenschmidt, Kemi Wang,
	dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, J. Bruce Fields,
	linux-sctp-u79uwXL29TY76Z2rM5mHXA, Paul Mackerras, Pavel Machek,
	Christoph Lameter, K. Y. Srinivasan, Sumit Semwal,
	David (ChunMing) Zhou, Petr Tesarik, Michael Ellerman,
	ceph-devel-u79uwXL29TY76Z2rM5mHXA, James Morris,
	kasan-dev-/JYPxA39Uh5TLH3MbocFFw, Marcos Paulo de Souza,
	Steven J. Hill, David Rientjes, Anthony Yznaga, Daniel Vacek,
	Roman Gushchin, Len Brown
In-Reply-To: <1540229092-25207-1-git-send-email-arunks-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>

On Mon, Oct 22, 2018 at 10:53:22PM +0530, Arun KS wrote:
> Remove managed_page_count_lock spinlock and instead use atomic
> variables.
> 
> Suggested-by: Michal Hocko <mhocko-IBi9RG/b67k@public.gmane.org>
> Suggested-by: Vlastimil Babka <vbabka-AlSwsSmVLrQ@public.gmane.org>
> Signed-off-by: Arun KS <arunks-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> 
> ---
> As discussed here,
> https://patchwork.kernel.org/patch/10627521/#22261253
> ---
> ---
>  arch/csky/mm/init.c                           |  4 +-
>  arch/powerpc/platforms/pseries/cmm.c          | 11 ++--
>  arch/s390/mm/init.c                           |  2 +-
>  arch/um/kernel/mem.c                          |  4 +-
>  arch/x86/kernel/cpu/microcode/core.c          |  5 +-
>  drivers/char/agp/backend.c                    |  4 +-
>  drivers/gpu/drm/amd/amdkfd/kfd_crat.c         |  2 +-
>  drivers/gpu/drm/i915/i915_gem.c               |  2 +-
>  drivers/gpu/drm/i915/selftests/i915_gem_gtt.c |  4 +-
>  drivers/hv/hv_balloon.c                       | 19 +++----
>  drivers/md/dm-bufio.c                         |  5 +-
>  drivers/md/dm-crypt.c                         |  4 +-
>  drivers/md/dm-integrity.c                     |  4 +-
>  drivers/md/dm-stats.c                         |  3 +-
>  drivers/media/platform/mtk-vpu/mtk_vpu.c      |  3 +-
>  drivers/misc/vmw_balloon.c                    |  2 +-
>  drivers/parisc/ccio-dma.c                     |  5 +-
>  drivers/parisc/sba_iommu.c                    |  5 +-
>  drivers/staging/android/ion/ion_system_heap.c |  2 +-
>  drivers/xen/xen-selfballoon.c                 |  7 +--
>  fs/ceph/super.h                               |  3 +-
>  fs/file_table.c                               |  9 ++--
>  fs/fuse/inode.c                               |  4 +-
>  fs/nfs/write.c                                |  3 +-
>  fs/nfsd/nfscache.c                            |  3 +-
>  fs/ntfs/malloc.h                              |  2 +-
>  fs/proc/base.c                                |  3 +-
>  include/linux/highmem.h                       |  2 +-
>  include/linux/mm.h                            |  2 +-
>  include/linux/mmzone.h                        | 10 +---
>  include/linux/swap.h                          |  2 +-
>  kernel/fork.c                                 |  6 +--
>  kernel/kexec_core.c                           |  5 +-
>  kernel/power/snapshot.c                       |  2 +-
>  lib/show_mem.c                                |  3 +-
>  mm/highmem.c                                  |  2 +-
>  mm/huge_memory.c                              |  2 +-
>  mm/kasan/quarantine.c                         |  4 +-
>  mm/memblock.c                                 |  6 +--
>  mm/memory_hotplug.c                           |  4 +-
>  mm/mm_init.c                                  |  3 +-
>  mm/oom_kill.c                                 |  2 +-
>  mm/page_alloc.c                               | 75 ++++++++++++++-------------
>  mm/shmem.c                                    | 12 +++--
>  mm/slab.c                                     |  3 +-
>  mm/swap.c                                     |  3 +-
>  mm/util.c                                     |  2 +-
>  mm/vmalloc.c                                  |  4 +-
>  mm/vmstat.c                                   |  4 +-
>  mm/workingset.c                               |  2 +-
>  mm/zswap.c                                    |  2 +-
>  net/dccp/proto.c                              |  6 +--
>  net/decnet/dn_route.c                         |  2 +-
>  net/ipv4/tcp_metrics.c                        |  2 +-
>  net/netfilter/nf_conntrack_core.c             |  6 +--
>  net/netfilter/xt_hashlimit.c                  |  4 +-
>  net/sctp/protocol.c                           |  6 +--
>  security/integrity/ima/ima_kexec.c            |  2 +-
>  58 files changed, 171 insertions(+), 143 deletions(-)
> 
> diff --git a/arch/csky/mm/init.c b/arch/csky/mm/init.c
> index dc07c07..3f4d35e 100644
> --- a/arch/csky/mm/init.c
> +++ b/arch/csky/mm/init.c
> @@ -71,7 +71,7 @@ void free_initrd_mem(unsigned long start, unsigned long end)
>  		ClearPageReserved(virt_to_page(start));
>  		init_page_count(virt_to_page(start));
>  		free_page(start);
> -		totalram_pages++;
> +		atomic_long_inc(&totalram_pages);
>  	}
>  }
>  #endif
> @@ -88,7 +88,7 @@ void free_initmem(void)
>  		ClearPageReserved(virt_to_page(addr));
>  		init_page_count(virt_to_page(addr));
>  		free_page(addr);
> -		totalram_pages++;
> +		atomic_long_inc(&totalram_pages);
>  		addr += PAGE_SIZE;
>  	}
For csky part, it's OK.

 Guo Ren

^ permalink raw reply

* Re: hw csum failure + conntrack with more debugging information
From: Andre Tomt @ 2018-11-21 14:48 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: netdev, Cong Wang
In-Reply-To: <CANn89i+VooCQ9eTXWMjXWPvV0_LfKeQ_+aJbxBzdhSe8=7R6uw@mail.gmail.com>

On 18.11.2018 02:12, Eric Dumazet wrote:
> Please try this patch, we suspect mlx4 support for CHECKSUM_COMPLETE is 
> wrong.
> 
> (Only IPv4 handled, but I suspect a similar fix is needed for IPv6)

Not conclusive, but.. Have not seen any splats or other weirdness since 
applying this patch 3 days ago. Routing IPv4 only.

> root@pps:~# ethtool -S p0xe0 |grep sum
>      rx_csum_good: 530992724
>      rx_csum_none: 153315
>      rx_csum_complete: 188156
>      tx_chksum_offload: 213784665
> root@pps:~# ethtool -S p0xe1 |grep sum
>      rx_csum_good: 873708754
>      rx_csum_none: 160324
>      rx_csum_complete: 42605
>      tx_chksum_offload: 160708

^ permalink raw reply

* Re: [PATCH bpf-next v2] filter: add BPF_ADJ_ROOM_DATA mode to bpf_skb_adjust_room()
From: Nicolas Dichtel @ 2018-11-21 14:43 UTC (permalink / raw)
  To: Daniel Borkmann; +Cc: kafai, ast, davem, netdev
In-Reply-To: <a932d9f5-f0c1-0b70-9224-579f1f1e7f29@iogearbox.net>

Le 20/11/2018 à 02:06, Daniel Borkmann a écrit :
> On 11/13/2018 05:35 PM, Nicolas Dichtel wrote:
>> This new mode enables to add or remove an l2 header in a programmatic way
>> with cls_bpf.
>> For example, it enables to play with mpls headers.
>>
>> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
>> Acked-by: Martin KaFai Lau <kafai@fb.com>
> 
> (Sorry for late reply, swamped due to Plumbers.)
I thought so, no problem ;-)

> 
>> ---
>>
>> v2: use skb_set_network_header()
>>
>>  include/uapi/linux/bpf.h       |  3 ++
>>  net/core/filter.c              | 53 ++++++++++++++++++++++++++++++++++
>>  tools/include/uapi/linux/bpf.h |  3 ++
>>  3 files changed, 59 insertions(+)
>>
>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>> index 47d606d744cc..9762ef3a536c 100644
>> --- a/include/uapi/linux/bpf.h
>> +++ b/include/uapi/linux/bpf.h
>> @@ -1467,6 +1467,8 @@ union bpf_attr {
>>   *
>>   * 		* **BPF_ADJ_ROOM_NET**: Adjust room at the network layer
>>   * 		  (room space is added or removed below the layer 3 header).
>> + * 		* **BPF_ADJ_ROOM_DATA**: Adjust room at the beginning of the
>> + * 		  packet (room space is added or removed below skb->data).
> 
> Nit: in this case it would probably make more sense to name it BPF_ADJ_ROOM_MAC.
> BPF_ADJ_ROOM_DATA reads like we're adjusting payload data.
Yep, I first had in mind skb->data, but you're right.

> 
>>   * 		All values for *flags* are reserved for future usage, and must
>>   * 		be left at zero.
>> @@ -2412,6 +2414,7 @@ enum bpf_func_id {
>>  /* Mode for BPF_FUNC_skb_adjust_room helper. */
>>  enum bpf_adj_room_mode {
>>  	BPF_ADJ_ROOM_NET,
>> +	BPF_ADJ_ROOM_DATA,
>>  };
>>  
>>  /* Mode for BPF_FUNC_skb_load_bytes_relative helper. */
>> diff --git a/net/core/filter.c b/net/core/filter.c
>> index 53d50fb75ea1..e56da3077dca 100644
>> --- a/net/core/filter.c
>> +++ b/net/core/filter.c
>> @@ -2884,6 +2884,57 @@ static int bpf_skb_adjust_net(struct sk_buff *skb, s32 len_diff)
>>  	return ret;
>>  }
>>  
>> +static int bpf_skb_data_shrink(struct sk_buff *skb, u32 len)
>> +{
>> +	unsigned short hhlen = skb->dev->header_ops ?
>> +			       skb->dev->hard_header_len : 0;
>> +	int ret;
>> +
>> +	ret = skb_unclone(skb, GFP_ATOMIC);
>> +	if (unlikely(ret < 0))
>> +		return ret;
>> +
>> +	__skb_pull(skb, len);
>> +	skb_reset_mac_header(skb);
>> +	skb_set_network_header(skb, hhlen);
>> +	skb_reset_transport_header(skb);
>> +	return 0;
>> +}
>> +
>> +static int bpf_skb_data_grow(struct sk_buff *skb, u32 len)
>> +{
>> +	unsigned short hhlen = skb->dev->header_ops ?
>> +			       skb->dev->hard_header_len : 0;
>> +	int ret;
>> +
>> +	ret = skb_cow(skb, len);
>> +	if (unlikely(ret < 0))
>> +		return ret;
>> +
>> +	skb_push(skb, len);
>> +	skb_reset_mac_header(skb);
> 
> Looks like this could leak uninitialized mem into the BPF prog as it's
> not zeroed out. Can't we just reuse bpf_skb_generic_push() and the
> bpf_skb_generic_pop()?
Hmm, at some point, it was not possible, but it was an intermediate version and
I don't see why now.

> 
> bpf_skb_vlan_push() and bpf_skb_vlan_pop() do a bpf_push_mac_rcsum() /
> bpf_pull_mac_rcsum() in order to not screw up things like csum complete.
> Wouldn't this be needed here as well?
> 
> How does this interact with MPLS GSO?
I overlooked both points.

> 
> If the packet gets pushed back into the stack, would AF_MPLS handle it?
> Probably good to add test cases to BPF kselftest suite with it.
Ok, I will have a look to this.

> 
> Don't we need to reject the packet in case of skb->encapsulation?
> 
> Looking at push_mpls() and pop_mpls() from OVS implementation, do we
> also need to deal with skb inner network header / proto?
> 
> Given all this would it rather make sense to add MPLS specific helpers
> in similar fashion to the vlan ones we have?
> 
> Is there any other use case aside from MPLS?
Yes, I was targeting a generic encap/decap at l2 level. Maybe more complex than
I thought.

> 
>> +	return 0;
>> +}
>> +
>> +static int bpf_skb_adjust_data(struct sk_buff *skb, s32 len_diff)
>> +{
>> +	u32 len_diff_abs = abs(len_diff);
>> +	bool shrink = len_diff < 0;
>> +	int ret;
>> +
>> +	if (unlikely(len_diff_abs > 0xfffU))
>> +		return -EFAULT;
>> +
>> +	if (shrink && len_diff_abs >= skb_headlen(skb))
>> +		return -EFAULT;
>> +
>> +	ret = shrink ? bpf_skb_data_shrink(skb, len_diff_abs) :
>> +		       bpf_skb_data_grow(skb, len_diff_abs);
> 
> Hmm, I think this has a bug in that the above two do not adjust
> skb->mac_len. Then things like cls_bpf_classify() will __skb_pull()
> based on the old mac_len, getting you to a wrong offset in the
> packet when this is for example pushed back into ingress, etc.
Nice catch!


Thank you,
Nicolas

^ permalink raw reply

* [PATCH net] net/dim: Update DIM start sample after each DIM iteration
From: Tal Gilboa @ 2018-11-21 14:28 UTC (permalink / raw)
  To: David Miller
  Cc: netdev, Tariq Toukan, Tal Gilboa, Saeed Mahameed,
	Florian Fainelli, Andy Gospodarek

On every iteration of net_dim, the algorithm may choose to
check for the system state by comparing current data sample
with previous data sample. After each of these comparison,
regardless of the action taken, the sample used as baseline
is needed to be updated.

This patch fixes a bug that causes DIM to take wrong decisions,
due to never updating the baseline sample for comparison between
iterations. This way, DIM always compares current sample with
zeros.

Although this is a functional fix, it also improves and stabilizes
performance as the algorithm works properly now.

Performance:
Tested single UDP TX stream with pktgen:
samples/pktgen/pktgen_sample03_burst_single_flow.sh -i p4p2 -d 1.1.1.1
-m 24:8a:07:88:26:8b -f 3 -b 128

ConnectX-5 100GbE packet rate improved from 15-19Mpps to 19-20Mpps.
Also, toggling between profiles is less frequent with the fix.

Fixes: 8115b750dbcb ("net/dim: use struct net_dim_sample as arg to net_dim")
Signed-off-by: Tal Gilboa <talgi@mellanox.com>
Reviewed-by: Tariq Toukan <tariqt@mellanox.com>
---
 include/linux/net_dim.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/net_dim.h b/include/linux/net_dim.h
index c79e859..fd45838 100644
--- a/include/linux/net_dim.h
+++ b/include/linux/net_dim.h
@@ -406,6 +406,8 @@ static inline void net_dim(struct net_dim *dim,
 		}
 		/* fall through */
 	case NET_DIM_START_MEASURE:
+		net_dim_sample(end_sample.event_ctr, end_sample.pkt_ctr, end_sample.byte_ctr,
+			       &dim->start_sample);
 		dim->state = NET_DIM_MEASURE_IN_PROGRESS;
 		break;
 	case NET_DIM_APPLY_NEW_PROFILE:
-- 
1.8.3.1

^ permalink raw reply related

* Re: [PATCH v2] net: faraday: ftmac100: remove netif_running(netdev) check before disabling interrupts
From: David Miller @ 2018-11-22  0:58 UTC (permalink / raw)
  To: vincentc; +Cc: gregkh, rdunlap, yuehaibing, netdev, linux-kernel
In-Reply-To: <1542764291-11208-1-git-send-email-vincentc@andestech.com>

From: Vincent Chen <vincentc@andestech.com>
Date: Wed, 21 Nov 2018 09:38:11 +0800

> In the original ftmac100_interrupt(), the interrupts are only disabled when
> the condition "netif_running(netdev)" is true. However, this condition
> causes kerenl hang in the following case. When the user requests to
> disable the network device, kernel will clear the bit __LINK_STATE_START
> from the dev->state and then call the driver's ndo_stop function. Network
> device interrupts are not blocked during this process. If an interrupt
> occurs between clearing __LINK_STATE_START and stopping network device,
> kernel cannot disable the interrupts due to the condition
> "netif_running(netdev)" in the ISR. Hence, kernel will hang due to the
> continuous interruption of the network device.
> 
> In order to solve the above problem, the interrupts of the network device
> should always be disabled in the ISR without being restricted by the
> condition "netif_running(netdev)".
> 
> [V2]
> Remove unnecessary curly braces.
> 
> Signed-off-by: Vincent Chen <vincentc@andestech.com>

Applied.

^ permalink raw reply

* Re: [PATCH net-next] net/ncsi: Add NCSI Mellanox OEM command
From: David Miller @ 2018-11-22  0:52 UTC (permalink / raw)
  To: vijaykhemka
  Cc: sam, netdev, linux-kernel, openbmc, Justin.Lee1, joel,
	linux-aspeed
In-Reply-To: <20181120203516.3460764-1-vijaykhemka@fb.com>

From: Vijay Khemka <vijaykhemka@fb.com>
Date: Tue, 20 Nov 2018 12:35:16 -0800

> +static int ncsi_oem_gma_handler_mlx(struct ncsi_cmd_arg *nca)
> +{
> +	unsigned char data[NCSI_OEM_MLX_CMD_GMA_LEN];
> +	int ret = 0;
> +
> +	nca->payload = NCSI_OEM_MLX_CMD_GMA_LEN;
> +
> +	memset(data, 0, NCSI_OEM_MLX_CMD_GMA_LEN);
> +	*(unsigned int *)data = ntohl(NCSI_OEM_MFR_MLX_ID);

'data' is not necessarily aligned on an unsigned int boundary.

But second of all you want to use a fixed size type like "u32" or
similar here, not "unsigned int".

Altogether, something like:

	union {
		u8 data_u8[NCSI_OEM_MLX_CMD_GMA_LEN];
		u32 data_u32[[NCSI_OEM_MLX_CMD_GMA_LEN / sizeof(u32)];
	} u;

	memset(&u, 0, sizeof(u));
	u.data_u32[0] = ntohl(NCSI_OEM_MFR_MLX_ID);
	u.data_u8[5] = NCSI_OEM_MLX_CMD_GMA;
	u.data_u8[6] = NCSI_OEM_MLX_CMD_GMA_PARAM;

If the rest of the ncsi driver has crud like this, it all needs
to be fixed up similarly.

^ permalink raw reply

* Re: [RFC v3 3/3] vxlan: handle underlay VRF changes
From: Alexis Bauvin @ 2018-11-21 14:05 UTC (permalink / raw)
  To: David Ahern; +Cc: Roopa Prabhu, netdev, akherbouche
In-Reply-To: <c56b5c57-98c7-865a-8089-d0017ad3a625@cumulusnetworks.com>

Le 20 nov. 2018 à 18:09, David Ahern <dsa@cumulusnetworks.com> a écrit :
> On 11/20/18 9:58 AM, Alexis Bauvin wrote:
>> A socket bound to vrf-blue listens on *:4789, thus owning the port. If moving an
>> underlay to the default vrf (ip link set dummy-b nomaster), a new socket will be
>> created, unbound to any interface and listening on *:4789. However, because it
>> will be in the default vrf, it will try to take ownership of port 4789 on ALL
>> vrfs, and fail because this port is already owned in vrf-blue for vxlan-a.
> 
> SO_REUSEPORT will fix that and incoming traffic through a vrf and
> default (non-)vrf will work. The recent changes by Vyatta provide even
> better isolation of default vrf and overlapping ports.

Did not think about that one, I will give it a shot.

There is one issue I can see with SO_REUSEPORT (if my understanding of it is
correct). From what I understood, enabling this option will balance incoming
connections (for TCP) / dgrams (for UDP) based on a 4-tuple hash (sip, dip,
sport, dport) between sockets listening on the same port.

If we have two separate vxlan fabrics, with one underlay in the default vrf, and
another in some random vrf. Since the socket for the default vrf would own the
port on all vrfs, the port would effectively be reused between the two vrfs.
Wouldn't vxlan packets be directed to "random" (as in not related to the vxlan
fabric itself) sockets, meaning a complete mix of the two fabrics? This would
imply a complete drop of the packets not directed to the correct socket.

I guess the Vyatta changes you are talking about are "vrf: allow simultaneous
service instances in default and other VRFs"? If so, it looks like it would
solve the default vrf problem, not even requiring SO_REUSEPORT.

Thanks!

^ permalink raw reply

* Re: [PATCH iproute2-next v3] rdma: Document IB device renaming option
From: Dennis Dalessandro @ 2018-11-21 13:59 UTC (permalink / raw)
  To: Leon Romanovsky, Ruhl, Michael J
  Cc: David Ahern, netdev, RDMA mailing list, Stephen Hemminger
In-Reply-To: <20181118110535.GJ24359@mtr-leonro.mtl.com>

On 11/18/2018 6:05 AM, Leon Romanovsky wrote:
> On Fri, Nov 16, 2018 at 08:10:35PM +0000, Ruhl, Michael J wrote:
>>> -----Original Message-----
>>> From: linux-rdma-owner@vger.kernel.org [mailto:linux-rdma-
>>> owner@vger.kernel.org] On Behalf Of Leon Romanovsky
>>> Sent: Sunday, November 4, 2018 2:11 PM
>>> To: David Ahern <dsahern@gmail.com>
>>> Cc: Leon Romanovsky <leonro@mellanox.com>; netdev
>>> <netdev@vger.kernel.org>; RDMA mailing list <linux-rdma@vger.kernel.org>;
>>> Stephen Hemminger <stephen@networkplumber.org>
>>> Subject: [PATCH iproute2-next v3] rdma: Document IB device renaming
>>> option
>>>
>>> From: Leon Romanovsky <leonro@mellanox.com>
>>
>> Hi Leon,
>>
>> After looking at this and Steve Wise's changes for the ADDLINK/DELLINK,
>> it occurred to me that the driver that handed the name to ib_register_device()
>> might be interested in knowing that this name change occurred.
>>
>> Are there plans to include a some kind of notify mechanism so drivers can
>> find out when things like this occur?
> 
> At least for device rename, I don't see any real need for such event,
> because drivers are not supposed to rely on names.

It just seems like something they want to know. But agree driver should 
not rely on a name.

> I would say that it is probably driver bug to rely on device name
> during its execution.
> 
>>
>> Is this something that should be done?
> 
> I think yes, we can extend ib_event to support more events than now,
> but should we?

On one hand it make sense to have an event. In case the driver had 
something it wanted to do with the name but I can't really think of a 
compelling reason it does. So I'd say we don't worry about it now but if 
someone finds a use for it then adding it later should be an option that 
is on the table.

-Denny

^ permalink raw reply

* [PATCH net] net/ipv6: re-do dad when interface has IFF_NOARP flag change
From: Hangbin Liu @ 2018-11-21 13:52 UTC (permalink / raw)
  To: netdev
  Cc: Stefano Brivio, Paolo Abeni, David Miller, YOSHIFUJI Hideaki,
	Mahesh Bandewar, Hangbin Liu

When we add a new IPv6 address, we should also join corresponding solicited-node
multicast address, unless the interface has IFF_NOARP flag, as function
addrconf_join_solict() did. But if we remove IFF_NOARP flag later, we do
not do dad and add the mcast address. So we will drop corresponding neighbour
discovery message that came from other nodes.

A typical example is after creating a ipvlan with mode l3, setting up an ipv6
address and changing the mode to l2. Then we will not be able to ping this
address as the interface doesn't join related solicited-node mcast address.

Fix it by re-doing dad when interface changed IFF_NOARP flag. Then we will add
corresponding mcast group and check if there is a duplicate address on the
network.

Reported-by: Jianlin Shi <jishi@redhat.com>
Reviewed-by: Stefano Brivio <sbrivio@redhat.com>
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 net/ipv6/addrconf.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 63a808d..045597b 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -179,7 +179,7 @@ static void addrconf_dad_start(struct inet6_ifaddr *ifp);
 static void addrconf_dad_work(struct work_struct *w);
 static void addrconf_dad_completed(struct inet6_ifaddr *ifp, bool bump_id,
 				   bool send_na);
-static void addrconf_dad_run(struct inet6_dev *idev);
+static void addrconf_dad_run(struct inet6_dev *idev, bool restart);
 static void addrconf_rs_timer(struct timer_list *t);
 static void __ipv6_ifa_notify(int event, struct inet6_ifaddr *ifa);
 static void ipv6_ifa_notify(int event, struct inet6_ifaddr *ifa);
@@ -3439,6 +3439,7 @@ static int addrconf_notify(struct notifier_block *this, unsigned long event,
 			   void *ptr)
 {
 	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
+	struct netdev_notifier_change_info *change_info;
 	struct netdev_notifier_changeupper_info *info;
 	struct inet6_dev *idev = __in6_dev_get(dev);
 	struct net *net = dev_net(dev);
@@ -3513,7 +3514,7 @@ static int addrconf_notify(struct notifier_block *this, unsigned long event,
 				break;
 			}
 
-			if (idev) {
+			if (!IS_ERR_OR_NULL(idev)) {
 				if (idev->if_flags & IF_READY) {
 					/* device is already configured -
 					 * but resend MLD reports, we might
@@ -3521,6 +3522,9 @@ static int addrconf_notify(struct notifier_block *this, unsigned long event,
 					 * multicast snooping switches
 					 */
 					ipv6_mc_up(idev);
+					change_info = ptr;
+					if (change_info->flags_changed & IFF_NOARP)
+						addrconf_dad_run(idev, true);
 					rt6_sync_up(dev, RTNH_F_LINKDOWN);
 					break;
 				}
@@ -3555,7 +3559,7 @@ static int addrconf_notify(struct notifier_block *this, unsigned long event,
 
 		if (!IS_ERR_OR_NULL(idev)) {
 			if (run_pending)
-				addrconf_dad_run(idev);
+				addrconf_dad_run(idev, false);
 
 			/* Device has an address by now */
 			rt6_sync_up(dev, RTNH_F_DEAD);
@@ -4173,16 +4177,19 @@ static void addrconf_dad_completed(struct inet6_ifaddr *ifp, bool bump_id,
 		addrconf_verify_rtnl();
 }
 
-static void addrconf_dad_run(struct inet6_dev *idev)
+static void addrconf_dad_run(struct inet6_dev *idev, bool restart)
 {
 	struct inet6_ifaddr *ifp;
 
 	read_lock_bh(&idev->lock);
 	list_for_each_entry(ifp, &idev->addr_list, if_list) {
 		spin_lock(&ifp->lock);
-		if (ifp->flags & IFA_F_TENTATIVE &&
-		    ifp->state == INET6_IFADDR_STATE_DAD)
+		if ((ifp->flags & IFA_F_TENTATIVE &&
+		     ifp->state == INET6_IFADDR_STATE_DAD) || restart) {
+			if (restart)
+				ifp->state = INET6_IFADDR_STATE_PREDAD;
 			addrconf_dad_kick(ifp);
+		}
 		spin_unlock(&ifp->lock);
 	}
 	read_unlock_bh(&idev->lock);
-- 
2.5.5

^ permalink raw reply related

* Re: [PATCH v2 14/14] nvme-tcp: add NVMe over TCP host driver
From: Mikhail Skorzhinskii @ 2018-11-21 12:01 UTC (permalink / raw)
  To: Sagi Grimberg
  Cc: linux-nvme, linux-block, netdev, David S. Miller, Keith Busch,
	Christoph Hellwig
In-Reply-To: <20181120030019.31738-16-sagi@grimberg.me>

Sagi Grimberg <sagi@grimberg.me> writes:
 > +static inline void nvme_tcp_queue_request(struct nvme_tcp_request *req)
 > +{
 > +	struct nvme_tcp_queue *queue = req->queue;
 > +
 > +	spin_lock_bh(&queue->lock);
 > +	list_add_tail(&req->entry, &queue->send_list);
 > +	spin_unlock_bh(&queue->lock);
 > +
 > +	queue_work_on(queue->io_cpu, nvme_tcp_wq, &queue->io_work);
 > +}

May be I missing something, but why bother with bottom half version of
locking?

There are few places where this lock could be accessed:

 (1) From ->queue_rq() call;
 (2) From submitting new AEN request;
 (3) From receiving new R2T;

Which one if these originates from bottom half? Not 100% about queue_rq
data path, but (2) and (3) looks perfectly safe for me.

Possibly just a relic of some previous iterations of experimenting?

Mikhail Skorzhinskii

^ permalink raw reply

* [PATCH net-next] selftests: explicitly require kernel features needed by udpgro tests
From: Paolo Abeni @ 2018-11-21 13:31 UTC (permalink / raw)
  To: netdev; +Cc: Naresh Kamboju, David S. Miller

commit 3327a9c46352f1 ("selftests: add functionals test for UDP GRO")
make use of ipv6 NAT, but such a feature is not currently implied by
selftests. Since the 'ip[6]tables' commands may actually create nft rules,
depending on the specific user-space version, let's pull both NF and
NFT nat modules plus the needed deps.

Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
Fixes: 3327a9c46352f1 ("selftests: add functionals test for UDP GRO")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 tools/testing/selftests/net/config | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/tools/testing/selftests/net/config b/tools/testing/selftests/net/config
index cd3a2f1545b5..5821bdd98d20 100644
--- a/tools/testing/selftests/net/config
+++ b/tools/testing/selftests/net/config
@@ -14,3 +14,17 @@ CONFIG_IPV6_VTI=y
 CONFIG_DUMMY=y
 CONFIG_BRIDGE=y
 CONFIG_VLAN_8021Q=y
+CONFIG_NETFILTER=y
+CONFIG_NETFILTER_ADVANCED=y
+CONFIG_NF_CONNTRACK=m
+CONFIG_NF_NAT_IPV6=m
+CONFIG_NF_NAT_IPV4=m
+CONFIG_IP6_NF_IPTABLES=m
+CONFIG_IP_NF_IPTABLES=m
+CONFIG_IP6_NF_NAT=m
+CONFIG_IP_NF_NAT=m
+CONFIG_NF_TABLES=m
+CONFIG_NF_TABLES_IPV6=y
+CONFIG_NF_TABLES_IPV4=y
+CONFIG_NFT_CHAIN_NAT_IPV6=m
+CONFIG_NFT_CHAIN_NAT_IPV4=m
-- 
2.17.2

^ permalink raw reply related

* Re: [RFC v3 0/3] Add VRF support for VXLAN underlay
From: Alexis Bauvin @ 2018-11-21 13:30 UTC (permalink / raw)
  To: David Ahern, roopa; +Cc: netdev, akherbouche, Alexis Bauvin
In-Reply-To: <3c4788fb-c71e-f626-138b-2a06a527b21a@cumulusnetworks.com>

Le 20 nov. 2018 à 22:45, David Ahern <dsa@cumulusnetworks.com> a écrit :
> 
> On 11/20/18 7:23 AM, Alexis Bauvin wrote:
>> We are trying to isolate the VXLAN traffic from different VMs with VRF as shown
>> in the schemas below:
>> 
>> +-------------------------+   +----------------------------+
>> | +----------+            |   |     +------------+         |
>> | |          |            |   |     |            |         |
>> | | tap-red  |            |   |     |  tap-blue  |         |
>> | |          |            |   |     |            |         |
>> | +----+-----+            |   |     +-----+------+         |
>> |      |                  |   |           |                |
>> |      |                  |   |           |                |
>> | +----+---+              |   |      +----+----+           |
>> | |        |              |   |      |         |           |
>> | | br-red |              |   |      | br-blue |           |
>> | |        |              |   |      |         |           |
>> | +----+---+              |   |      +----+----+           |
>> |      |                  |   |           |                |
>> |      |                  |   |           |                |
>> |      |                  |   |           |                |
>> | +----+--------+         |   |     +--------------+       |
>> | |             |         |   |     |              |       |
>> | |  vxlan-red  |         |   |     |  vxlan-blue  |       |
>> | |             |         |   |     |              |       |
>> | +------+------+         |   |     +-------+------+       |
>> |        |                |   |             |              |
>> |        |           VRF  |   |             |          VRF |
>> |        |           red  |   |             |         blue |
>> +-------------------------+   +----------------------------+
> 
> Roopa and I were discussing this setup and are puzzled by the VRF
> association here. Does br-red and br-blue have an address? The commands
> below do not show it and from our perspective seems odd for this
> scenario. If it does not have an address, then there is no reason for
> the VRF labeling.

Yes, br-red and br-blue have an address. Addresses (both MAC and IP)
are anycast addresses, to make an anycast gateway for the VMs behind
the taps. This serves for a classical evpn symmetric distributed
routing, both between vxlans and towards external networks. I am using
FRR as a control plane to exchange bgp-evpn information between
hypervisors.

The schematic could have been more complete, including more bridges
and more taps in the vrf, with an added bridge+vxlan for the l3vni.
I did not include them as I wanted to focus on the underlay itself
and not on what vxlan was used for.

Here is a more complete example:
                                   +---------+
                                   |         |
                                   | vrf1000 |
                                   |         |
                                   +--+-+-+--+
                                      | | |
                +---------------------+ | +---------------+
                |                       |                 |
         +------+------+         +------+------+     +----+---+
         |             |         |             |     |        |
         |     br0     |         |     br1     |     | br1000 |
         | 10.0.0.1/24 |         | 10.0.1.1/24 |     |        |
         |             |         |             |     +----+---+
         +----+-+-+----+         +-----+-+-----+          |
              | | |                    | |                |
      +-------+ | +-------+         +--+ +---+            |
      |         |         |         |        |            |
   +--+---+ +---+--+ +----+---+ +---+--+ +---+----+ +-----+-----+
   |      | |      | |        | |      | |        | |           |
   | tap0 | | tap1 | | vxlan0 | | tap2 | | vxlan1 | | vxlan1000 |
   |      | |      | |        | |      | |        | |           |
   +------+ +------+ +--------+ +------+ +--------+ +-----------+
                          .                   .           .
. . . . . . . . . . . . . . . . . . . . . . . . . . . . . .
.
.                    +---------+                  +--------------+
.                    |         |                  |              |
.                    | vrf1001 |                  | vrf-underlay |
.                    |         |                  |              |
.                    +---+-+---+                  +-------+------+
.                        | |                              |
.              +---------+ +----------+                   |
.              |                      |                   |
.       +------+------+          +----+---+               |
.       |             |          |        |               |
.       |     br2     |          | br1001 |               |
.       | 10.0.2.1/24 |          |        |               |
.       |             |          +----+---+               |
.       +----+-+-+----+               |                   |
.            | | |                    |                   |
.      +-----+ | +-------+            |                   |
.      |       |         |            |           +-------+-------+
.  +---+--+ +--+---+ +---+----+ +-----+-----+     |               |
.  |      | |      | |        | |           |     |   eth0.2030   |
.  | tap3 | | tap4 | | vxlan2 | | vxlan1001 |     | 172.16.0.2/24 |
.  |      | |      | |        | |           |     |               |
.  +------+ +------+ +--------+ +-----------+     +---------------+
.                         .           .                 ^  .
. . . . . . . . . . . . . . . . . . . . . . . . . . . . .  .
           (vxlan lower device is eth0.2030)               v
                                                 +----------------+
                                                 |                |
                                                 |      eth0      |
                                                 | 192.168.1.2/24 |
                                                 |                |
                                                 +----------------+

In this example, the underlay is rather simple. A more complex case
could be with several uplinks in the underlay vrf, and a dummy/loopback
with a /32 ip, used as a vxlan lower device and to bind routing daemons
(e.g. bgpd).

                  +--------------+
                  |              |
                  | vrf-underlay |
                  |              |
                  +-----+-+-+----+
                        | | |
        +---------------+ | +---------------+
        |                 |                 |
+-------+-------+ +-------+-------+ +-------+--------+
|               | |               | |                |
|     eth1      | |     eth2      | |     dummy0     |
| 172.16.0.2/31 | | 172.16.0.4/31 | | 192.168.0.1/32 |
|               | |               | |                |
+---------------+ +---------------+ +----------------+
                                            .
. . . . . . . . . . . . . . . . . . . . . . .
        (vxlan lower device is dummy0)

As for default vrf, it would contain for example management traffic. We
don’t want to mix the underlay routing tables to other traffics.

> Also, it would be good to have a unit test this case. Can you create a
> shell script that creates the setup and runs a few tests verifying
> connectivity? You can use network namespaces and veth pairs in place of
> the VM with a tap device. From there the functionality is the same.
> Tests can be initial VRF association for the vxlan lower device,
> changing the VRF to another device, and then changing again back to
> default VRF - checking proper connectivity for each.

Sure! I’ve just finished writing it, but I am not sure of the best way
so send it. I guess I will have to add it to a v4 of the patches, in
tools/testing/selftests/net?

> Thanks

^ permalink raw reply

* Re: [PATCH net] sctp: hold transport before accessing its asoc in sctp_hash_transport
From: Neil Horman @ 2018-11-21 13:27 UTC (permalink / raw)
  To: Marcelo Ricardo Leitner; +Cc: Xin Long, network dev, linux-sctp, davem
In-Reply-To: <20181121004625.GC3601@localhost.localdomain>

On Tue, Nov 20, 2018 at 10:46:26PM -0200, Marcelo Ricardo Leitner wrote:
> On Tue, Nov 20, 2018 at 07:52:48AM -0500, Neil Horman wrote:
> > On Tue, Nov 20, 2018 at 07:09:16PM +0800, Xin Long wrote:
> > > In sctp_hash_transport, it dereferences a transport's asoc only under
> > > rcu_read_lock. Without holding the transport, its asoc could be freed
> > > already, which leads to a use-after-free panic.
> > > 
> > > A similar fix as Commit bab1be79a516 ("sctp: hold transport before
> > > accessing its asoc in sctp_transport_get_next") is needed to hold
> > > the transport before accessing its asoc in sctp_hash_transport.
> > > 
> > > Fixes: cd2b70875058 ("sctp: check duplicate node before inserting a new transport")
> > > Reported-by: syzbot+0b05d8aa7cb185107483@syzkaller.appspotmail.com
> > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > ---
> > >  net/sctp/input.c | 7 ++++++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/net/sctp/input.c b/net/sctp/input.c
> > > index 5c36a99..69584e9 100644
> > > --- a/net/sctp/input.c
> > > +++ b/net/sctp/input.c
> > > @@ -896,11 +896,16 @@ int sctp_hash_transport(struct sctp_transport *t)
> > >  	list = rhltable_lookup(&sctp_transport_hashtable, &arg,
> > >  			       sctp_hash_params);
> > >  
> > > -	rhl_for_each_entry_rcu(transport, tmp, list, node)
> > > +	rhl_for_each_entry_rcu(transport, tmp, list, node) {
> > > +		if (!sctp_transport_hold(transport))
> > > +			continue;
> > >  		if (transport->asoc->ep == t->asoc->ep) {
> > > +			sctp_transport_put(transport);
> > >  			rcu_read_unlock();
> > >  			return -EEXIST;
> > >  		}
> > > +		sctp_transport_put(transport);
> > > +	}
> > >  	rcu_read_unlock();
> > >  
> > >  	err = rhltable_insert_key(&sctp_transport_hashtable, &arg,
> > > -- 
> > > 2.1.0
> > > 
> > > 
> > 
> > something doesn't feel at all right about this.  If we are inserting a transport
> > to an association, it would seem to me that we should have at least one user of
> > the association (i.e. non-zero refcount).  As such it seems something is wrong
> > with the association refcount here.  At the very least, if there is a case where
> > an association is being removed while a transport is being added, the better
> > solution would be to ensure that sctp_association_destroy goes through a
> > quiescent point prior to unhashing transports from the list, to ensure that
> > there is no conflict with the add operation above.
> 
> Consider that the rhl_for_each_entry_rcu() is traversing the global
> rhashtable, and that it may operate on unrelated transports/asocs.
> E.g., transport->asoc in the for() is potentially different from the
> asoc under socket lock.
> 
Ah, ok, we're comparing associations that are not related to the association
being searched for, that makes sense.

> The core of the fix is at:
> +		if (!sctp_transport_hold(transport))
> +			continue;
> If we can get a hold, the asoc will be available for dereferencing in
> subsequent lines. Otherwise, move on.
> 
> With that, the patch makes sense to me.
> 
Yes, I agree, but as you note below, this still seems like a lousy way to fix
the problem.

> Although I would prefer if we come up with a better way to do this
> jump, or even avoid the jump. We are only comparing pointers here and
> if we had asoc->ep cached on sctp_transport itself, we could avoid the
> atomics here.
> 
> This change, in the next patch on sctp_epaddr_lookup_transport, will
> hurt performance as that is called in datapath. Rhashtable will help
> on keeping entry lists to a size, but still.
> 
I still think the rcu_read_lock would be sufficient here, if we just ensured
that removals from the list occured after a quiescent point.  The lookup is in
the datapath, but adds/removes can have a little more latency added to them, and
if it removes the atomic operation from the fast path, I think thats a net win.

Neil

>   Marcelo
> 

^ permalink raw reply

* Re: pull-request: wireless-drivers 2018-11-20
From: David Miller @ 2018-11-21 23:51 UTC (permalink / raw)
  To: kvalo; +Cc: linux-wireless, netdev, linux-kernel
In-Reply-To: <87k1l7bwps.fsf@kamboji.qca.qualcomm.com>

From: Kalle Valo <kvalo@codeaurora.org>
Date: Tue, 20 Nov 2018 17:22:07 +0200

> here's a pull request to net tree for 4.20, more info below. Please let
> me know if there are any problems.

Pulled, thank you.

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox