* Re: [PATCH 1/4] net: dsa: Change DT bindings for Vitesse VSC73xx switches
From: Florian Fainelli @ 2019-07-01 16:44 UTC (permalink / raw)
To: Pawel Dembicki
Cc: linus.walleij, Andrew Lunn, Vivien Didelot, David S. Miller,
Rob Herring, Mark Rutland, netdev, devicetree, linux-kernel
In-Reply-To: <20190701152723.624-1-paweldembicki@gmail.com>
On 7/1/19 8:27 AM, Pawel Dembicki wrote:
> This commit document changes after split vsc73xx driver into core and
> spi part. The change of DT bindings is required for support the same
> vsc73xx chip, which need PI bus to communicate with CPU. It also
> introduce how to use vsc73xx platform driver.
>
> Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
> ---
> .../bindings/net/dsa/vitesse,vsc73xx.txt | 74 ++++++++++++++++---
> 1 file changed, 64 insertions(+), 10 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/net/dsa/vitesse,vsc73xx.txt b/Documentation/devicetree/bindings/net/dsa/vitesse,vsc73xx.txt
> index ed4710c40641..c6a4cd85891c 100644
> --- a/Documentation/devicetree/bindings/net/dsa/vitesse,vsc73xx.txt
> +++ b/Documentation/devicetree/bindings/net/dsa/vitesse,vsc73xx.txt
> @@ -2,8 +2,8 @@ Vitesse VSC73xx Switches
> ========================
>
> This defines device tree bindings for the Vitesse VSC73xx switch chips.
> -The Vitesse company has been acquired by Microsemi and Microsemi in turn
> -acquired by Microchip but retains this vendor branding.
> +The Vitesse company has been acquired by Microsemi and Microsemi has
> +been acquired Microchip but retains this vendor branding.
>
> The currently supported switch chips are:
> Vitesse VSC7385 SparX-G5 5+1-port Integrated Gigabit Ethernet Switch
> @@ -11,16 +11,26 @@ Vitesse VSC7388 SparX-G8 8-port Integrated Gigabit Ethernet Switch
> Vitesse VSC7395 SparX-G5e 5+1-port Integrated Gigabit Ethernet Switch
> Vitesse VSC7398 SparX-G8e 8-port Integrated Gigabit Ethernet Switch
>
> -The device tree node is an SPI device so it must reside inside a SPI bus
> -device tree node, see spi/spi-bus.txt
> +This switch could have two different management interface.
> +
> +If SPI interface is used, the device tree node is an SPI device so it must
> +reside inside a SPI bus device tree node, see spi/spi-bus.txt
> +
> +If Platform driver is used, the device tree node is an platform device so it
> +must reside inside a platform bus device tree node.
That should not be required because the device remains the same and how
it connects to the host is entirely described by the Device Tree topology.
Take b53 for instance which supports MDIO and SPI by default, and
optionally memory mapped and SRAB (indirect memory map) accesses, they
all have the same compatible strings. Whether the switches will appear
as spi_device, platform_device, or something else is entirely based on
how the Device Tree is laid out.
--
Florian
^ permalink raw reply
* [PATCH net-next 7/7] net/rds: Initialize ic->i_fastreg_wrs upon allocation
From: Gerd Rausch @ 2019-07-01 16:40 UTC (permalink / raw)
To: Santosh Shilimkar, netdev; +Cc: David Miller
Otherwise, if an IB connection is torn down before "rds_ib_setup_qp"
is called, the value of "ic->i_fastreg_wrs" is still at zero
(as it wasn't initialized by "rds_ib_setup_qp").
Consequently "rds_ib_conn_path_shutdown" will spin forever,
waiting for it to go back to "RDS_IB_DEFAULT_FR_WR",
which of course will never happen as there are no
outstanding work requests.
Signed-off-by: Gerd Rausch <gerd.rausch@oracle.com>
---
net/rds/ib_cm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/rds/ib_cm.c b/net/rds/ib_cm.c
index 1b6fd6c8b12b..4de0214da63c 100644
--- a/net/rds/ib_cm.c
+++ b/net/rds/ib_cm.c
@@ -527,7 +527,6 @@ static int rds_ib_setup_qp(struct rds_connection *conn)
attr.qp_type = IB_QPT_RC;
attr.send_cq = ic->i_send_cq;
attr.recv_cq = ic->i_recv_cq;
- atomic_set(&ic->i_fastreg_wrs, RDS_IB_DEFAULT_FR_WR);
/*
* XXX this can fail if max_*_wr is too large? Are we supposed
@@ -1139,6 +1138,7 @@ int rds_ib_conn_alloc(struct rds_connection *conn, gfp_t gfp)
spin_lock_init(&ic->i_ack_lock);
#endif
atomic_set(&ic->i_signaled_sends, 0);
+ atomic_set(&ic->i_fastreg_wrs, RDS_IB_DEFAULT_FR_WR);
/*
* rds_ib_conn_shutdown() waits for these to be emptied so they
--
2.18.0
^ permalink raw reply related
* [PATCH net-next 6/7] net/rds: Keep track of and wait for FRWR segments in use upon shutdown
From: Gerd Rausch @ 2019-07-01 16:40 UTC (permalink / raw)
To: Santosh Shilimkar, netdev; +Cc: David Miller
Since "rds_ib_free_frmr" and "rds_ib_free_frmr_list" simply put
the FRMR memory segments on the "drop_list" or "free_list",
and it is the job of "rds_ib_flush_mr_pool" to reap those entries
by ultimately issuing a "IB_WR_LOCAL_INV" work-request,
we need to trigger and then wait for all those memory segments
attached to a particular connection to be fully released before
we can move on to release the QP, CQ, etc.
So we make "rds_ib_conn_path_shutdown" wait for one more
atomic_t called "i_fastreg_inuse_count" that keeps track of how
many FRWR memory segments are out there marked "FRMR_IS_INUSE"
(and also wake_up rds_ib_ring_empty_wait, as they go away).
Signed-off-by: Gerd Rausch <gerd.rausch@oracle.com>
---
net/rds/ib.h | 1 +
net/rds/ib_cm.c | 7 +++++++
net/rds/ib_frmr.c | 45 ++++++++++++++++++++++++++++++++++++++-------
3 files changed, 46 insertions(+), 7 deletions(-)
diff --git a/net/rds/ib.h b/net/rds/ib.h
index 66c03c7665b2..303c6ee8bdb7 100644
--- a/net/rds/ib.h
+++ b/net/rds/ib.h
@@ -156,6 +156,7 @@ struct rds_ib_connection {
/* To control the number of wrs from fastreg */
atomic_t i_fastreg_wrs;
+ atomic_t i_fastreg_inuse_count;
/* interrupt handling */
struct tasklet_struct i_send_tasklet;
diff --git a/net/rds/ib_cm.c b/net/rds/ib_cm.c
index 8891822eba4f..1b6fd6c8b12b 100644
--- a/net/rds/ib_cm.c
+++ b/net/rds/ib_cm.c
@@ -40,6 +40,7 @@
#include "rds_single_path.h"
#include "rds.h"
#include "ib.h"
+#include "ib_mr.h"
/*
* Set the selected protocol version
@@ -993,6 +994,11 @@ void rds_ib_conn_path_shutdown(struct rds_conn_path *cp)
ic->i_cm_id, err);
}
+ /* kick off "flush_worker" for all pools in order to reap
+ * all FRMR registrations that are still marked "FRMR_IS_INUSE"
+ */
+ rds_ib_flush_mrs();
+
/*
* We want to wait for tx and rx completion to finish
* before we tear down the connection, but we have to be
@@ -1005,6 +1011,7 @@ void rds_ib_conn_path_shutdown(struct rds_conn_path *cp)
wait_event(rds_ib_ring_empty_wait,
rds_ib_ring_empty(&ic->i_recv_ring) &&
(atomic_read(&ic->i_signaled_sends) == 0) &&
+ (atomic_read(&ic->i_fastreg_inuse_count) == 0) &&
(atomic_read(&ic->i_fastreg_wrs) == RDS_IB_DEFAULT_FR_WR));
tasklet_kill(&ic->i_send_tasklet);
tasklet_kill(&ic->i_recv_tasklet);
diff --git a/net/rds/ib_frmr.c b/net/rds/ib_frmr.c
index a5d8f4128515..19c4cafb6952 100644
--- a/net/rds/ib_frmr.c
+++ b/net/rds/ib_frmr.c
@@ -32,6 +32,24 @@
#include "ib_mr.h"
+static inline void
+rds_transition_frwr_state(struct rds_ib_mr *ibmr,
+ enum rds_ib_fr_state old_state,
+ enum rds_ib_fr_state new_state)
+{
+ if (cmpxchg(&ibmr->u.frmr.fr_state,
+ old_state, new_state) == old_state &&
+ old_state == FRMR_IS_INUSE) {
+ /* enforce order of ibmr->u.frmr.fr_state update
+ * before decrementing i_fastreg_inuse_count
+ */
+ smp_mb__before_atomic();
+ atomic_dec(&ibmr->ic->i_fastreg_inuse_count);
+ if (waitqueue_active(&rds_ib_ring_empty_wait))
+ wake_up(&rds_ib_ring_empty_wait);
+ }
+}
+
static struct rds_ib_mr *rds_ib_alloc_frmr(struct rds_ib_device *rds_ibdev,
int npages)
{
@@ -118,13 +136,18 @@ static int rds_ib_post_reg_frmr(struct rds_ib_mr *ibmr)
if (unlikely(ret != ibmr->sg_len))
return ret < 0 ? ret : -EINVAL;
+ if (cmpxchg(&frmr->fr_state,
+ FRMR_IS_FREE, FRMR_IS_INUSE) != FRMR_IS_FREE)
+ return -EBUSY;
+
+ atomic_inc(&ibmr->ic->i_fastreg_inuse_count);
+
/* Perform a WR for the fast_reg_mr. Each individual page
* in the sg list is added to the fast reg page list and placed
* inside the fast_reg_mr WR. The key used is a rolling 8bit
* counter, which should guarantee uniqueness.
*/
ib_update_fast_reg_key(frmr->mr, ibmr->remap_count++);
- frmr->fr_state = FRMR_IS_INUSE;
frmr->fr_reg = true;
memset(®_wr, 0, sizeof(reg_wr));
@@ -141,7 +164,8 @@ static int rds_ib_post_reg_frmr(struct rds_ib_mr *ibmr)
ret = ib_post_send(ibmr->ic->i_cm_id->qp, ®_wr.wr, NULL);
if (unlikely(ret)) {
/* Failure here can be because of -ENOMEM as well */
- frmr->fr_state = FRMR_IS_STALE;
+ rds_transition_frwr_state(ibmr, FRMR_IS_INUSE, FRMR_IS_STALE);
+
atomic_inc(&ibmr->ic->i_fastreg_wrs);
if (printk_ratelimit())
pr_warn("RDS/IB: %s returned error(%d)\n",
@@ -163,7 +187,7 @@ static int rds_ib_post_reg_frmr(struct rds_ib_mr *ibmr)
if (frmr->fr_reg) {
pr_warn("RDS/IB: %s registration still incomplete after 100msec\n",
__func__);
- frmr->fr_state = FRMR_IS_STALE;
+ rds_transition_frwr_state(ibmr, FRMR_IS_INUSE, FRMR_IS_STALE);
ret = -EBUSY;
}
@@ -280,8 +304,12 @@ static int rds_ib_post_inv(struct rds_ib_mr *ibmr)
ret = ib_post_send(i_cm_id->qp, s_wr, NULL);
if (unlikely(ret)) {
- frmr->fr_state = FRMR_IS_STALE;
+ rds_transition_frwr_state(ibmr, FRMR_IS_INUSE, FRMR_IS_STALE);
frmr->fr_inv = false;
+ /* enforce order of frmr->fr_inv update
+ * before incrementing i_fastreg_wrs
+ */
+ smp_mb__before_atomic();
atomic_inc(&ibmr->ic->i_fastreg_wrs);
pr_err("RDS/IB: %s returned error(%d)\n", __func__, ret);
goto out;
@@ -316,7 +344,7 @@ void rds_ib_mr_cqe_handler(struct rds_ib_connection *ic, struct ib_wc *wc)
struct rds_ib_frmr *frmr = &ibmr->u.frmr;
if (wc->status != IB_WC_SUCCESS) {
- frmr->fr_state = FRMR_IS_STALE;
+ rds_transition_frwr_state(ibmr, FRMR_IS_INUSE, FRMR_IS_STALE);
if (rds_conn_up(ic->conn))
rds_ib_conn_error(ic->conn,
"frmr completion <%pI4,%pI4> status %u(%s), vendor_err 0x%x, disconnecting and reconnecting\n",
@@ -328,8 +356,7 @@ void rds_ib_mr_cqe_handler(struct rds_ib_connection *ic, struct ib_wc *wc)
}
if (frmr->fr_inv) {
- if (frmr->fr_state == FRMR_IS_INUSE)
- frmr->fr_state = FRMR_IS_FREE;
+ rds_transition_frwr_state(ibmr, FRMR_IS_INUSE, FRMR_IS_FREE);
frmr->fr_inv = false;
wake_up(&frmr->fr_inv_done);
}
@@ -339,6 +366,10 @@ void rds_ib_mr_cqe_handler(struct rds_ib_connection *ic, struct ib_wc *wc)
wake_up(&frmr->fr_reg_done);
}
+ /* enforce order of frmr->{fr_reg,fr_inv} update
+ * before incrementing i_fastreg_wrs
+ */
+ smp_mb__before_atomic();
atomic_inc(&ic->i_fastreg_wrs);
}
--
2.18.0
^ permalink raw reply related
* [PATCH net-next 5/7] net/rds: Set fr_state only to FRMR_IS_FREE if IB_WR_LOCAL_INV had been successful
From: Gerd Rausch @ 2019-07-01 16:40 UTC (permalink / raw)
To: Santosh Shilimkar, netdev; +Cc: David Miller
Fix a bug where fr_state first goes to FRMR_IS_STALE, because of a failure
of operation IB_WR_LOCAL_INV, but then gets set back to "FRMR_IS_FREE"
uncoditionally, even though the operation failed.
Signed-off-by: Gerd Rausch <gerd.rausch@oracle.com>
---
net/rds/ib_frmr.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/rds/ib_frmr.c b/net/rds/ib_frmr.c
index 3c953034dca3..a5d8f4128515 100644
--- a/net/rds/ib_frmr.c
+++ b/net/rds/ib_frmr.c
@@ -328,7 +328,8 @@ void rds_ib_mr_cqe_handler(struct rds_ib_connection *ic, struct ib_wc *wc)
}
if (frmr->fr_inv) {
- frmr->fr_state = FRMR_IS_FREE;
+ if (frmr->fr_state == FRMR_IS_INUSE)
+ frmr->fr_state = FRMR_IS_FREE;
frmr->fr_inv = false;
wake_up(&frmr->fr_inv_done);
}
--
2.18.0
^ permalink raw reply related
* [PATCH net-next 4/7] net/rds: Fix NULL/ERR_PTR inconsistency
From: Gerd Rausch @ 2019-07-01 16:39 UTC (permalink / raw)
To: Santosh Shilimkar, netdev; +Cc: David Miller
Make function "rds_ib_try_reuse_ibmr" return NULL in case
memory region could not be allocated, since callers
simply check if the return value is not NULL.
Signed-off-by: Gerd Rausch <gerd.rausch@oracle.com>
---
net/rds/ib_rdma.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/rds/ib_rdma.c b/net/rds/ib_rdma.c
index 6b047e63a769..c8c1e3ae8d84 100644
--- a/net/rds/ib_rdma.c
+++ b/net/rds/ib_rdma.c
@@ -450,7 +450,7 @@ struct rds_ib_mr *rds_ib_try_reuse_ibmr(struct rds_ib_mr_pool *pool)
rds_ib_stats_inc(s_ib_rdma_mr_8k_pool_depleted);
else
rds_ib_stats_inc(s_ib_rdma_mr_1m_pool_depleted);
- return ERR_PTR(-EAGAIN);
+ break;
}
/* We do have some empty MRs. Flush them out. */
@@ -464,7 +464,7 @@ struct rds_ib_mr *rds_ib_try_reuse_ibmr(struct rds_ib_mr_pool *pool)
return ibmr;
}
- return ibmr;
+ return NULL;
}
static void rds_ib_mr_pool_flush_worker(struct work_struct *work)
--
2.18.0
^ permalink raw reply related
* [PATCH net-next 3/7] net/rds: Wait for the FRMR_IS_FREE (or FRMR_IS_STALE) transition after posting IB_WR_LOCAL_INV
From: Gerd Rausch @ 2019-07-01 16:39 UTC (permalink / raw)
To: Santosh Shilimkar, netdev; +Cc: David Miller
In order to:
1) avoid a silly bouncing between "clean_list" and "drop_list"
triggered by function "rds_ib_reg_frmr" as it is releases frmr
regions whose state is not "FRMR_IS_FREE" right away.
2) prevent an invalid access error in a race from a pending
"IB_WR_LOCAL_INV" operation with a teardown ("dma_unmap_sg", "put_page")
and de-registration ("ib_dereg_mr") of the corresponding
memory region.
Signed-off-by: Gerd Rausch <gerd.rausch@oracle.com>
---
net/rds/ib_frmr.c | 89 ++++++++++++++++++++++++++++++-----------------
net/rds/ib_mr.h | 2 ++
2 files changed, 59 insertions(+), 32 deletions(-)
diff --git a/net/rds/ib_frmr.c b/net/rds/ib_frmr.c
index 9f8aa310c27a..3c953034dca3 100644
--- a/net/rds/ib_frmr.c
+++ b/net/rds/ib_frmr.c
@@ -76,6 +76,7 @@ static struct rds_ib_mr *rds_ib_alloc_frmr(struct rds_ib_device *rds_ibdev,
frmr->fr_state = FRMR_IS_FREE;
init_waitqueue_head(&frmr->fr_inv_done);
+ init_waitqueue_head(&frmr->fr_reg_done);
return ibmr;
out_no_cigar:
@@ -124,6 +125,7 @@ static int rds_ib_post_reg_frmr(struct rds_ib_mr *ibmr)
*/
ib_update_fast_reg_key(frmr->mr, ibmr->remap_count++);
frmr->fr_state = FRMR_IS_INUSE;
+ frmr->fr_reg = true;
memset(®_wr, 0, sizeof(reg_wr));
reg_wr.wr.wr_id = (unsigned long)(void *)ibmr;
@@ -144,7 +146,29 @@ static int rds_ib_post_reg_frmr(struct rds_ib_mr *ibmr)
if (printk_ratelimit())
pr_warn("RDS/IB: %s returned error(%d)\n",
__func__, ret);
+ goto out;
+ }
+
+ if (!frmr->fr_reg)
+ goto out;
+
+ /* Wait for the registration to complete in order to prevent an invalid
+ * access error resulting from a race between the memory region already
+ * being accessed while registration is still pending.
+ */
+ wait_event_timeout(frmr->fr_reg_done, !frmr->fr_reg,
+ msecs_to_jiffies(100));
+
+ /* Registration did not complete within one second, something's wrong */
+ if (frmr->fr_reg) {
+ pr_warn("RDS/IB: %s registration still incomplete after 100msec\n",
+ __func__);
+ frmr->fr_state = FRMR_IS_STALE;
+ ret = -EBUSY;
}
+
+out:
+
return ret;
}
@@ -262,6 +286,26 @@ static int rds_ib_post_inv(struct rds_ib_mr *ibmr)
pr_err("RDS/IB: %s returned error(%d)\n", __func__, ret);
goto out;
}
+
+ if (frmr->fr_state != FRMR_IS_INUSE)
+ goto out;
+
+ /* Wait for the FRMR_IS_FREE (or FRMR_IS_STALE) transition in order to
+ * 1) avoid a silly bouncing between "clean_list" and "drop_list"
+ * triggered by function "rds_ib_reg_frmr" as it is releases frmr
+ * regions whose state is not "FRMR_IS_FREE" right away.
+ * 2) prevents an invalid access error in a race
+ * from a pending "IB_WR_LOCAL_INV" operation
+ * with a teardown ("dma_unmap_sg", "put_page")
+ * and de-registration ("ib_dereg_mr") of the corresponding
+ * memory region.
+ */
+ wait_event_timeout(frmr->fr_inv_done, frmr->fr_state != FRMR_IS_INUSE,
+ msecs_to_jiffies(50));
+
+ if (frmr->fr_state == FRMR_IS_INUSE)
+ ret = -EBUSY;
+
out:
return ret;
}
@@ -289,6 +333,11 @@ void rds_ib_mr_cqe_handler(struct rds_ib_connection *ic, struct ib_wc *wc)
wake_up(&frmr->fr_inv_done);
}
+ if (frmr->fr_reg) {
+ frmr->fr_reg = false;
+ wake_up(&frmr->fr_reg_done);
+ }
+
atomic_inc(&ic->i_fastreg_wrs);
}
@@ -297,14 +346,18 @@ void rds_ib_unreg_frmr(struct list_head *list, unsigned int *nfreed,
{
struct rds_ib_mr *ibmr, *next;
struct rds_ib_frmr *frmr;
- int ret = 0;
+ int ret = 0, ret2;
unsigned int freed = *nfreed;
/* String all ib_mr's onto one list and hand them to ib_unmap_fmr */
list_for_each_entry(ibmr, list, unmap_list) {
- if (ibmr->sg_dma_len)
- ret |= rds_ib_post_inv(ibmr);
+ if (ibmr->sg_dma_len) {
+ ret2 = rds_ib_post_inv(ibmr);
+ if (ret2 && !ret)
+ ret = ret2;
+ }
}
+
if (ret)
pr_warn("RDS/IB: %s failed (err=%d)\n", __func__, ret);
@@ -347,36 +400,8 @@ struct rds_ib_mr *rds_ib_reg_frmr(struct rds_ib_device *rds_ibdev,
}
do {
- if (ibmr) {
- /* Memory regions make it onto the "clean_list" via
- * "rds_ib_flush_mr_pool", after the memory region has
- * been posted for invalidation via "rds_ib_post_inv".
- *
- * At that point in time, "fr_state" may still be
- * in state "FRMR_IS_INUSE", since the only place where
- * "fr_state" transitions to "FRMR_IS_FREE" is in
- * is in "rds_ib_mr_cqe_handler", which is
- * triggered by a tasklet.
- *
- * So in case we notice that
- * "fr_state != FRMR_IS_FREE" (see below), * we wait for
- * "fr_inv_done" to trigger with a maximum of 10msec.
- * Then we check again, and only put the memory region
- * onto the drop_list (via "rds_ib_free_frmr")
- * in case the situation remains unchanged.
- *
- * This avoids the problem of memory-regions bouncing
- * between "clean_list" and "drop_list" before they
- * even have a chance to be properly invalidated.
- */
- frmr = &ibmr->u.frmr;
- wait_event_timeout(frmr->fr_inv_done,
- frmr->fr_state == FRMR_IS_FREE,
- msecs_to_jiffies(10));
- if (frmr->fr_state == FRMR_IS_FREE)
- break;
+ if (ibmr)
rds_ib_free_frmr(ibmr, true);
- }
ibmr = rds_ib_alloc_frmr(rds_ibdev, nents);
if (IS_ERR(ibmr))
return ibmr;
diff --git a/net/rds/ib_mr.h b/net/rds/ib_mr.h
index ab26c20ed66f..9045a8c0edff 100644
--- a/net/rds/ib_mr.h
+++ b/net/rds/ib_mr.h
@@ -58,6 +58,8 @@ struct rds_ib_frmr {
enum rds_ib_fr_state fr_state;
bool fr_inv;
wait_queue_head_t fr_inv_done;
+ bool fr_reg;
+ wait_queue_head_t fr_reg_done;
struct ib_send_wr fr_wr;
unsigned int dma_npages;
unsigned int sg_byte_len;
--
2.18.0
^ permalink raw reply related
* [PATCH net-next 2/7] net/rds: Get rid of "wait_clean_list_grace" and add locking
From: Gerd Rausch @ 2019-07-01 16:39 UTC (permalink / raw)
To: Santosh Shilimkar, netdev; +Cc: David Miller
Waiting for activity on the "clean_list" to quiesce is no substitute
for proper locking.
We can have multiple threads competing for "llist_del_first"
via "rds_ib_reuse_mr", and a single thread competing
for "llist_del_all" and "llist_del_first" via "rds_ib_flush_mr_pool".
Since "llist_del_first" depends on "list->first->next" not to change
in the midst of the operation, simply waiting for all current calls
to "rds_ib_reuse_mr" to quiesce across all CPUs is woefully inadequate:
By the time "wait_clean_list_grace" is done iterating over all CPUs to see
that there is no concurrent caller to "rds_ib_reuse_mr", a new caller may
have just shown up on the first CPU.
Furthermore, <linux/llist.h> explicitly calls out the need for locking:
* Cases where locking is needed:
* If we have multiple consumers with llist_del_first used in one consumer,
* and llist_del_first or llist_del_all used in other consumers,
* then a lock is needed.
Also, while at it, drop the unused "pool" parameter
from "list_to_llist_nodes".
Signed-off-by: Gerd Rausch <gerd.rausch@oracle.com>
---
net/rds/ib_mr.h | 1 +
net/rds/ib_rdma.c | 56 +++++++++++++++--------------------------------
2 files changed, 19 insertions(+), 38 deletions(-)
diff --git a/net/rds/ib_mr.h b/net/rds/ib_mr.h
index 42daccb7b5eb..ab26c20ed66f 100644
--- a/net/rds/ib_mr.h
+++ b/net/rds/ib_mr.h
@@ -98,6 +98,7 @@ struct rds_ib_mr_pool {
struct llist_head free_list; /* unused MRs */
struct llist_head clean_list; /* unused & unmapped MRs */
wait_queue_head_t flush_wait;
+ spinlock_t clean_lock; /* "clean_list" concurrency */
atomic_t free_pinned; /* memory pinned by free MRs */
unsigned long max_items;
diff --git a/net/rds/ib_rdma.c b/net/rds/ib_rdma.c
index 0b347f46b2f4..6b047e63a769 100644
--- a/net/rds/ib_rdma.c
+++ b/net/rds/ib_rdma.c
@@ -40,9 +40,6 @@
struct workqueue_struct *rds_ib_mr_wq;
-static DEFINE_PER_CPU(unsigned long, clean_list_grace);
-#define CLEAN_LIST_BUSY_BIT 0
-
static struct rds_ib_device *rds_ib_get_device(__be32 ipaddr)
{
struct rds_ib_device *rds_ibdev;
@@ -195,12 +192,11 @@ struct rds_ib_mr *rds_ib_reuse_mr(struct rds_ib_mr_pool *pool)
{
struct rds_ib_mr *ibmr = NULL;
struct llist_node *ret;
- unsigned long *flag;
+ unsigned long flags;
- preempt_disable();
- flag = this_cpu_ptr(&clean_list_grace);
- set_bit(CLEAN_LIST_BUSY_BIT, flag);
+ spin_lock_irqsave(&pool->clean_lock, flags);
ret = llist_del_first(&pool->clean_list);
+ spin_unlock_irqrestore(&pool->clean_lock, flags);
if (ret) {
ibmr = llist_entry(ret, struct rds_ib_mr, llnode);
if (pool->pool_type == RDS_IB_MR_8K_POOL)
@@ -209,23 +205,9 @@ struct rds_ib_mr *rds_ib_reuse_mr(struct rds_ib_mr_pool *pool)
rds_ib_stats_inc(s_ib_rdma_mr_1m_reused);
}
- clear_bit(CLEAN_LIST_BUSY_BIT, flag);
- preempt_enable();
return ibmr;
}
-static inline void wait_clean_list_grace(void)
-{
- int cpu;
- unsigned long *flag;
-
- for_each_online_cpu(cpu) {
- flag = &per_cpu(clean_list_grace, cpu);
- while (test_bit(CLEAN_LIST_BUSY_BIT, flag))
- cpu_relax();
- }
-}
-
void rds_ib_sync_mr(void *trans_private, int direction)
{
struct rds_ib_mr *ibmr = trans_private;
@@ -324,8 +306,7 @@ static unsigned int llist_append_to_list(struct llist_head *llist,
* of clusters. Each cluster has linked llist nodes of
* MR_CLUSTER_SIZE mrs that are ready for reuse.
*/
-static void list_to_llist_nodes(struct rds_ib_mr_pool *pool,
- struct list_head *list,
+static void list_to_llist_nodes(struct list_head *list,
struct llist_node **nodes_head,
struct llist_node **nodes_tail)
{
@@ -402,8 +383,13 @@ int rds_ib_flush_mr_pool(struct rds_ib_mr_pool *pool,
*/
dirty_to_clean = llist_append_to_list(&pool->drop_list, &unmap_list);
dirty_to_clean += llist_append_to_list(&pool->free_list, &unmap_list);
- if (free_all)
+ if (free_all) {
+ unsigned long flags;
+
+ spin_lock_irqsave(&pool->clean_lock, flags);
llist_append_to_list(&pool->clean_list, &unmap_list);
+ spin_unlock_irqrestore(&pool->clean_lock, flags);
+ }
free_goal = rds_ib_flush_goal(pool, free_all);
@@ -416,27 +402,20 @@ int rds_ib_flush_mr_pool(struct rds_ib_mr_pool *pool,
rds_ib_unreg_fmr(&unmap_list, &nfreed, &unpinned, free_goal);
if (!list_empty(&unmap_list)) {
- /* we have to make sure that none of the things we're about
- * to put on the clean list would race with other cpus trying
- * to pull items off. The llist would explode if we managed to
- * remove something from the clean list and then add it back again
- * while another CPU was spinning on that same item in llist_del_first.
- *
- * This is pretty unlikely, but just in case wait for an llist grace period
- * here before adding anything back into the clean list.
- */
- wait_clean_list_grace();
-
- list_to_llist_nodes(pool, &unmap_list, &clean_nodes, &clean_tail);
+ unsigned long flags;
+
+ list_to_llist_nodes(&unmap_list, &clean_nodes, &clean_tail);
if (ibmr_ret) {
*ibmr_ret = llist_entry(clean_nodes, struct rds_ib_mr, llnode);
clean_nodes = clean_nodes->next;
}
/* more than one entry in llist nodes */
- if (clean_nodes)
+ if (clean_nodes) {
+ spin_lock_irqsave(&pool->clean_lock, flags);
llist_add_batch(clean_nodes, clean_tail,
&pool->clean_list);
-
+ spin_unlock_irqrestore(&pool->clean_lock, flags);
+ }
}
atomic_sub(unpinned, &pool->free_pinned);
@@ -610,6 +589,7 @@ struct rds_ib_mr_pool *rds_ib_create_mr_pool(struct rds_ib_device *rds_ibdev,
init_llist_head(&pool->free_list);
init_llist_head(&pool->drop_list);
init_llist_head(&pool->clean_list);
+ spin_lock_init(&pool->clean_lock);
mutex_init(&pool->flush_lock);
init_waitqueue_head(&pool->flush_wait);
INIT_DELAYED_WORK(&pool->flush_worker, rds_ib_mr_pool_flush_worker);
--
2.18.0
^ permalink raw reply related
* [PATCH net-next 0/7] net/rds: RDMA fixes
From: Gerd Rausch @ 2019-07-01 16:39 UTC (permalink / raw)
To: Santosh Shilimkar, netdev; +Cc: David Miller
A number of net/rds fixes necessary to make "rds_rdma.ko"
pass some basic Oracle internal tests.
Gerd Rausch (7):
net/rds: Give fr_state a chance to transition to FRMR_IS_FREE
net/rds: Get rid of "wait_clean_list_grace" and add locking
net/rds: Wait for the FRMR_IS_FREE (or FRMR_IS_STALE) transition after
posting IB_WR_LOCAL_INV
net/rds: Fix NULL/ERR_PTR inconsistency
net/rds: Set fr_state only to FRMR_IS_FREE if IB_WR_LOCAL_INV had been
successful
net/rds: Keep track of and wait for FRWR segments in use upon shutdown
net/rds: Initialize ic->i_fastreg_wrs upon allocation
net/rds/ib.h | 1 +
net/rds/ib_cm.c | 9 +++-
net/rds/ib_frmr.c | 103 ++++++++++++++++++++++++++++++++++++++++++----
net/rds/ib_mr.h | 4 ++
net/rds/ib_rdma.c | 60 +++++++++------------------
5 files changed, 128 insertions(+), 49 deletions(-)
--
2.18.0
^ permalink raw reply
* [PATCH net-next 1/7] net/rds: Give fr_state a chance to transition to FRMR_IS_FREE
From: Gerd Rausch @ 2019-07-01 16:39 UTC (permalink / raw)
To: Santosh Shilimkar, netdev; +Cc: David Miller
In the context of FRMR (ib_frmr.c):
Memory regions make it onto the "clean_list" via "rds_ib_flush_mr_pool",
after the memory region has been posted for invalidation via
"rds_ib_post_inv".
At that point in time, "fr_state" may still be in state "FRMR_IS_INUSE",
since the only place where "fr_state" transitions to "FRMR_IS_FREE"
is in "rds_ib_mr_cqe_handler", which is triggered by a tasklet.
So in case we notice that "fr_state != FRMR_IS_FREE" (see below),
we wait for "fr_inv_done" to trigger with a maximum of 10msec.
Then we check again, and only put the memory region onto the drop_list
(via "rds_ib_free_frmr") in case the situation remains unchanged.
This avoids the problem of memory-regions bouncing between "clean_list"
and "drop_list" before they even have a chance to be properly invalidated.
Signed-off-by: Gerd Rausch <gerd.rausch@oracle.com>
---
net/rds/ib_frmr.c | 32 +++++++++++++++++++++++++++++++-
net/rds/ib_mr.h | 1 +
2 files changed, 32 insertions(+), 1 deletion(-)
diff --git a/net/rds/ib_frmr.c b/net/rds/ib_frmr.c
index 32ae26ed58a0..9f8aa310c27a 100644
--- a/net/rds/ib_frmr.c
+++ b/net/rds/ib_frmr.c
@@ -75,6 +75,7 @@ static struct rds_ib_mr *rds_ib_alloc_frmr(struct rds_ib_device *rds_ibdev,
pool->max_items_soft = pool->max_items;
frmr->fr_state = FRMR_IS_FREE;
+ init_waitqueue_head(&frmr->fr_inv_done);
return ibmr;
out_no_cigar:
@@ -285,6 +286,7 @@ void rds_ib_mr_cqe_handler(struct rds_ib_connection *ic, struct ib_wc *wc)
if (frmr->fr_inv) {
frmr->fr_state = FRMR_IS_FREE;
frmr->fr_inv = false;
+ wake_up(&frmr->fr_inv_done);
}
atomic_inc(&ic->i_fastreg_wrs);
@@ -345,8 +347,36 @@ struct rds_ib_mr *rds_ib_reg_frmr(struct rds_ib_device *rds_ibdev,
}
do {
- if (ibmr)
+ if (ibmr) {
+ /* Memory regions make it onto the "clean_list" via
+ * "rds_ib_flush_mr_pool", after the memory region has
+ * been posted for invalidation via "rds_ib_post_inv".
+ *
+ * At that point in time, "fr_state" may still be
+ * in state "FRMR_IS_INUSE", since the only place where
+ * "fr_state" transitions to "FRMR_IS_FREE" is in
+ * is in "rds_ib_mr_cqe_handler", which is
+ * triggered by a tasklet.
+ *
+ * So in case we notice that
+ * "fr_state != FRMR_IS_FREE" (see below), * we wait for
+ * "fr_inv_done" to trigger with a maximum of 10msec.
+ * Then we check again, and only put the memory region
+ * onto the drop_list (via "rds_ib_free_frmr")
+ * in case the situation remains unchanged.
+ *
+ * This avoids the problem of memory-regions bouncing
+ * between "clean_list" and "drop_list" before they
+ * even have a chance to be properly invalidated.
+ */
+ frmr = &ibmr->u.frmr;
+ wait_event_timeout(frmr->fr_inv_done,
+ frmr->fr_state == FRMR_IS_FREE,
+ msecs_to_jiffies(10));
+ if (frmr->fr_state == FRMR_IS_FREE)
+ break;
rds_ib_free_frmr(ibmr, true);
+ }
ibmr = rds_ib_alloc_frmr(rds_ibdev, nents);
if (IS_ERR(ibmr))
return ibmr;
diff --git a/net/rds/ib_mr.h b/net/rds/ib_mr.h
index 5da12c248431..42daccb7b5eb 100644
--- a/net/rds/ib_mr.h
+++ b/net/rds/ib_mr.h
@@ -57,6 +57,7 @@ struct rds_ib_frmr {
struct ib_mr *mr;
enum rds_ib_fr_state fr_state;
bool fr_inv;
+ wait_queue_head_t fr_inv_done;
struct ib_send_wr fr_wr;
unsigned int dma_npages;
unsigned int sg_byte_len;
--
2.18.0
^ permalink raw reply related
* [PATCH] User mode linux bump maximum MTU tuntap interface
From: Алексей @ 2019-07-01 16:29 UTC (permalink / raw)
To: netdev
Hello, the parameter ETH_MAX_PACKET limited to 1500 bytes is the not
support jumbo frames.
This patch change ETH_MAX_PACKET the 65535 bytes to jumbo frame support
with user mode linux tuntap driver.
PATCH:
-------------------
diff -ruNp ./src/1/linux-5.1/arch/um/include/shared/net_user.h
./src/linux-5.1/$
--- ./arch/um/include/shared/net_user.h 2019-05-06 00:42:58.000000000 +0000
+++ ./arch/um/include/shared/net_user.h 2019-07-01 16:09:20.316666597 +0000
@@ -9,7 +9,7 @@
#define ETH_ADDR_LEN (6)
#define ETH_HEADER_ETHERTAP (16)
#define ETH_HEADER_OTHER (26) /* 14 for ethernet + VLAN + MPLS for
crazy peopl$
-#define ETH_MAX_PACKET (1500)
+#define ETH_MAX_PACKET (65535)
#define UML_NET_VERSION (4)
^ permalink raw reply
* [PATCH bpf-next v2 3/3] selftests/bpf: add verifier tests for wide stores
From: Stanislav Fomichev @ 2019-07-01 16:31 UTC (permalink / raw)
To: netdev, bpf
Cc: davem, ast, daniel, Stanislav Fomichev, Andrii Nakryiko,
Yonghong Song
In-Reply-To: <20190701163103.237550-1-sdf@google.com>
Make sure that wide stores are allowed at proper (aligned) addresses.
Note that user_ip6 is naturally aligned on 8-byte boundary, so
correct addresses are user_ip6[0] and user_ip6[2]. msg_src_ip6 is,
however, aligned on a 4-byte bondary, so only msg_src_ip6[1]
can be wide-stored.
Cc: Andrii Nakryiko <andriin@fb.com>
Cc: Yonghong Song <yhs@fb.com>
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
tools/testing/selftests/bpf/test_verifier.c | 17 +++++++--
.../selftests/bpf/verifier/wide_store.c | 36 +++++++++++++++++++
2 files changed, 50 insertions(+), 3 deletions(-)
create mode 100644 tools/testing/selftests/bpf/verifier/wide_store.c
diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index c5514daf8865..b0773291012a 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -105,6 +105,7 @@ struct bpf_test {
__u64 data64[TEST_DATA_LEN / 8];
};
} retvals[MAX_TEST_RUNS];
+ enum bpf_attach_type expected_attach_type;
};
/* Note we want this to be 64 bit aligned so that the end of our array is
@@ -850,6 +851,7 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
int fd_prog, expected_ret, alignment_prevented_execution;
int prog_len, prog_type = test->prog_type;
struct bpf_insn *prog = test->insns;
+ struct bpf_load_program_attr attr;
int run_errs, run_successes;
int map_fds[MAX_NR_MAPS];
const char *expected_err;
@@ -881,8 +883,17 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
pflags |= BPF_F_STRICT_ALIGNMENT;
if (test->flags & F_NEEDS_EFFICIENT_UNALIGNED_ACCESS)
pflags |= BPF_F_ANY_ALIGNMENT;
- fd_prog = bpf_verify_program(prog_type, prog, prog_len, pflags,
- "GPL", 0, bpf_vlog, sizeof(bpf_vlog), 4);
+
+ memset(&attr, 0, sizeof(attr));
+ attr.prog_type = prog_type;
+ attr.expected_attach_type = test->expected_attach_type;
+ attr.insns = prog;
+ attr.insns_cnt = prog_len;
+ attr.license = "GPL";
+ attr.log_level = 4;
+ attr.prog_flags = pflags;
+
+ fd_prog = bpf_load_program_xattr(&attr, bpf_vlog, sizeof(bpf_vlog));
if (fd_prog < 0 && !bpf_probe_prog_type(prog_type, 0)) {
printf("SKIP (unsupported program type %d)\n", prog_type);
skips++;
@@ -912,7 +923,7 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
printf("FAIL\nUnexpected success to load!\n");
goto fail_log;
}
- if (!strstr(bpf_vlog, expected_err)) {
+ if (!expected_err || !strstr(bpf_vlog, expected_err)) {
printf("FAIL\nUnexpected error message!\n\tEXP: %s\n\tRES: %s\n",
expected_err, bpf_vlog);
goto fail_log;
diff --git a/tools/testing/selftests/bpf/verifier/wide_store.c b/tools/testing/selftests/bpf/verifier/wide_store.c
new file mode 100644
index 000000000000..8fe99602ded4
--- /dev/null
+++ b/tools/testing/selftests/bpf/verifier/wide_store.c
@@ -0,0 +1,36 @@
+#define BPF_SOCK_ADDR(field, off, res, err) \
+{ \
+ "wide store to bpf_sock_addr." #field "[" #off "]", \
+ .insns = { \
+ BPF_MOV64_IMM(BPF_REG_0, 1), \
+ BPF_STX_MEM(BPF_DW, BPF_REG_1, BPF_REG_0, \
+ offsetof(struct bpf_sock_addr, field[off])), \
+ BPF_EXIT_INSN(), \
+ }, \
+ .result = res, \
+ .prog_type = BPF_PROG_TYPE_CGROUP_SOCK_ADDR, \
+ .expected_attach_type = BPF_CGROUP_UDP6_SENDMSG, \
+ .errstr = err, \
+}
+
+/* user_ip6[0] is u64 aligned */
+BPF_SOCK_ADDR(user_ip6, 0, ACCEPT,
+ NULL),
+BPF_SOCK_ADDR(user_ip6, 1, REJECT,
+ "invalid bpf_context access off=12 size=8"),
+BPF_SOCK_ADDR(user_ip6, 2, ACCEPT,
+ NULL),
+BPF_SOCK_ADDR(user_ip6, 3, REJECT,
+ "invalid bpf_context access off=20 size=8"),
+
+/* msg_src_ip6[0] is _not_ u64 aligned */
+BPF_SOCK_ADDR(msg_src_ip6, 0, REJECT,
+ "invalid bpf_context access off=44 size=8"),
+BPF_SOCK_ADDR(msg_src_ip6, 1, ACCEPT,
+ NULL),
+BPF_SOCK_ADDR(msg_src_ip6, 2, REJECT,
+ "invalid bpf_context access off=52 size=8"),
+BPF_SOCK_ADDR(msg_src_ip6, 3, REJECT,
+ "invalid bpf_context access off=56 size=8"),
+
+#undef BPF_SOCK_ADDR
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCH bpf-next v2 2/3] bpf: sync bpf.h to tools/
From: Stanislav Fomichev @ 2019-07-01 16:31 UTC (permalink / raw)
To: netdev, bpf; +Cc: davem, ast, daniel, Stanislav Fomichev
In-Reply-To: <20190701163103.237550-1-sdf@google.com>
Sync user_ip6 & msg_src_ip6 comments.
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
tools/include/uapi/linux/bpf.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index a396b516a2b2..586867fe6102 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -3237,7 +3237,7 @@ struct bpf_sock_addr {
__u32 user_ip4; /* Allows 1,2,4-byte read and 4-byte write.
* Stored in network byte order.
*/
- __u32 user_ip6[4]; /* Allows 1,2,4-byte read an 4-byte write.
+ __u32 user_ip6[4]; /* Allows 1,2,4-byte read an 4,8-byte write.
* Stored in network byte order.
*/
__u32 user_port; /* Allows 4-byte read and write.
@@ -3249,7 +3249,7 @@ struct bpf_sock_addr {
__u32 msg_src_ip4; /* Allows 1,2,4-byte read an 4-byte write.
* Stored in network byte order.
*/
- __u32 msg_src_ip6[4]; /* Allows 1,2,4-byte read an 4-byte write.
+ __u32 msg_src_ip6[4]; /* Allows 1,2,4-byte read an 4,8-byte write.
* Stored in network byte order.
*/
__bpf_md_ptr(struct bpf_sock *, sk);
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCH bpf-next v2 1/3] bpf: allow wide (u64) aligned stores for some fields of bpf_sock_addr
From: Stanislav Fomichev @ 2019-07-01 16:31 UTC (permalink / raw)
To: netdev, bpf
Cc: davem, ast, daniel, Stanislav Fomichev, Andrii Nakryiko,
Yonghong Song, kernel test robot
In-Reply-To: <20190701163103.237550-1-sdf@google.com>
Since commit cd17d7770578 ("bpf/tools: sync bpf.h") clang decided
that it can do a single u64 store into user_ip6[2] instead of two
separate u32 ones:
# 17: (18) r2 = 0x100000000000000
# ; ctx->user_ip6[2] = bpf_htonl(DST_REWRITE_IP6_2);
# 19: (7b) *(u64 *)(r1 +16) = r2
# invalid bpf_context access off=16 size=8
From the compiler point of view it does look like a correct thing
to do, so let's support it on the kernel side.
Credit to Andrii Nakryiko for a proper implementation of
bpf_ctx_wide_store_ok.
Cc: Andrii Nakryiko <andriin@fb.com>
Cc: Yonghong Song <yhs@fb.com>
Fixes: cd17d7770578 ("bpf/tools: sync bpf.h")
Reported-by: kernel test robot <rong.a.chen@intel.com>
Acked-by: Yonghong Song <yhs@fb.com>
Acked-by: Andrii Nakryiko <andriin@fb.com>
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
include/linux/filter.h | 6 ++++++
include/uapi/linux/bpf.h | 4 ++--
net/core/filter.c | 22 ++++++++++++++--------
3 files changed, 22 insertions(+), 10 deletions(-)
diff --git a/include/linux/filter.h b/include/linux/filter.h
index 340f7d648974..3901007e36f1 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -746,6 +746,12 @@ bpf_ctx_narrow_access_ok(u32 off, u32 size, u32 size_default)
return size <= size_default && (size & (size - 1)) == 0;
}
+#define bpf_ctx_wide_store_ok(off, size, type, field) \
+ (size == sizeof(__u64) && \
+ off >= offsetof(type, field) && \
+ off + sizeof(__u64) <= offsetofend(type, field) && \
+ off % sizeof(__u64) == 0)
+
#define bpf_classic_proglen(fprog) (fprog->len * sizeof(fprog->filter[0]))
static inline void bpf_prog_lock_ro(struct bpf_prog *fp)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index a396b516a2b2..586867fe6102 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -3237,7 +3237,7 @@ struct bpf_sock_addr {
__u32 user_ip4; /* Allows 1,2,4-byte read and 4-byte write.
* Stored in network byte order.
*/
- __u32 user_ip6[4]; /* Allows 1,2,4-byte read an 4-byte write.
+ __u32 user_ip6[4]; /* Allows 1,2,4-byte read an 4,8-byte write.
* Stored in network byte order.
*/
__u32 user_port; /* Allows 4-byte read and write.
@@ -3249,7 +3249,7 @@ struct bpf_sock_addr {
__u32 msg_src_ip4; /* Allows 1,2,4-byte read an 4-byte write.
* Stored in network byte order.
*/
- __u32 msg_src_ip6[4]; /* Allows 1,2,4-byte read an 4-byte write.
+ __u32 msg_src_ip6[4]; /* Allows 1,2,4-byte read an 4,8-byte write.
* Stored in network byte order.
*/
__bpf_md_ptr(struct bpf_sock *, sk);
diff --git a/net/core/filter.c b/net/core/filter.c
index dc8534be12fc..5d33f2146dab 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -6849,6 +6849,16 @@ static bool sock_addr_is_valid_access(int off, int size,
if (!bpf_ctx_narrow_access_ok(off, size, size_default))
return false;
} else {
+ if (bpf_ctx_wide_store_ok(off, size,
+ struct bpf_sock_addr,
+ user_ip6))
+ return true;
+
+ if (bpf_ctx_wide_store_ok(off, size,
+ struct bpf_sock_addr,
+ msg_src_ip6))
+ return true;
+
if (size != size_default)
return false;
}
@@ -7689,9 +7699,6 @@ static u32 xdp_convert_ctx_access(enum bpf_access_type type,
/* SOCK_ADDR_STORE_NESTED_FIELD_OFF() has semantic similar to
* SOCK_ADDR_LOAD_NESTED_FIELD_SIZE_OFF() but for store operation.
*
- * It doesn't support SIZE argument though since narrow stores are not
- * supported for now.
- *
* In addition it uses Temporary Field TF (member of struct S) as the 3rd
* "register" since two registers available in convert_ctx_access are not
* enough: we can't override neither SRC, since it contains value to store, nor
@@ -7699,7 +7706,7 @@ static u32 xdp_convert_ctx_access(enum bpf_access_type type,
* instructions. But we need a temporary place to save pointer to nested
* structure whose field we want to store to.
*/
-#define SOCK_ADDR_STORE_NESTED_FIELD_OFF(S, NS, F, NF, OFF, TF) \
+#define SOCK_ADDR_STORE_NESTED_FIELD_OFF(S, NS, F, NF, SIZE, OFF, TF) \
do { \
int tmp_reg = BPF_REG_9; \
if (si->src_reg == tmp_reg || si->dst_reg == tmp_reg) \
@@ -7710,8 +7717,7 @@ static u32 xdp_convert_ctx_access(enum bpf_access_type type,
offsetof(S, TF)); \
*insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(S, F), tmp_reg, \
si->dst_reg, offsetof(S, F)); \
- *insn++ = BPF_STX_MEM( \
- BPF_FIELD_SIZEOF(NS, NF), tmp_reg, si->src_reg, \
+ *insn++ = BPF_STX_MEM(SIZE, tmp_reg, si->src_reg, \
bpf_target_off(NS, NF, FIELD_SIZEOF(NS, NF), \
target_size) \
+ OFF); \
@@ -7723,8 +7729,8 @@ static u32 xdp_convert_ctx_access(enum bpf_access_type type,
TF) \
do { \
if (type == BPF_WRITE) { \
- SOCK_ADDR_STORE_NESTED_FIELD_OFF(S, NS, F, NF, OFF, \
- TF); \
+ SOCK_ADDR_STORE_NESTED_FIELD_OFF(S, NS, F, NF, SIZE, \
+ OFF, TF); \
} else { \
SOCK_ADDR_LOAD_NESTED_FIELD_SIZE_OFF( \
S, NS, F, NF, SIZE, OFF); \
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCH bpf-next v2 0/3] bpf: allow wide (u64) aligned stores for some fields of bpf_sock_addr
From: Stanislav Fomichev @ 2019-07-01 16:31 UTC (permalink / raw)
To: netdev, bpf; +Cc: davem, ast, daniel, Stanislav Fomichev
Clang can generate 8-byte stores for user_ip6 & msg_src_ip6,
let's support that on the verifier side.
v2:
* Add simple cover letter (Yonghong Song)
* Update comments (Yonghong Song)
* Remove [4] selftests (Yonghong Song)
Stanislav Fomichev (3):
bpf: allow wide (u64) aligned stores for some fields of bpf_sock_addr
bpf: sync bpf.h to tools/
selftests/bpf: add verifier tests for wide stores
include/linux/filter.h | 6 ++++
include/uapi/linux/bpf.h | 4 +--
net/core/filter.c | 22 +++++++-----
tools/include/uapi/linux/bpf.h | 4 +--
tools/testing/selftests/bpf/test_verifier.c | 17 +++++++--
.../selftests/bpf/verifier/wide_store.c | 36 +++++++++++++++++++
6 files changed, 74 insertions(+), 15 deletions(-)
create mode 100644 tools/testing/selftests/bpf/verifier/wide_store.c
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply
* [PATCH][next] iwlwifi: mvm: fix comparison of u32 variable with less than zero
From: Colin King @ 2019-07-01 16:26 UTC (permalink / raw)
To: Haim Dreyfuss, Johannes Berg, Emmanuel Grumbach, Luca Coelho,
Intel Linux Wireless, Kalle Valo, David S . Miller,
linux-wireless, netdev
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
The comparison of the u32 variable wgds_tbl_idx with less than zero is
always going to be false because it is unsigned. Fix this by making
wgds_tbl_idx a plain signed int.
Addresses-Coverity: ("Unsigned compared against 0")
Fixes: 4fd445a2c855 ("iwlwifi: mvm: Add log information about SAR status")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/net/wireless/intel/iwlwifi/mvm/nvm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/nvm.c b/drivers/net/wireless/intel/iwlwifi/mvm/nvm.c
index 719f793b3487..a9bb43a2f27b 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/nvm.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/nvm.c
@@ -620,7 +620,7 @@ void iwl_mvm_rx_chub_update_mcc(struct iwl_mvm *mvm,
enum iwl_mcc_source src;
char mcc[3];
struct ieee80211_regdomain *regd;
- u32 wgds_tbl_idx;
+ int wgds_tbl_idx;
lockdep_assert_held(&mvm->mutex);
--
2.20.1
^ permalink raw reply related
* Re: [PATCH net] Documentation/networking: fix default_ttl typo in mpls-sysctl
From: David Ahern @ 2019-07-01 16:21 UTC (permalink / raw)
To: Hangbin Liu, netdev; +Cc: David Miller
In-Reply-To: <20190701084528.25872-1-liuhangbin@gmail.com>
On 7/1/19 2:45 AM, Hangbin Liu wrote:
> default_ttl should be integer instead of bool
>
> Reported-by: Ying Xu <yinxu@redhat.com>
> Fixes: a59166e47086 ("mpls: allow TTL propagation from IP packets to be configured")
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
> Documentation/networking/mpls-sysctl.txt | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
Reviewed-by: David Ahern <dsahern@gmail.com>
^ permalink raw reply
* Re: [PATCH v5 net-next 6/6] net: ethernet: ti: cpsw: add XDP support
From: Jesper Dangaard Brouer @ 2019-07-01 16:19 UTC (permalink / raw)
To: Ivan Khoronzhuk
Cc: grygorii.strashko, davem, ast, linux-kernel, linux-omap,
ilias.apalodimas, netdev, daniel, jakub.kicinski, john.fastabend,
brouer
In-Reply-To: <20190630172348.5692-7-ivan.khoronzhuk@linaro.org>
On Sun, 30 Jun 2019 20:23:48 +0300
Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
> +static int cpsw_ndev_create_xdp_rxq(struct cpsw_priv *priv, int ch)
> +{
> + struct cpsw_common *cpsw = priv->cpsw;
> + int ret, new_pool = false;
> + struct xdp_rxq_info *rxq;
> +
> + rxq = &priv->xdp_rxq[ch];
> +
> + ret = xdp_rxq_info_reg(rxq, priv->ndev, ch);
> + if (ret)
> + return ret;
> +
> + if (!cpsw->page_pool[ch]) {
> + ret = cpsw_create_rx_pool(cpsw, ch);
> + if (ret)
> + goto err_rxq;
> +
> + new_pool = true;
> + }
> +
> + ret = xdp_rxq_info_reg_mem_model(rxq, MEM_TYPE_PAGE_POOL,
> + cpsw->page_pool[ch]);
> + if (!ret)
> + return 0;
> +
> + if (new_pool) {
> + page_pool_free(cpsw->page_pool[ch]);
> + cpsw->page_pool[ch] = NULL;
> + }
> +
> +err_rxq:
> + xdp_rxq_info_unreg(rxq);
> + return ret;
> +}
Looking at this, and Ilias'es XDP-netsec error handling path, it might
be a mistake that I removed page_pool_destroy() and instead put the
responsibility on xdp_rxq_info_unreg().
As here, we have to detect if page_pool_create() was a success, and then
if xdp_rxq_info_reg_mem_model() was a failure, explicitly call
page_pool_free() because the xdp_rxq_info_unreg() call cannot "free"
the page_pool object given it was not registered.
Ivan's patch in[1], might be a better approach, which forced all
drivers to explicitly call page_pool_free(), even-though it just
dec-refcnt and the real call to page_pool_free() happened via
xdp_rxq_info_unreg().
To better handle error path, I would re-introduce page_pool_destroy(),
as a driver API, that would gracefully handle NULL-pointer case, and
then call page_pool_free() with the atomic_dec_and_test(). (It should
hopefully simplify the error handling code a bit)
[1] https://lore.kernel.org/netdev/20190625175948.24771-2-ivan.khoronzhuk@linaro.org/
> +void cpsw_ndev_destroy_xdp_rxqs(struct cpsw_priv *priv)
> +{
> + struct cpsw_common *cpsw = priv->cpsw;
> + struct xdp_rxq_info *rxq;
> + int i;
> +
> + for (i = 0; i < cpsw->rx_ch_num; i++) {
> + rxq = &priv->xdp_rxq[i];
> + if (xdp_rxq_info_is_reg(rxq))
> + xdp_rxq_info_unreg(rxq);
> + }
> +}
Are you sure you need to test xdp_rxq_info_is_reg() here?
You should just call xdp_rxq_info_unreg(rxq), if you know that this rxq
should be registered. If your assumption failed, you will get a
WARNing, and discover your driver level bug. This is one of the ways
the API is designed to "detect" misuse of the API. (I found this
rather useful, when I converted the approx 12 drivers using this
xdp_rxq_info API).
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* Re: [PATCH net] ipv4: don't set IPv6 only flags to IPv4 addresses
From: Matteo Croce @ 2019-07-01 16:13 UTC (permalink / raw)
To: Joe Perches
Cc: netdev, LKML, David S. Miller, Alexey Kuznetsov,
Hideaki YOSHIFUJI
In-Reply-To: <42624f83da71354a5daef959a4749cb75516d37f.camel@perches.com>
On Mon, Jul 1, 2019 at 6:10 PM Joe Perches <joe@perches.com> wrote:
>
> On Mon, 2019-07-01 at 18:08 +0200, Matteo Croce wrote:
> > Avoid the situation where an IPV6 only flag is applied to an IPv4 address:
> []
> > diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
> []
> > @@ -468,6 +473,9 @@ static int __inet_insert_ifa(struct in_ifaddr *ifa, struct nlmsghdr *nlh,
> > ifa->ifa_flags &= ~IFA_F_SECONDARY;
> > last_primary = &in_dev->ifa_list;
> >
> > + /* Don't set IPv6 only flags to IPv6 addresses */
>
> umm, IPv4 addresses?
>
>
Ouch, right.
/* Don't set IPv6 only flags to IPv4 addresses */
Can this be edidet on patchwork instead of spamming with a v2?
--
Matteo Croce
per aspera ad upstream
^ permalink raw reply
* Re: [PATCH 3/4] net: dsa: vsc73xx: add support for parallel mode
From: Andrew Lunn @ 2019-07-01 16:11 UTC (permalink / raw)
To: Pawel Dembicki
Cc: linus.walleij, Vivien Didelot, Florian Fainelli, David S. Miller,
Rob Herring, Mark Rutland, netdev, devicetree, linux-kernel
In-Reply-To: <20190701152723.624-3-paweldembicki@gmail.com>
On Mon, Jul 01, 2019 at 05:27:22PM +0200, Pawel Dembicki wrote:
> +static int vsc73xx_platform_read(struct vsc73xx *vsc, u8 block, u8 subblock,
> + u8 reg, u32 *val)
> +{
> + struct vsc73xx_platform *vsc_platform = vsc->priv;
> + u32 offset;
> +
> + if (!vsc73xx_is_addr_valid(block, subblock))
> + return -EINVAL;
> +
> + offset = vsc73xx_make_addr(block, subblock, reg);
> +
> + mutex_lock(&vsc->lock);
> + *val = ioread32be(vsc_platform->base_addr + offset);
> + mutex_unlock(&vsc->lock);
Hi Pawel
What is this mutex protecting?
Plus the indentation is wrong.
Thanks
Andrew
^ permalink raw reply
* Re: [PATCH v4 bpf-next 0/9] libbpf: add bpf_link and tracing attach APIs
From: Stanislav Fomichev @ 2019-07-01 16:10 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: andrii.nakryiko, bpf, netdev, ast, daniel, kernel-team,
songliubraving
In-Reply-To: <20190629034906.1209916-1-andriin@fb.com>
On 06/28, Andrii Nakryiko wrote:
> This patchset adds the following APIs to allow attaching BPF programs to
> tracing entities:
> - bpf_program__attach_perf_event for attaching to any opened perf event FD,
> allowing users full control;
> - bpf_program__attach_kprobe for attaching to kernel probes (both entry and
> return probes);
> - bpf_program__attach_uprobe for attaching to user probes (both entry/return);
> - bpf_program__attach_tracepoint for attaching to kernel tracepoints;
> - bpf_program__attach_raw_tracepoint for attaching to raw kernel tracepoint
> (wrapper around bpf_raw_tracepoint_open);
>
> This set of APIs makes libbpf more useful for tracing applications.
>
> All attach APIs return abstract struct bpf_link that encapsulates logic of
> detaching BPF program. See patch #2 for details. bpf_assoc was considered as
> an alternative name for this opaque "handle", but bpf_link seems to be
> appropriate semantically and is nice and short.
>
> Pre-patch #1 makes internal libbpf_strerror_r helper function work w/ negative
> error codes, lifting the burder off callers to keep track of error sign.
> Patch #2 adds bpf_link abstraction.
> Patch #3 adds attach_perf_event, which is the base for all other APIs.
> Patch #4 adds kprobe/uprobe APIs.
> Patch #5 adds tracepoint API.
> Patch #6 adds raw_tracepoint API.
> Patch #7 converts one existing test to use attach_perf_event.
> Patch #8 adds new kprobe/uprobe tests.
> Patch #9 converts some selftests currently using tracepoint to new APIs.
>
> v3->v4:
> - proper errno handling (Stanislav);
> - bpf_fd -> prog_fd (Stanislav);
> - switch to fprintf (Song);
Reviewed-by: Stanislav Fomichev <sdf@google.com>
Thanks!
> v2->v3:
> - added bpf_link concept (Daniel);
> - didn't add generic bpf_link__attach_program for reasons described in [0];
> - dropped Stanislav's Reviewed-by from patches #2-#6, in case he doesn't like
> the change;
> v1->v2:
> - preserve errno before close() call (Stanislav);
> - use libbpf_perf_event_disable_and_close in selftest (Stanislav);
> - remove unnecessary memset (Stanislav);
>
> [0] https://lore.kernel.org/bpf/CAEf4BzZ7EM5eP2eaZn7T2Yb5QgVRiwAs+epeLR1g01TTx-6m6Q@mail.gmail.com/
>
> Andrii Nakryiko (9):
> libbpf: make libbpf_strerror_r agnostic to sign of error
> libbpf: introduce concept of bpf_link
> libbpf: add ability to attach/detach BPF program to perf event
> libbpf: add kprobe/uprobe attach API
> libbpf: add tracepoint attach API
> libbpf: add raw tracepoint attach API
> selftests/bpf: switch test to new attach_perf_event API
> selftests/bpf: add kprobe/uprobe selftests
> selftests/bpf: convert existing tracepoint tests to new APIs
>
> tools/lib/bpf/libbpf.c | 359 ++++++++++++++++++
> tools/lib/bpf/libbpf.h | 21 +
> tools/lib/bpf/libbpf.map | 8 +-
> tools/lib/bpf/str_error.c | 2 +-
> .../selftests/bpf/prog_tests/attach_probe.c | 155 ++++++++
> .../bpf/prog_tests/stacktrace_build_id.c | 50 +--
> .../bpf/prog_tests/stacktrace_build_id_nmi.c | 31 +-
> .../selftests/bpf/prog_tests/stacktrace_map.c | 43 +--
> .../bpf/prog_tests/stacktrace_map_raw_tp.c | 15 +-
> .../selftests/bpf/progs/test_attach_probe.c | 55 +++
> 10 files changed, 644 insertions(+), 95 deletions(-)
> create mode 100644 tools/testing/selftests/bpf/prog_tests/attach_probe.c
> create mode 100644 tools/testing/selftests/bpf/progs/test_attach_probe.c
>
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH net] ipv4: don't set IPv6 only flags to IPv4 addresses
From: Joe Perches @ 2019-07-01 16:10 UTC (permalink / raw)
To: Matteo Croce, netdev
Cc: linux-kernel, David S. Miller, Alexey Kuznetsov,
Hideaki YOSHIFUJI
In-Reply-To: <20190701160805.32404-1-mcroce@redhat.com>
On Mon, 2019-07-01 at 18:08 +0200, Matteo Croce wrote:
> Avoid the situation where an IPV6 only flag is applied to an IPv4 address:
[]
> diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
[]
> @@ -468,6 +473,9 @@ static int __inet_insert_ifa(struct in_ifaddr *ifa, struct nlmsghdr *nlh,
> ifa->ifa_flags &= ~IFA_F_SECONDARY;
> last_primary = &in_dev->ifa_list;
>
> + /* Don't set IPv6 only flags to IPv6 addresses */
umm, IPv4 addresses?
^ permalink raw reply
* [PATCH net] ipv4: don't set IPv6 only flags to IPv4 addresses
From: Matteo Croce @ 2019-07-01 16:08 UTC (permalink / raw)
To: netdev; +Cc: linux-kernel, David S. Miller, Alexey Kuznetsov,
Hideaki YOSHIFUJI
Avoid the situation where an IPV6 only flag is applied to an IPv4 address:
# ip addr add 192.0.2.1/24 dev dummy0 nodad home mngtmpaddr noprefixroute
# ip -4 addr show dev dummy0
2: dummy0: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 qdisc noqueue state UNKNOWN group default qlen 1000
inet 192.0.2.1/24 scope global noprefixroute dummy0
valid_lft forever preferred_lft forever
Or worse, by sending a malicious netlink command:
# ip -4 addr show dev dummy0
2: dummy0: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 qdisc noqueue state UNKNOWN group default qlen 1000
inet 192.0.2.1/24 scope global nodad optimistic dadfailed home tentative mngtmpaddr noprefixroute stable-privacy dummy0
valid_lft forever preferred_lft forever
Signed-off-by: Matteo Croce <mcroce@redhat.com>
---
net/ipv4/devinet.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index c6bd0f7a020a..f40ccdcf4cfe 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -62,6 +62,11 @@
#include <net/net_namespace.h>
#include <net/addrconf.h>
+#define IPV6ONLY_FLAGS \
+ (IFA_F_NODAD | IFA_F_OPTIMISTIC | IFA_F_DADFAILED | \
+ IFA_F_HOMEADDRESS | IFA_F_TENTATIVE | \
+ IFA_F_MANAGETEMPADDR | IFA_F_STABLE_PRIVACY)
+
static struct ipv4_devconf ipv4_devconf = {
.data = {
[IPV4_DEVCONF_ACCEPT_REDIRECTS - 1] = 1,
@@ -468,6 +473,9 @@ static int __inet_insert_ifa(struct in_ifaddr *ifa, struct nlmsghdr *nlh,
ifa->ifa_flags &= ~IFA_F_SECONDARY;
last_primary = &in_dev->ifa_list;
+ /* Don't set IPv6 only flags to IPv6 addresses */
+ ifa->ifa_flags &= ~IPV6ONLY_FLAGS;
+
for (ifap = &in_dev->ifa_list; (ifa1 = *ifap) != NULL;
ifap = &ifa1->ifa_next) {
if (!(ifa1->ifa_flags & IFA_F_SECONDARY) &&
--
2.21.0
^ permalink raw reply related
* Re: [PATCH bpf-next 1/2] bpf: allow wide (u64) aligned stores for some fields of bpf_sock_addr
From: Stanislav Fomichev @ 2019-07-01 16:04 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Yonghong Song, Stanislav Fomichev, netdev@vger.kernel.org,
bpf@vger.kernel.org, davem@davemloft.net, ast@kernel.org,
daniel@iogearbox.net, Andrii Nakryiko, kernel test robot
In-Reply-To: <CAEf4BzbT7h2oDapgSwQr8gSMnunCssqu88KMdymMjgBGpZpA4Q@mail.gmail.com>
On 07/01, Andrii Nakryiko wrote:
> On Sat, Jun 29, 2019 at 10:53 PM Yonghong Song <yhs@fb.com> wrote:
> >
> >
> >
> > On 6/28/19 4:10 PM, Stanislav Fomichev wrote:
> > > Since commit cd17d7770578 ("bpf/tools: sync bpf.h") clang decided
> > > that it can do a single u64 store into user_ip6[2] instead of two
> > > separate u32 ones:
> > >
> > > # 17: (18) r2 = 0x100000000000000
> > > # ; ctx->user_ip6[2] = bpf_htonl(DST_REWRITE_IP6_2);
> > > # 19: (7b) *(u64 *)(r1 +16) = r2
> > > # invalid bpf_context access off=16 size=8
> > >
> > > From the compiler point of view it does look like a correct thing
> > > to do, so let's support it on the kernel side.
> > >
> > > Credit to Andrii Nakryiko for a proper implementation of
> > > bpf_ctx_wide_store_ok.
> > >
> > > Cc: Andrii Nakryiko <andriin@fb.com>
> > > Cc: Yonghong Song <yhs@fb.com>
> > > Fixes: cd17d7770578 ("bpf/tools: sync bpf.h")
> > > Reported-by: kernel test robot <rong.a.chen@intel.com>
> > > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> >
> > The change looks good to me with the following nits:
> > 1. could you add a cover letter for the patch set?
> > typically if the number of patches is more than one,
> > it would be a good practice with a cover letter.
> > See bpf_devel_QA.rst .
> > 2. with this change, the comments in uapi bpf.h
> > are not accurate any more.
> > __u32 user_ip6[4]; /* Allows 1,2,4-byte read an 4-byte write.
> > * Stored in network byte order.
> >
> > */
> > __u32 msg_src_ip6[4]; /* Allows 1,2,4-byte read an 4-byte write.
> > * Stored in network byte order.
> > */
> > now for stores, aligned 8-byte write is permitted.
> > could you update this as well?
> >
> > From the typical usage pattern, I did not see a need
> > for 8-tye read of user_ip6 and msg_src_ip6 yet. So let
> > us just deal with write for now.
>
> But I guess it's still possible for clang to optimize two consecutive
> 4-byte reads into single 8-byte read in some circumstances? If that's
> the case, maybe it's a good idea to have corresponding read checks as
> well?
I guess clang can do those kinds of optimizations. I can put it on my
todo and address later (or when we actually see it out in the wild).
> But overall this looks good to me:
>
> Acked-by: Andrii Nakryiko <andriin@fb.com>
Thanks for a review!
> >
> > With the above two nits,
> > Acked-by: Yonghong Song <yhs@fb.com>
> >
> > > ---
> > > include/linux/filter.h | 6 ++++++
> > > net/core/filter.c | 22 ++++++++++++++--------
> > > 2 files changed, 20 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/include/linux/filter.h b/include/linux/filter.h
> > > index 340f7d648974..3901007e36f1 100644
> > > --- a/include/linux/filter.h
> > > +++ b/include/linux/filter.h
> > > @@ -746,6 +746,12 @@ bpf_ctx_narrow_access_ok(u32 off, u32 size, u32 size_default)
> > > return size <= size_default && (size & (size - 1)) == 0;
> > > }
> > >
> > > +#define bpf_ctx_wide_store_ok(off, size, type, field) \
> > > + (size == sizeof(__u64) && \
> > > + off >= offsetof(type, field) && \
> > > + off + sizeof(__u64) <= offsetofend(type, field) && \
> > > + off % sizeof(__u64) == 0)
> > > +
> > > #define bpf_classic_proglen(fprog) (fprog->len * sizeof(fprog->filter[0]))
> > >
> > > static inline void bpf_prog_lock_ro(struct bpf_prog *fp)
> > > diff --git a/net/core/filter.c b/net/core/filter.c
> > > index dc8534be12fc..5d33f2146dab 100644
> > > --- a/net/core/filter.c
> > > +++ b/net/core/filter.c
> > > @@ -6849,6 +6849,16 @@ static bool sock_addr_is_valid_access(int off, int size,
> > > if (!bpf_ctx_narrow_access_ok(off, size, size_default))
> > > return false;
> > > } else {
> > > + if (bpf_ctx_wide_store_ok(off, size,
> > > + struct bpf_sock_addr,
> > > + user_ip6))
> > > + return true;
> > > +
> > > + if (bpf_ctx_wide_store_ok(off, size,
> > > + struct bpf_sock_addr,
> > > + msg_src_ip6))
> > > + return true;
> > > +
> > > if (size != size_default)
> > > return false;
> > > }
> > > @@ -7689,9 +7699,6 @@ static u32 xdp_convert_ctx_access(enum bpf_access_type type,
> > > /* SOCK_ADDR_STORE_NESTED_FIELD_OFF() has semantic similar to
> > > * SOCK_ADDR_LOAD_NESTED_FIELD_SIZE_OFF() but for store operation.
> > > *
> > > - * It doesn't support SIZE argument though since narrow stores are not
> > > - * supported for now.
> > > - *
> > > * In addition it uses Temporary Field TF (member of struct S) as the 3rd
> > > * "register" since two registers available in convert_ctx_access are not
> > > * enough: we can't override neither SRC, since it contains value to store, nor
> > > @@ -7699,7 +7706,7 @@ static u32 xdp_convert_ctx_access(enum bpf_access_type type,
> > > * instructions. But we need a temporary place to save pointer to nested
> > > * structure whose field we want to store to.
> > > */
> > > -#define SOCK_ADDR_STORE_NESTED_FIELD_OFF(S, NS, F, NF, OFF, TF) \
> > > +#define SOCK_ADDR_STORE_NESTED_FIELD_OFF(S, NS, F, NF, SIZE, OFF, TF) \
> > > do { \
> > > int tmp_reg = BPF_REG_9; \
> > > if (si->src_reg == tmp_reg || si->dst_reg == tmp_reg) \
> > > @@ -7710,8 +7717,7 @@ static u32 xdp_convert_ctx_access(enum bpf_access_type type,
> > > offsetof(S, TF)); \
> > > *insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(S, F), tmp_reg, \
> > > si->dst_reg, offsetof(S, F)); \
> > > - *insn++ = BPF_STX_MEM( \
> > > - BPF_FIELD_SIZEOF(NS, NF), tmp_reg, si->src_reg, \
> > > + *insn++ = BPF_STX_MEM(SIZE, tmp_reg, si->src_reg, \
> > > bpf_target_off(NS, NF, FIELD_SIZEOF(NS, NF), \
> > > target_size) \
> > > + OFF); \
> > > @@ -7723,8 +7729,8 @@ static u32 xdp_convert_ctx_access(enum bpf_access_type type,
> > > TF) \
> > > do { \
> > > if (type == BPF_WRITE) { \
> > > - SOCK_ADDR_STORE_NESTED_FIELD_OFF(S, NS, F, NF, OFF, \
> > > - TF); \
> > > + SOCK_ADDR_STORE_NESTED_FIELD_OFF(S, NS, F, NF, SIZE, \
> > > + OFF, TF); \
> > > } else { \
> > > SOCK_ADDR_LOAD_NESTED_FIELD_SIZE_OFF( \
> > > S, NS, F, NF, SIZE, OFF); \
> > >
^ permalink raw reply
* Re: [PATCH 2/4] net: dsa: vsc73xx: Split vsc73xx driver
From: Andrew Lunn @ 2019-07-01 16:03 UTC (permalink / raw)
To: Pawel Dembicki
Cc: linus.walleij, Vivien Didelot, Florian Fainelli, David S. Miller,
Rob Herring, Mark Rutland, netdev, devicetree, linux-kernel
In-Reply-To: <20190701152723.624-2-paweldembicki@gmail.com>
> @@ -495,12 +380,12 @@ static int vsc73xx_update_bits(struct vsc73xx *vsc, u8 block, u8 subblock,
> int ret;
>
> /* Same read-modify-write algorithm as e.g. regmap */
> - ret = vsc73xx_read(vsc, block, subblock, reg, &orig);
> + ret = vsc->ops->read(vsc, block, subblock, reg, &orig);
> if (ret)
> return ret;
> tmp = orig & ~mask;
> tmp |= val & mask;
> - return vsc73xx_write(vsc, block, subblock, reg, tmp);
> + return vsc->ops->write(vsc, block, subblock, reg, tmp);
This patch would be a lot less invasive and smaller if you hid the
difference between SPI and platform inside vsc73xx_write() and
vsc73xx_read().
> -static int vsc73xx_probe(struct spi_device *spi)
> +int vsc73xx_probe(struct vsc73xx *vsc)
> {
> - struct device *dev = &spi->dev;
struct device *dev = vsc->dev;
and then a lot of the changes you make here go away.
In general, think about how to make the changes small. It saves your
time from actually making changes, and reviewer time since the patch
it smaller.
Andrew
^ permalink raw reply
* Re: [PATCH bpf-next 1/2] bpf: allow wide (u64) aligned stores for some fields of bpf_sock_addr
From: Stanislav Fomichev @ 2019-07-01 16:01 UTC (permalink / raw)
To: Yonghong Song
Cc: Stanislav Fomichev, netdev@vger.kernel.org, bpf@vger.kernel.org,
davem@davemloft.net, ast@kernel.org, daniel@iogearbox.net,
Andrii Nakryiko, kernel test robot
In-Reply-To: <be223396-b181-e587-d63c-2b15eaca3721@fb.com>
On 06/30, Yonghong Song wrote:
>
>
> On 6/28/19 4:10 PM, Stanislav Fomichev wrote:
> > Since commit cd17d7770578 ("bpf/tools: sync bpf.h") clang decided
> > that it can do a single u64 store into user_ip6[2] instead of two
> > separate u32 ones:
> >
> > # 17: (18) r2 = 0x100000000000000
> > # ; ctx->user_ip6[2] = bpf_htonl(DST_REWRITE_IP6_2);
> > # 19: (7b) *(u64 *)(r1 +16) = r2
> > # invalid bpf_context access off=16 size=8
> >
> > From the compiler point of view it does look like a correct thing
> > to do, so let's support it on the kernel side.
> >
> > Credit to Andrii Nakryiko for a proper implementation of
> > bpf_ctx_wide_store_ok.
> >
> > Cc: Andrii Nakryiko <andriin@fb.com>
> > Cc: Yonghong Song <yhs@fb.com>
> > Fixes: cd17d7770578 ("bpf/tools: sync bpf.h")
> > Reported-by: kernel test robot <rong.a.chen@intel.com>
> > Signed-off-by: Stanislav Fomichev <sdf@google.com>
>
> The change looks good to me with the following nits:
> 1. could you add a cover letter for the patch set?
> typically if the number of patches is more than one,
> it would be a good practice with a cover letter.
> See bpf_devel_QA.rst .
> 2. with this change, the comments in uapi bpf.h
> are not accurate any more.
> __u32 user_ip6[4]; /* Allows 1,2,4-byte read an 4-byte write.
> * Stored in network byte order.
>
> */
> __u32 msg_src_ip6[4]; /* Allows 1,2,4-byte read an 4-byte write.
> * Stored in network byte order.
> */
> now for stores, aligned 8-byte write is permitted.
> could you update this as well?
>
> From the typical usage pattern, I did not see a need
> for 8-tye read of user_ip6 and msg_src_ip6 yet. So let
> us just deal with write for now.
>
> With the above two nits,
> Acked-by: Yonghong Song <yhs@fb.com>
Thank you for a review, will follow up with a v2 shortly with both
things addressed!
> > ---
> > include/linux/filter.h | 6 ++++++
> > net/core/filter.c | 22 ++++++++++++++--------
> > 2 files changed, 20 insertions(+), 8 deletions(-)
> >
> > diff --git a/include/linux/filter.h b/include/linux/filter.h
> > index 340f7d648974..3901007e36f1 100644
> > --- a/include/linux/filter.h
> > +++ b/include/linux/filter.h
> > @@ -746,6 +746,12 @@ bpf_ctx_narrow_access_ok(u32 off, u32 size, u32 size_default)
> > return size <= size_default && (size & (size - 1)) == 0;
> > }
> >
> > +#define bpf_ctx_wide_store_ok(off, size, type, field) \
> > + (size == sizeof(__u64) && \
> > + off >= offsetof(type, field) && \
> > + off + sizeof(__u64) <= offsetofend(type, field) && \
> > + off % sizeof(__u64) == 0)
> > +
> > #define bpf_classic_proglen(fprog) (fprog->len * sizeof(fprog->filter[0]))
> >
> > static inline void bpf_prog_lock_ro(struct bpf_prog *fp)
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index dc8534be12fc..5d33f2146dab 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -6849,6 +6849,16 @@ static bool sock_addr_is_valid_access(int off, int size,
> > if (!bpf_ctx_narrow_access_ok(off, size, size_default))
> > return false;
> > } else {
> > + if (bpf_ctx_wide_store_ok(off, size,
> > + struct bpf_sock_addr,
> > + user_ip6))
> > + return true;
> > +
> > + if (bpf_ctx_wide_store_ok(off, size,
> > + struct bpf_sock_addr,
> > + msg_src_ip6))
> > + return true;
> > +
> > if (size != size_default)
> > return false;
> > }
> > @@ -7689,9 +7699,6 @@ static u32 xdp_convert_ctx_access(enum bpf_access_type type,
> > /* SOCK_ADDR_STORE_NESTED_FIELD_OFF() has semantic similar to
> > * SOCK_ADDR_LOAD_NESTED_FIELD_SIZE_OFF() but for store operation.
> > *
> > - * It doesn't support SIZE argument though since narrow stores are not
> > - * supported for now.
> > - *
> > * In addition it uses Temporary Field TF (member of struct S) as the 3rd
> > * "register" since two registers available in convert_ctx_access are not
> > * enough: we can't override neither SRC, since it contains value to store, nor
> > @@ -7699,7 +7706,7 @@ static u32 xdp_convert_ctx_access(enum bpf_access_type type,
> > * instructions. But we need a temporary place to save pointer to nested
> > * structure whose field we want to store to.
> > */
> > -#define SOCK_ADDR_STORE_NESTED_FIELD_OFF(S, NS, F, NF, OFF, TF) \
> > +#define SOCK_ADDR_STORE_NESTED_FIELD_OFF(S, NS, F, NF, SIZE, OFF, TF) \
> > do { \
> > int tmp_reg = BPF_REG_9; \
> > if (si->src_reg == tmp_reg || si->dst_reg == tmp_reg) \
> > @@ -7710,8 +7717,7 @@ static u32 xdp_convert_ctx_access(enum bpf_access_type type,
> > offsetof(S, TF)); \
> > *insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(S, F), tmp_reg, \
> > si->dst_reg, offsetof(S, F)); \
> > - *insn++ = BPF_STX_MEM( \
> > - BPF_FIELD_SIZEOF(NS, NF), tmp_reg, si->src_reg, \
> > + *insn++ = BPF_STX_MEM(SIZE, tmp_reg, si->src_reg, \
> > bpf_target_off(NS, NF, FIELD_SIZEOF(NS, NF), \
> > target_size) \
> > + OFF); \
> > @@ -7723,8 +7729,8 @@ static u32 xdp_convert_ctx_access(enum bpf_access_type type,
> > TF) \
> > do { \
> > if (type == BPF_WRITE) { \
> > - SOCK_ADDR_STORE_NESTED_FIELD_OFF(S, NS, F, NF, OFF, \
> > - TF); \
> > + SOCK_ADDR_STORE_NESTED_FIELD_OFF(S, NS, F, NF, SIZE, \
> > + OFF, TF); \
> > } else { \
> > SOCK_ADDR_LOAD_NESTED_FIELD_SIZE_OFF( \
> > S, NS, F, NF, SIZE, OFF); \
> >
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox