* [PATCH v4 0/2] net/ibmvnic: serialization fixes
From: Juliet Kim @ 2019-09-20 20:11 UTC (permalink / raw)
To: netdev; +Cc: julietk, tlfalcon, linuxppc-dev
This series includes two fixes. The first improves reset code to allow
linkwatch_event to proceed during reset. The second ensures that no more
than one thread runs in reset at a time.
v2:
- Separate change param reset from do_reset()
- Return IBMVNIC_OPEN_FAILED if __ibmvnic_open fails
- Remove setting wait_for_reset to false from __ibmvnic_reset(), this
is done in wait_for_reset()
- Move the check for force_reset_recovery from patch 1 to patch 2
v3:
- Restore reset’s successful return in open failure case
v4:
- Change resetting flag access to atomic
Juliet Kim (2):
net/ibmvnic: unlock rtnl_lock in reset so linkwatch_event can run
net/ibmvnic: prevent more than one thread from running in reset
drivers/net/ethernet/ibm/ibmvnic.c | 262 ++++++++++++++++++++++++++-----------
drivers/net/ethernet/ibm/ibmvnic.h | 6 +-
2 files changed, 190 insertions(+), 78 deletions(-)
--
2.16.4
^ permalink raw reply
* [PATCH v4 1/2] net/ibmvnic: unlock rtnl_lock in reset so linkwatch_event can run
From: Juliet Kim @ 2019-09-20 20:11 UTC (permalink / raw)
To: netdev; +Cc: julietk, tlfalcon, linuxppc-dev
In-Reply-To: <20190920201123.18913-1-julietk@linux.vnet.ibm.com>
Commit a5681e20b541 ("net/ibmnvic: Fix deadlock problem in reset")
made the change to hold the RTNL lock during a reset to avoid deadlock
but linkwatch_event is fired during the reset and needs the RTNL lock.
That keeps linkwatch_event process from proceeding until the reset
is complete. The reset process cannot tolerate the linkwatch_event
processing after reset completes, so release the RTNL lock during the
process to allow a chance for linkwatch_event to run during reset.
This does not guarantee that the linkwatch_event will be processed as
soon as link state changes, but is an improvement over the current code
where linkwatch_event processing is always delayed, which prevents
transmissions on the device from being deactivated leading transmit
watchdog timer to time-out.
Release the RTNL lock before link state change and re-acquire after
the link state change to allow linkwatch_event to grab the RTNL lock
and run during the reset.
Fixes: a5681e20b541 ("net/ibmnvic: Fix deadlock problem in reset")
Signed-off-by: Juliet Kim <julietk@linux.vnet.ibm.com>
---
drivers/net/ethernet/ibm/ibmvnic.c | 224 ++++++++++++++++++++++++++-----------
drivers/net/ethernet/ibm/ibmvnic.h | 1 +
2 files changed, 157 insertions(+), 68 deletions(-)
diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index 5cb55ea671e3..ba340aaff1b3 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -1723,6 +1723,86 @@ static int ibmvnic_set_mac(struct net_device *netdev, void *p)
return rc;
}
+/**
+ * do_change_param_reset returns zero if we are able to keep processing reset
+ * events, or non-zero if we hit a fatal error and must halt.
+ */
+static int do_change_param_reset(struct ibmvnic_adapter *adapter,
+ struct ibmvnic_rwi *rwi,
+ u32 reset_state)
+{
+ struct net_device *netdev = adapter->netdev;
+ int i, rc;
+
+ netdev_dbg(adapter->netdev, "Change param resetting driver (%d)\n",
+ rwi->reset_reason);
+
+ netif_carrier_off(netdev);
+ adapter->reset_reason = rwi->reset_reason;
+
+ ibmvnic_cleanup(netdev);
+
+ if (reset_state == VNIC_OPEN) {
+ rc = __ibmvnic_close(netdev);
+ if (rc)
+ return rc;
+ }
+
+ release_resources(adapter);
+ release_sub_crqs(adapter, 1);
+ release_crq_queue(adapter);
+
+ adapter->state = VNIC_PROBED;
+
+ rc = init_crq_queue(adapter);
+
+ if (rc) {
+ netdev_err(adapter->netdev,
+ "Couldn't initialize crq. rc=%d\n", rc);
+ return rc;
+ }
+
+ rc = ibmvnic_reset_init(adapter);
+ if (rc)
+ return IBMVNIC_INIT_FAILED;
+
+ /* If the adapter was in PROBE state prior to the reset,
+ * exit here.
+ */
+ if (reset_state == VNIC_PROBED)
+ return 0;
+
+ rc = ibmvnic_login(netdev);
+ if (rc) {
+ adapter->state = reset_state;
+ return rc;
+ }
+
+ rc = init_resources(adapter);
+ if (rc)
+ return rc;
+
+ ibmvnic_disable_irqs(adapter);
+
+ adapter->state = VNIC_CLOSED;
+
+ if (reset_state == VNIC_CLOSED)
+ return 0;
+
+ rc = __ibmvnic_open(netdev);
+ if (rc)
+ return IBMVNIC_OPEN_FAILED;
+
+ /* refresh device's multicast list */
+ ibmvnic_set_multi(netdev);
+
+ /* kick napi */
+ for (i = 0; i < adapter->req_rx_queues; i++)
+ napi_schedule(&adapter->napi[i]);
+
+ return 0;
+}
+
/**
* do_reset returns zero if we are able to keep processing reset events, or
* non-zero if we hit a fatal error and must halt.
@@ -1738,6 +1818,8 @@ static int do_reset(struct ibmvnic_adapter *adapter,
netdev_dbg(adapter->netdev, "Re-setting driver (%d)\n",
rwi->reset_reason);
+ rtnl_lock();
+
netif_carrier_off(netdev);
adapter->reset_reason = rwi->reset_reason;
@@ -1751,16 +1833,25 @@ static int do_reset(struct ibmvnic_adapter *adapter,
if (reset_state == VNIC_OPEN &&
adapter->reset_reason != VNIC_RESET_MOBILITY &&
adapter->reset_reason != VNIC_RESET_FAILOVER) {
- rc = __ibmvnic_close(netdev);
+ adapter->state = VNIC_CLOSING;
+
+ /* Release the RTNL lock before link state change and
+ * re-acquire after the link state change to allow
+ * linkwatch_event to grab the RTNL lock and run during
+ * a reset.
+ */
+ rtnl_unlock();
+ rc = set_link_state(adapter, IBMVNIC_LOGICAL_LNK_DN);
+ rtnl_lock();
if (rc)
- return rc;
- }
+ goto out;
- if (adapter->reset_reason == VNIC_RESET_CHANGE_PARAM ||
- adapter->wait_for_reset) {
- release_resources(adapter);
- release_sub_crqs(adapter, 1);
- release_crq_queue(adapter);
+ if (adapter->state != VNIC_CLOSING) {
+ rc = -1;
+ goto out;
+ }
+
+ adapter->state = VNIC_CLOSED;
}
if (adapter->reset_reason != VNIC_RESET_NON_FATAL) {
@@ -1769,9 +1860,7 @@ static int do_reset(struct ibmvnic_adapter *adapter,
*/
adapter->state = VNIC_PROBED;
- if (adapter->wait_for_reset) {
- rc = init_crq_queue(adapter);
- } else if (adapter->reset_reason == VNIC_RESET_MOBILITY) {
+ if (adapter->reset_reason == VNIC_RESET_MOBILITY) {
rc = ibmvnic_reenable_crq_queue(adapter);
release_sub_crqs(adapter, 1);
} else {
@@ -1783,36 +1872,35 @@ static int do_reset(struct ibmvnic_adapter *adapter,
if (rc) {
netdev_err(adapter->netdev,
"Couldn't initialize crq. rc=%d\n", rc);
- return rc;
+ goto out;
}
rc = ibmvnic_reset_init(adapter);
- if (rc)
- return IBMVNIC_INIT_FAILED;
+ if (rc) {
+ rc = IBMVNIC_INIT_FAILED;
+ goto out;
+ }
/* If the adapter was in PROBE state prior to the reset,
* exit here.
*/
- if (reset_state == VNIC_PROBED)
- return 0;
+ if (reset_state == VNIC_PROBED) {
+ rc = 0;
+ goto out;
+ }
rc = ibmvnic_login(netdev);
if (rc) {
adapter->state = reset_state;
- return rc;
+ goto out;
}
- if (adapter->reset_reason == VNIC_RESET_CHANGE_PARAM ||
- adapter->wait_for_reset) {
- rc = init_resources(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_rx_add_entries_per_subcrq !=
- old_num_rx_slots ||
- adapter->req_tx_entries_per_subcrq !=
- old_num_tx_slots) {
+ if (adapter->req_rx_queues != old_num_rx_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);
@@ -1820,32 +1908,30 @@ static int do_reset(struct ibmvnic_adapter *adapter,
rc = init_resources(adapter);
if (rc)
- return rc;
+ goto out;
} else {
rc = reset_tx_pools(adapter);
if (rc)
- return rc;
+ goto out;
rc = reset_rx_pools(adapter);
if (rc)
- return rc;
+ goto out;
}
ibmvnic_disable_irqs(adapter);
}
adapter->state = VNIC_CLOSED;
- if (reset_state == VNIC_CLOSED)
- return 0;
+ if (reset_state == VNIC_CLOSED) {
+ rc = 0;
+ goto out;
+ }
rc = __ibmvnic_open(netdev);
if (rc) {
- if (list_empty(&adapter->rwi_list))
- adapter->state = VNIC_CLOSED;
- else
- adapter->state = reset_state;
-
- return 0;
+ rc = IBMVNIC_OPEN_FAILED;
+ goto out;
}
/* refresh device's multicast list */
@@ -1855,11 +1941,15 @@ static int do_reset(struct ibmvnic_adapter *adapter,
for (i = 0; i < adapter->req_rx_queues; i++)
napi_schedule(&adapter->napi[i]);
- if (adapter->reset_reason != VNIC_RESET_FAILOVER &&
- adapter->reset_reason != VNIC_RESET_CHANGE_PARAM)
+ if (adapter->reset_reason != VNIC_RESET_FAILOVER)
call_netdevice_notifiers(NETDEV_NOTIFY_PEERS, netdev);
- return 0;
+ rc = 0;
+
+out:
+ rtnl_unlock();
+
+ return rc;
}
static int do_hard_reset(struct ibmvnic_adapter *adapter,
@@ -1919,14 +2009,8 @@ static int do_hard_reset(struct ibmvnic_adapter *adapter,
return 0;
rc = __ibmvnic_open(netdev);
- if (rc) {
- if (list_empty(&adapter->rwi_list))
- adapter->state = VNIC_CLOSED;
- else
- adapter->state = reset_state;
-
- return 0;
- }
+ if (rc)
+ return IBMVNIC_OPEN_FAILED;
return 0;
}
@@ -1965,20 +2049,11 @@ static void __ibmvnic_reset(struct work_struct *work)
{
struct ibmvnic_rwi *rwi;
struct ibmvnic_adapter *adapter;
- bool we_lock_rtnl = false;
u32 reset_state;
int rc = 0;
adapter = container_of(work, struct ibmvnic_adapter, ibmvnic_reset);
- /* netif_set_real_num_xx_queues needs to take rtnl lock here
- * unless wait_for_reset is set, in which case the rtnl lock
- * has already been taken before initializing the reset
- */
- if (!adapter->wait_for_reset) {
- rtnl_lock();
- we_lock_rtnl = true;
- }
reset_state = adapter->state;
rwi = get_next_rwi(adapter);
@@ -1990,14 +2065,32 @@ static void __ibmvnic_reset(struct work_struct *work)
break;
}
- if (adapter->force_reset_recovery) {
- adapter->force_reset_recovery = false;
- rc = do_hard_reset(adapter, rwi, reset_state);
+ if (rwi->reset_reason == VNIC_RESET_CHANGE_PARAM) {
+ /* CHANGE_PARAM requestor holds rtnl_lock */
+ rc = do_change_param_reset(adapter, rwi, reset_state);
+ } else if (adapter->force_reset_recovery) {
+ /* Transport event occurred during previous reset */
+ if (adapter->wait_for_reset) {
+ /* Previous was CHANGE_PARAM; caller locked */
+ adapter->force_reset_recovery = false;
+ rc = do_hard_reset(adapter, rwi, reset_state);
+ } else {
+ rtnl_lock();
+ adapter->force_reset_recovery = false;
+ rc = do_hard_reset(adapter, rwi, reset_state);
+ rtnl_unlock();
+ }
} else {
rc = do_reset(adapter, rwi, reset_state);
}
kfree(rwi);
- if (rc && rc != IBMVNIC_INIT_FAILED &&
+ if (rc == IBMVNIC_OPEN_FAILED) {
+ if (list_empty(&adapter->rwi_list))
+ adapter->state = VNIC_CLOSED;
+ else
+ adapter->state = reset_state;
+ rc = 0;
+ } else if (rc && rc != IBMVNIC_INIT_FAILED &&
!adapter->force_reset_recovery)
break;
@@ -2005,7 +2098,6 @@ static void __ibmvnic_reset(struct work_struct *work)
}
if (adapter->wait_for_reset) {
- adapter->wait_for_reset = false;
adapter->reset_done_rc = rc;
complete(&adapter->reset_done);
}
@@ -2016,8 +2108,6 @@ static void __ibmvnic_reset(struct work_struct *work)
}
adapter->resetting = false;
- if (we_lock_rtnl)
- rtnl_unlock();
}
static int ibmvnic_reset(struct ibmvnic_adapter *adapter,
@@ -2078,8 +2168,6 @@ static int ibmvnic_reset(struct ibmvnic_adapter *adapter,
return 0;
err:
- if (adapter->wait_for_reset)
- adapter->wait_for_reset = false;
return -ret;
}
diff --git a/drivers/net/ethernet/ibm/ibmvnic.h b/drivers/net/ethernet/ibm/ibmvnic.h
index 70bd286f8932..9d3d35cc91d6 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.h
+++ b/drivers/net/ethernet/ibm/ibmvnic.h
@@ -20,6 +20,7 @@
#define IBMVNIC_INVALID_MAP -1
#define IBMVNIC_STATS_TIMEOUT 1
#define IBMVNIC_INIT_FAILED 2
+#define IBMVNIC_OPEN_FAILED 3
/* basic structures plus 100 2k buffers */
#define IBMVNIC_IO_ENTITLEMENT_DEFAULT 610305
--
2.16.4
^ permalink raw reply related
* [PATCH v4 2/2] net/ibmvnic: prevent more than one thread from running in reset
From: Juliet Kim @ 2019-09-20 20:11 UTC (permalink / raw)
To: netdev; +Cc: julietk, tlfalcon, linuxppc-dev
In-Reply-To: <20190920201123.18913-1-julietk@linux.vnet.ibm.com>
The current code allows more than one thread to run in reset. This can
corrupt struct adapter data. Check adapter->resetting before performing
a reset, if there is another reset running delay (100 msec) before trying
again.
Signed-off-by: Juliet Kim <julietk@linux.vnet.ibm.com>
---
drivers/net/ethernet/ibm/ibmvnic.c | 40 ++++++++++++++++++++++++++++----------
drivers/net/ethernet/ibm/ibmvnic.h | 5 ++++-
2 files changed, 34 insertions(+), 11 deletions(-)
diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index ba340aaff1b3..6aef574acdf2 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -1207,7 +1207,7 @@ static void ibmvnic_cleanup(struct net_device *netdev)
struct ibmvnic_adapter *adapter = netdev_priv(netdev);
/* ensure that transmissions are stopped if called by do_reset */
- if (adapter->resetting)
+ if (test_bit(0, &adapter->resetting))
netif_tx_disable(netdev);
else
netif_tx_stop_all_queues(netdev);
@@ -1428,7 +1428,7 @@ static netdev_tx_t ibmvnic_xmit(struct sk_buff *skb, struct net_device *netdev)
u8 proto = 0;
netdev_tx_t ret = NETDEV_TX_OK;
- if (adapter->resetting) {
+ if (test_bit(0, &adapter->resetting)) {
if (!netif_subqueue_stopped(netdev, skb))
netif_stop_subqueue(netdev, queue_num);
dev_kfree_skb_any(skb);
@@ -2054,6 +2054,12 @@ static void __ibmvnic_reset(struct work_struct *work)
adapter = container_of(work, struct ibmvnic_adapter, ibmvnic_reset);
+ if (test_and_set_bit_lock(0, &adapter->resetting)) {
+ schedule_delayed_work(&adapter->ibmvnic_delayed_reset,
+ IBMVNIC_RESET_DELAY);
+ return;
+ }
+
reset_state = adapter->state;
rwi = get_next_rwi(adapter);
@@ -2095,6 +2101,10 @@ static void __ibmvnic_reset(struct work_struct *work)
break;
rwi = get_next_rwi(adapter);
+
+ if (rwi && (rwi->reset_reason == VNIC_RESET_FAILOVER ||
+ rwi->reset_reason == VNIC_RESET_MOBILITY))
+ adapter->force_reset_recovery = true;
}
if (adapter->wait_for_reset) {
@@ -2107,7 +2117,16 @@ static void __ibmvnic_reset(struct work_struct *work)
free_all_rwi(adapter);
}
- adapter->resetting = false;
+ clear_bit_unlock(0, &adapter->resetting);
+}
+
+static void __ibmvnic_delayed_reset(struct work_struct *work)
+{
+ struct ibmvnic_adapter *adapter;
+
+ adapter = container_of(work, struct ibmvnic_adapter,
+ ibmvnic_delayed_reset.work);
+ __ibmvnic_reset(&adapter->ibmvnic_reset);
}
static int ibmvnic_reset(struct ibmvnic_adapter *adapter,
@@ -2162,7 +2181,6 @@ static int ibmvnic_reset(struct ibmvnic_adapter *adapter,
rwi->reset_reason = reason;
list_add_tail(&rwi->list, &adapter->rwi_list);
spin_unlock_irqrestore(&adapter->rwi_lock, flags);
- adapter->resetting = true;
netdev_dbg(adapter->netdev, "Scheduling reset (reason %d)\n", reason);
schedule_work(&adapter->ibmvnic_reset);
@@ -2207,7 +2225,7 @@ static int ibmvnic_poll(struct napi_struct *napi, int budget)
u16 offset;
u8 flags = 0;
- if (unlikely(adapter->resetting &&
+ if (unlikely(test_bit(0, &adapter->resetting) &&
adapter->reset_reason != VNIC_RESET_NON_FATAL)) {
enable_scrq_irq(adapter, adapter->rx_scrq[scrq_num]);
napi_complete_done(napi, frames_processed);
@@ -2858,7 +2876,7 @@ static int enable_scrq_irq(struct ibmvnic_adapter *adapter,
return 1;
}
- if (adapter->resetting &&
+ if (test_bit(0, &adapter->resetting) &&
adapter->reset_reason == VNIC_RESET_MOBILITY) {
u64 val = (0xff000000) | scrq->hw_irq;
@@ -3408,7 +3426,7 @@ static int ibmvnic_send_crq(struct ibmvnic_adapter *adapter,
if (rc) {
if (rc == H_CLOSED) {
dev_warn(dev, "CRQ Queue closed\n");
- if (adapter->resetting)
+ if (test_bit(0, &adapter->resetting))
ibmvnic_reset(adapter, VNIC_RESET_FATAL);
}
@@ -4483,7 +4501,7 @@ static void ibmvnic_handle_crq(union ibmvnic_crq *crq,
case IBMVNIC_CRQ_XPORT_EVENT:
netif_carrier_off(netdev);
adapter->crq.active = false;
- if (adapter->resetting)
+ if (test_bit(0, &adapter->resetting))
adapter->force_reset_recovery = true;
if (gen_crq->cmd == IBMVNIC_PARTITION_MIGRATED) {
dev_info(dev, "Migrated, re-enabling adapter\n");
@@ -4821,7 +4839,7 @@ static int ibmvnic_reset_init(struct ibmvnic_adapter *adapter)
return -1;
}
- if (adapter->resetting && !adapter->wait_for_reset &&
+ if (test_bit(0, &adapter->resetting) && !adapter->wait_for_reset &&
adapter->reset_reason != VNIC_RESET_MOBILITY) {
if (adapter->req_rx_queues != old_num_rx_queues ||
adapter->req_tx_queues != old_num_tx_queues) {
@@ -4933,10 +4951,12 @@ static int ibmvnic_probe(struct vio_dev *dev, const struct vio_device_id *id)
spin_lock_init(&adapter->stats_lock);
INIT_WORK(&adapter->ibmvnic_reset, __ibmvnic_reset);
+ INIT_DELAYED_WORK(&adapter->ibmvnic_delayed_reset,
+ __ibmvnic_delayed_reset);
INIT_LIST_HEAD(&adapter->rwi_list);
spin_lock_init(&adapter->rwi_lock);
init_completion(&adapter->init_done);
- adapter->resetting = false;
+ clear_bit(0, &adapter->resetting);
do {
rc = init_crq_queue(adapter);
diff --git a/drivers/net/ethernet/ibm/ibmvnic.h b/drivers/net/ethernet/ibm/ibmvnic.h
index 9d3d35cc91d6..ebc39248b334 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.h
+++ b/drivers/net/ethernet/ibm/ibmvnic.h
@@ -39,6 +39,8 @@
#define IBMVNIC_MAX_LTB_SIZE ((1 << (MAX_ORDER - 1)) * PAGE_SIZE)
#define IBMVNIC_BUFFER_HLEN 500
+#define IBMVNIC_RESET_DELAY 100
+
static const char ibmvnic_priv_flags[][ETH_GSTRING_LEN] = {
#define IBMVNIC_USE_SERVER_MAXES 0x1
"use-server-maxes"
@@ -1077,7 +1079,8 @@ struct ibmvnic_adapter {
spinlock_t rwi_lock;
struct list_head rwi_list;
struct work_struct ibmvnic_reset;
- bool resetting;
+ struct delayed_work ibmvnic_delayed_reset;
+ unsigned long resetting;
bool napi_enabled, from_passive_init;
bool failover_pending;
--
2.16.4
^ permalink raw reply related
* Re: [PATCH bpf] libbpf: fix version identification on busybox
From: Ivan Khoronzhuk @ 2019-09-20 20:48 UTC (permalink / raw)
To: Andrii Nakryiko, Alexei Starovoitov, Daniel Borkmann, Martin Lau,
Andrii Nakryiko, Yonghong Song, Networking, bpf, open list
In-Reply-To: <20190920191941.GB2760@khorivan>
On Fri, Sep 20, 2019 at 10:19:43PM +0300, Ivan Khoronzhuk wrote:
>On Fri, Sep 20, 2019 at 09:34:51PM +0300, Ivan Khoronzhuk wrote:
>>On Fri, Sep 20, 2019 at 09:41:54AM -0700, Andrii Nakryiko wrote:
>>>On Fri, Sep 20, 2019 at 1:22 AM Ivan Khoronzhuk
>>><ivan.khoronzhuk@linaro.org> wrote:
>>>>
>>>>On Thu, Sep 19, 2019 at 01:02:40PM -0700, Andrii Nakryiko wrote:
>>>>>On Thu, Sep 19, 2019 at 11:22 AM Ivan Khoronzhuk
>>>>><ivan.khoronzhuk@linaro.org> wrote:
>>>>>>
>>>>>>It's very often for embedded to have stripped version of sort in
>>>>>>busybox, when no -V option present. It breaks build natively on target
>>>>>>board causing recursive loop.
>>>>>>
>>>>>>BusyBox v1.24.1 (2019-04-06 04:09:16 UTC) multi-call binary. \
>>>>>>Usage: sort [-nrugMcszbdfimSTokt] [-o FILE] [-k \
>>>>>>start[.offset][opts][,end[.offset][opts]] [-t CHAR] [FILE]...
>>>>>>
>>>>>>Lets modify command a little to avoid -V option.
>>>>>>
>>>>>>Fixes: dadb81d0afe732 ("libbpf: make libbpf.map source of truth for libbpf version")
>>>>>>
>>>>>>Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
>>>>>>---
>>>>>>
>>>>>>Based on bpf/master
>>>>>>
>>>>>> tools/lib/bpf/Makefile | 2 +-
>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>>diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile
>>>>>>index c6f94cffe06e..a12490ad6215 100644
>>>>>>--- a/tools/lib/bpf/Makefile
>>>>>>+++ b/tools/lib/bpf/Makefile
>>>>>>@@ -3,7 +3,7 @@
>>>>>>
>>>>>> LIBBPF_VERSION := $(shell \
>>>>>> grep -oE '^LIBBPF_([0-9.]+)' libbpf.map | \
>>>>>>- sort -rV | head -n1 | cut -d'_' -f2)
>>>>>>+ cut -d'_' -f2 | sort -r | head -n1)
>>>>>
>>>>>You can't just sort alphabetically, because:
>>>>>
>>>>>1.2
>>>>>1.11
>>>>>
>>>>>should be in that order. See discussion on mailing thread for original commit.
>>>>
>>>>if X1.X2.X3, where X = {0,1,....99999}
>>>>Then it can be:
>>>>
>>>>-LIBBPF_VERSION := $(shell \
>>>>- grep -oE '^LIBBPF_([0-9.]+)' libbpf.map | \
>>>>- sort -rV | head -n1 | cut -d'_' -f2)
>>>>+_LBPFLIST := $(patsubst %;,%,$(patsubst LIBBPF_%,%,$(filter LIBBPF_%, \
>>>>+ $(shell cat libbpf.map))))
>>>>+_LBPFLIST2 := $(foreach v,$(_LBPFLIST), \
>>>>+ $(subst $() $(),,$(foreach n,$(subst .,$() $(),$(v)), \
>>>>+ $(shell printf "%05d" $(n)))))
>>>>+_LBPF_VER := $(word $(words $(sort $(_LBPFLIST2))), $(sort $(_LBPFLIST2)))
>>>>+LIBBPF_VERSION := $(patsubst %_$(_LBPF_VER),%,$(filter %_$(_LBPF_VER), \
>>>>+ $(join $(addsuffix _, $(_LBPFLIST)),$(_LBPFLIST2))))
>>>>
>>>>It's bigger but avoids invocations of grep/sort/cut/head, only cat/printf
>>>>, thus -V option also.
>>>>
>>>
>>>No way, this is way too ugly (and still unreliable, if we ever have
>>>X.Y.Z.W or something). I'd rather go with my original approach of
>>Yes, forgot to add
>>X1,X2,X3,...XN, where X = {0,1,....99999} and N = const for all versions.
>>But frankly, 1.0.0 looks too far.
>
>It actually works for any numbs of X1.X2...X100
>but not when you have couple kindof:
>X1.X2.X3
>and
>X1.X2.X3.X4
>
>But, no absolutely any problem to extend this solution to handle all cases,
>by just adding leading 0 to every "transformed version", say limit it to 10
>possible 'dots' (%5*10d) and it will work as clocks. Advantage - mostly make
>functions.
_LBPFLIST := $(subst ;,,$(patsubst LIBBPF_%,%,$(filter LIBBPF_%, \
$(shell cat libbpf.map))))
_LBPF2 := $(foreach v,$(_LBPFLIST), \
$(subst $() $(),,$(foreach n,$(subst ., ,$(v)), \
$(shell printf "%05d" $(n)))))
_LBPF2 := $(foreach v,$(_LBPF2), $(shell printf "%050s" $(v)))
_LBPF_VER := $(word $(words $(sort $(_LBPF2))), $(sort $(_LBPF2)))
LIBBPF_VERSION := $(patsubst %_$(_LBPF_VER),%,$(filter %_$(_LBPF_VER), \
$(join $(addsuffix _, $(_LBPFLIST)),$(_LBPF2))))
>
>Here can be couple more solutions with sed, not sure it can look less maniac.
>
>>
>>>fetching the last version in libbpf.map file. See
>>>https://www.spinics.net/lists/netdev/msg592703.html.
>
>Yes it's nice but, no sort, no X1.X2.X3....XN
>
>Main is to solve it for a long time.
>
>>>
>>>>>
>>>>>> LIBBPF_MAJOR_VERSION := $(firstword $(subst ., ,$(LIBBPF_VERSION)))
>>>>>>
>>>>>> MAKEFLAGS += --no-print-directory
>>>>>>--
>>>>>>2.17.1
>>>>>>
>>>>
>>>>--
>>>>Regards,
>>>>Ivan Khoronzhuk
>>
>>--
>>Regards,
>>Ivan Khoronzhuk
>
>--
>Regards,
>Ivan Khoronzhuk
--
Regards,
Ivan Khoronzhuk
^ permalink raw reply
* Re: [PATCH v5.1-rc] iwlwifi: make locking in iwl_mvm_tx_mpdu() BH-safe
From: Jiri Kosina @ 2019-09-20 21:34 UTC (permalink / raw)
To: Johannes Berg
Cc: Emmanuel Grumbach, Luca Coelho, Intel Linux Wireless, Kalle Valo,
David S. Miller, netdev, linux-kernel
In-Reply-To: <nycvar.YFH.7.76.1909111301510.473@cbobk.fhfr.pm>
On Wed, 11 Sep 2019, Jiri Kosina wrote:
> > From: Jiri Kosina <jkosina@suse.cz>
> > Subject: [PATCH] iwlwifi: make locking in iwl_mvm_tx_mpdu() BH-safe
>
> Hm, scratch that, that might actually spuriously enable BHs if called from
> contexts that already did disabled BHs.
>
> So what solution would you prefer here? Just stick another par of
> bh_disable() / bh_enable() somewhere to the wake_txs() ->
> iwl_mvm_mac_itxq_xmit() -> iwl_mvm_tx_skb() -> iwl_mvm_tx_mpdu() path?
Ping? This seems to be still the case.
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH bpf] libbpf: fix version identification on busybox
From: Andrii Nakryiko @ 2019-09-20 21:51 UTC (permalink / raw)
To: Ivan Khoronzhuk
Cc: Alexei Starovoitov, Daniel Borkmann, Martin Lau, Andrii Nakryiko,
Yonghong Song, Networking, bpf, open list
In-Reply-To: <20190920191941.GB2760@khorivan>
On Fri, Sep 20, 2019 at 12:19 PM Ivan Khoronzhuk
<ivan.khoronzhuk@linaro.org> wrote:
>
> On Fri, Sep 20, 2019 at 09:34:51PM +0300, Ivan Khoronzhuk wrote:
> >On Fri, Sep 20, 2019 at 09:41:54AM -0700, Andrii Nakryiko wrote:
> >>On Fri, Sep 20, 2019 at 1:22 AM Ivan Khoronzhuk
> >><ivan.khoronzhuk@linaro.org> wrote:
> >>>
> >>>On Thu, Sep 19, 2019 at 01:02:40PM -0700, Andrii Nakryiko wrote:
> >>>>On Thu, Sep 19, 2019 at 11:22 AM Ivan Khoronzhuk
> >>>><ivan.khoronzhuk@linaro.org> wrote:
> >>>>>
> >>>>> It's very often for embedded to have stripped version of sort in
> >>>>> busybox, when no -V option present. It breaks build natively on target
> >>>>> board causing recursive loop.
> >>>>>
> >>>>> BusyBox v1.24.1 (2019-04-06 04:09:16 UTC) multi-call binary. \
> >>>>> Usage: sort [-nrugMcszbdfimSTokt] [-o FILE] [-k \
> >>>>> start[.offset][opts][,end[.offset][opts]] [-t CHAR] [FILE]...
> >>>>>
> >>>>> Lets modify command a little to avoid -V option.
> >>>>>
> >>>>> Fixes: dadb81d0afe732 ("libbpf: make libbpf.map source of truth for libbpf version")
> >>>>>
> >>>>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> >>>>> ---
> >>>>>
> >>>>> Based on bpf/master
> >>>>>
> >>>>> tools/lib/bpf/Makefile | 2 +-
> >>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile
> >>>>> index c6f94cffe06e..a12490ad6215 100644
> >>>>> --- a/tools/lib/bpf/Makefile
> >>>>> +++ b/tools/lib/bpf/Makefile
> >>>>> @@ -3,7 +3,7 @@
> >>>>>
> >>>>> LIBBPF_VERSION := $(shell \
> >>>>> grep -oE '^LIBBPF_([0-9.]+)' libbpf.map | \
> >>>>> - sort -rV | head -n1 | cut -d'_' -f2)
> >>>>> + cut -d'_' -f2 | sort -r | head -n1)
> >>>>
> >>>>You can't just sort alphabetically, because:
> >>>>
> >>>>1.2
> >>>>1.11
> >>>>
> >>>>should be in that order. See discussion on mailing thread for original commit.
> >>>
> >>>if X1.X2.X3, where X = {0,1,....99999}
> >>>Then it can be:
> >>>
> >>>-LIBBPF_VERSION := $(shell \
> >>>- grep -oE '^LIBBPF_([0-9.]+)' libbpf.map | \
> >>>- sort -rV | head -n1 | cut -d'_' -f2)
> >>>+_LBPFLIST := $(patsubst %;,%,$(patsubst LIBBPF_%,%,$(filter LIBBPF_%, \
> >>>+ $(shell cat libbpf.map))))
> >>>+_LBPFLIST2 := $(foreach v,$(_LBPFLIST), \
> >>>+ $(subst $() $(),,$(foreach n,$(subst .,$() $(),$(v)), \
> >>>+ $(shell printf "%05d" $(n)))))
> >>>+_LBPF_VER := $(word $(words $(sort $(_LBPFLIST2))), $(sort $(_LBPFLIST2)))
> >>>+LIBBPF_VERSION := $(patsubst %_$(_LBPF_VER),%,$(filter %_$(_LBPF_VER), \
> >>>+ $(join $(addsuffix _, $(_LBPFLIST)),$(_LBPFLIST2))))
> >>>
> >>>It's bigger but avoids invocations of grep/sort/cut/head, only cat/printf
> >>>, thus -V option also.
> >>>
> >>
> >>No way, this is way too ugly (and still unreliable, if we ever have
> >>X.Y.Z.W or something). I'd rather go with my original approach of
> >Yes, forgot to add
> >X1,X2,X3,...XN, where X = {0,1,....99999} and N = const for all versions.
> >But frankly, 1.0.0 looks too far.
>
> It actually works for any numbs of X1.X2...X100
> but not when you have couple kindof:
> X1.X2.X3
> and
> X1.X2.X3.X4
>
> But, no absolutely any problem to extend this solution to handle all cases,
> by just adding leading 0 to every "transformed version", say limit it to 10
> possible 'dots' (%5*10d) and it will work as clocks. Advantage - mostly make
> functions.
>
> Here can be couple more solutions with sed, not sure it can look less maniac.
>
> >
> >>fetching the last version in libbpf.map file. See
> >>https://www.spinics.net/lists/netdev/msg592703.html.
>
> Yes it's nice but, no sort, no X1.X2.X3....XN
>
> Main is to solve it for a long time.
Thinking a bit more about this, I'm even more convinced that we should
just go with my original approach: find last section in libbpf.map and
extract LIBBPF version from that. That will handle whatever crazy
version format we might decide to use (e.g., 1.2.3-experimental).
We'll just need to make sure that latest version is the last in
libbpf.map, which will just happen naturally. So instead of this
Makefile complexity, please can you port back my original approach?
Thanks!
>
> >>
> >>>>
> >>>>> LIBBPF_MAJOR_VERSION := $(firstword $(subst ., ,$(LIBBPF_VERSION)))
> >>>>>
> >>>>> MAKEFLAGS += --no-print-directory
> >>>>> --
> >>>>> 2.17.1
> >>>>>
> >>>
> >>>--
> >>>Regards,
> >>>Ivan Khoronzhuk
> >
> >--
> >Regards,
> >Ivan Khoronzhuk
>
> --
> Regards,
> Ivan Khoronzhuk
^ permalink raw reply
* Re: [0/2] net: dsa: vsc73xx: Adjustments for vsc73xx_platform_probe()
From: Jakub Kicinski @ 2019-09-20 22:12 UTC (permalink / raw)
To: Florian Fainelli
Cc: Markus Elfring, Andrew Lunn, netdev, David S. Miller,
Pawel Dembicki, Vivien Didelot, Linus Walleij, kernel-janitors,
LKML
In-Reply-To: <5d068275-796d-7d76-ba33-6eb03fb1d7cc@gmail.com>
On Fri, 20 Sep 2019 09:36:57 -0700, Florian Fainelli wrote:
> On 9/20/19 8:30 AM, Markus Elfring wrote:
> >> netdev is closed at the moment for patch.
> >
> > I wonder about this information.
>
> This is covered here:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/networking/netdev-FAQ.rst#n40
>
> and you can skip the reading and check this URL:
>
> http://vger.kernel.org/~davem/net-next.html
Indeed, looks like we have a mix of clean ups of varying clarity here.
I will just drop all, including the devm_platform_ioremap_resource()
conversion patches from patchwork for now.
Markus, please repost them all once net-next opens.
Sorry for inconvenience.
^ permalink raw reply
* Re: [PATCH bpf] libbpf: fix version identification on busybox
From: Ivan Khoronzhuk @ 2019-09-20 22:14 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Alexei Starovoitov, Daniel Borkmann, Martin Lau, Andrii Nakryiko,
Yonghong Song, Networking, bpf, open list
In-Reply-To: <CAEf4BzZGeY-WD17mq6FTd7Rae_f26j4kBAWCmuppeu4VjZxvUg@mail.gmail.com>
On Fri, Sep 20, 2019 at 02:51:14PM -0700, Andrii Nakryiko wrote:
>On Fri, Sep 20, 2019 at 12:19 PM Ivan Khoronzhuk
><ivan.khoronzhuk@linaro.org> wrote:
>>
>> On Fri, Sep 20, 2019 at 09:34:51PM +0300, Ivan Khoronzhuk wrote:
>> >On Fri, Sep 20, 2019 at 09:41:54AM -0700, Andrii Nakryiko wrote:
>> >>On Fri, Sep 20, 2019 at 1:22 AM Ivan Khoronzhuk
>> >><ivan.khoronzhuk@linaro.org> wrote:
>> >>>
>> >>>On Thu, Sep 19, 2019 at 01:02:40PM -0700, Andrii Nakryiko wrote:
>> >>>>On Thu, Sep 19, 2019 at 11:22 AM Ivan Khoronzhuk
>> >>>><ivan.khoronzhuk@linaro.org> wrote:
>> >>>>>
>> >>>>> It's very often for embedded to have stripped version of sort in
>> >>>>> busybox, when no -V option present. It breaks build natively on target
>> >>>>> board causing recursive loop.
>> >>>>>
>> >>>>> BusyBox v1.24.1 (2019-04-06 04:09:16 UTC) multi-call binary. \
>> >>>>> Usage: sort [-nrugMcszbdfimSTokt] [-o FILE] [-k \
>> >>>>> start[.offset][opts][,end[.offset][opts]] [-t CHAR] [FILE]...
>> >>>>>
>> >>>>> Lets modify command a little to avoid -V option.
>> >>>>>
>> >>>>> Fixes: dadb81d0afe732 ("libbpf: make libbpf.map source of truth for libbpf version")
>> >>>>>
>> >>>>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
>> >>>>> ---
>> >>>>>
>> >>>>> Based on bpf/master
>> >>>>>
>> >>>>> tools/lib/bpf/Makefile | 2 +-
>> >>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> >>>>>
>> >>>>> diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile
>> >>>>> index c6f94cffe06e..a12490ad6215 100644
>> >>>>> --- a/tools/lib/bpf/Makefile
>> >>>>> +++ b/tools/lib/bpf/Makefile
>> >>>>> @@ -3,7 +3,7 @@
>> >>>>>
>> >>>>> LIBBPF_VERSION := $(shell \
>> >>>>> grep -oE '^LIBBPF_([0-9.]+)' libbpf.map | \
>> >>>>> - sort -rV | head -n1 | cut -d'_' -f2)
>> >>>>> + cut -d'_' -f2 | sort -r | head -n1)
>> >>>>
>> >>>>You can't just sort alphabetically, because:
>> >>>>
>> >>>>1.2
>> >>>>1.11
>> >>>>
>> >>>>should be in that order. See discussion on mailing thread for original commit.
>> >>>
>> >>>if X1.X2.X3, where X = {0,1,....99999}
>> >>>Then it can be:
>> >>>
>> >>>-LIBBPF_VERSION := $(shell \
>> >>>- grep -oE '^LIBBPF_([0-9.]+)' libbpf.map | \
>> >>>- sort -rV | head -n1 | cut -d'_' -f2)
>> >>>+_LBPFLIST := $(patsubst %;,%,$(patsubst LIBBPF_%,%,$(filter LIBBPF_%, \
>> >>>+ $(shell cat libbpf.map))))
>> >>>+_LBPFLIST2 := $(foreach v,$(_LBPFLIST), \
>> >>>+ $(subst $() $(),,$(foreach n,$(subst .,$() $(),$(v)), \
>> >>>+ $(shell printf "%05d" $(n)))))
>> >>>+_LBPF_VER := $(word $(words $(sort $(_LBPFLIST2))), $(sort $(_LBPFLIST2)))
>> >>>+LIBBPF_VERSION := $(patsubst %_$(_LBPF_VER),%,$(filter %_$(_LBPF_VER), \
>> >>>+ $(join $(addsuffix _, $(_LBPFLIST)),$(_LBPFLIST2))))
>> >>>
>> >>>It's bigger but avoids invocations of grep/sort/cut/head, only cat/printf
>> >>>, thus -V option also.
>> >>>
>> >>
>> >>No way, this is way too ugly (and still unreliable, if we ever have
>> >>X.Y.Z.W or something). I'd rather go with my original approach of
>> >Yes, forgot to add
>> >X1,X2,X3,...XN, where X = {0,1,....99999} and N = const for all versions.
>> >But frankly, 1.0.0 looks too far.
>>
>> It actually works for any numbs of X1.X2...X100
>> but not when you have couple kindof:
>> X1.X2.X3
>> and
>> X1.X2.X3.X4
>>
>> But, no absolutely any problem to extend this solution to handle all cases,
>> by just adding leading 0 to every "transformed version", say limit it to 10
>> possible 'dots' (%5*10d) and it will work as clocks. Advantage - mostly make
>> functions.
>>
>> Here can be couple more solutions with sed, not sure it can look less maniac.
>>
>> >
>> >>fetching the last version in libbpf.map file. See
>> >>https://www.spinics.net/lists/netdev/msg592703.html.
>>
>> Yes it's nice but, no sort, no X1.X2.X3....XN
>>
>> Main is to solve it for a long time.
>
>Thinking a bit more about this, I'm even more convinced that we should
>just go with my original approach: find last section in libbpf.map and
>extract LIBBPF version from that. That will handle whatever crazy
>version format we might decide to use (e.g., 1.2.3-experimental).
>We'll just need to make sure that latest version is the last in
>libbpf.map, which will just happen naturally. So instead of this
>Makefile complexity, please can you port back my original approach?
>Thanks!
I don't insist, placed it for history and to show it can be sorted
alphabetically, I can live with cross-compilation that I hope goes soon,
on host no need to worry about this at all. So I better leave this change
up to you.
--
Regards,
Ivan Khoronzhuk
^ permalink raw reply
* Re: CONFIG_NET_TC_SKB_EXT
From: Daniel Borkmann @ 2019-09-20 22:15 UTC (permalink / raw)
To: Jakub Kicinski, Vlad Buslov
Cc: David Miller, Paul Blakey, netdev@vger.kernel.org, Jiri Pirko,
Cong Wang, Jamal Hadi Salim, Pravin Shelar, Simon Horman,
Alexei Starovoitov, Or Gerlitz
In-Reply-To: <20190920091647.0129e65f@cakuba.netronome.com>
On 9/20/19 6:16 PM, Jakub Kicinski wrote:
> On Thu, 19 Sep 2019 15:13:55 +0000, Vlad Buslov wrote:
>> On Thu 19 Sep 2019 at 14:21, David Miller <davem@davemloft.net> wrote:
>>> As Linus pointed out, the Kconfig logic for CONFIG_NET_TC_SKB_EXT
>>> is really not acceptable.
>>>
>>> It should not be enabled by default at all.
>>>
>>> Instead the actual users should turn it on or depend upon it, which in
>>> this case seems to be OVS.
>>>
>>> Please fix this, thank you.
>>
>> Hi David,
>>
>> We are working on it, but Paul is OoO today. Is it okay if we send the
>> fix early next week?
>
> Doesn't really seem like we have too many ways forward here, right?
>
> How about this?
>
> ------>8-----------------------------------
>
> net: hide NET_TC_SKB_EXT as a config option
>
> Linus points out the NET_TC_SKB_EXT config option looks suspicious.
> Indeed, it should really be selected to ensure correct OvS operation
> if TC offload is used. Hopefully those who care about TC-only and
> OvS-only performance disable the other one at compilation time.
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> ---
> net/openvswitch/Kconfig | 1 +
> net/sched/Kconfig | 13 +++----------
> 2 files changed, 4 insertions(+), 10 deletions(-)
>
> diff --git a/net/openvswitch/Kconfig b/net/openvswitch/Kconfig
> index 22d7d5604b4c..bd407ea7c263 100644
> --- a/net/openvswitch/Kconfig
> +++ b/net/openvswitch/Kconfig
> @@ -15,6 +15,7 @@ config OPENVSWITCH
> select NET_MPLS_GSO
> select DST_CACHE
> select NET_NSH
> + select NET_TC_SKB_EXT if NET_CLS_ACT
> ---help---
> Open vSwitch is a multilayer Ethernet switch targeted at virtualized
> environments. In addition to supporting a variety of features
> diff --git a/net/sched/Kconfig b/net/sched/Kconfig
> index b3faafeafab9..f1062ef55098 100644
> --- a/net/sched/Kconfig
> +++ b/net/sched/Kconfig
> @@ -719,6 +719,7 @@ config NET_EMATCH_IPT
> config NET_CLS_ACT
> bool "Actions"
> select NET_CLS
> + select NET_TC_SKB_EXT if OPENVSWITCH
But how would that make much of a difference :( Distros are still going to
enable all of this blindlessly. Given discussion in [0], could we just get
rid of this tasteless hack altogether which is for such a narrow use case
anyway?
I thought idea of stuffing things into skb extensions are only justified if
it's not enabled by default for everyone. :(
[0] https://lore.kernel.org/netdev/CAHC9VhSz1_KA1tCJtNjwK26BOkGhKGbPT7v1O82mWPduvWwd4A@mail.gmail.com/T/#u
> ---help---
> Say Y here if you want to use traffic control actions. Actions
> get attached to classifiers and are invoked after a successful
> @@ -964,18 +965,10 @@ config NET_IFE_SKBTCINDEX
> depends on NET_ACT_IFE
>
> config NET_TC_SKB_EXT
> - bool "TC recirculation support"
> - depends on NET_CLS_ACT
> - default y if NET_CLS_ACT
> + bool
> + depends on NET_CLS_ACT && OPENVSWITCH
> select SKB_EXTENSIONS
>
> - help
> - Say Y here to allow tc chain misses to continue in OvS datapath in
> - the correct recirc_id, and hardware chain misses to continue in
> - the correct chain in tc software datapath.
> -
> - Say N here if you won't be using tc<->ovs offload or tc chains offload.
> -
> endif # NET_SCHED
>
> config NET_SCH_FIFO
>
^ permalink raw reply
* Re: CONFIG_NET_TC_SKB_EXT
From: Jakub Kicinski @ 2019-09-20 22:56 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Vlad Buslov, David Miller, Paul Blakey, netdev@vger.kernel.org,
Jiri Pirko, Cong Wang, Jamal Hadi Salim, Pravin Shelar,
Simon Horman, Alexei Starovoitov, Or Gerlitz
In-Reply-To: <0e9a1701-356f-5f94-b88e-a39175dee77a@iogearbox.net>
On Sat, 21 Sep 2019 00:15:16 +0200, Daniel Borkmann wrote:
> On 9/20/19 6:16 PM, Jakub Kicinski wrote:
> > On Thu, 19 Sep 2019 15:13:55 +0000, Vlad Buslov wrote:
> >> On Thu 19 Sep 2019 at 14:21, David Miller <davem@davemloft.net> wrote:
> >>> As Linus pointed out, the Kconfig logic for CONFIG_NET_TC_SKB_EXT
> >>> is really not acceptable.
> >>>
> >>> It should not be enabled by default at all.
> >>>
> >>> Instead the actual users should turn it on or depend upon it, which in
> >>> this case seems to be OVS.
> >>>
> >>> Please fix this, thank you.
> >>
> >> Hi David,
> >>
> >> We are working on it, but Paul is OoO today. Is it okay if we send the
> >> fix early next week?
> >
> > Doesn't really seem like we have too many ways forward here, right?
> >
> > How about this?
> >
> > ------>8-----------------------------------
> >
> > net: hide NET_TC_SKB_EXT as a config option
> >
> > Linus points out the NET_TC_SKB_EXT config option looks suspicious.
> > Indeed, it should really be selected to ensure correct OvS operation
> > if TC offload is used. Hopefully those who care about TC-only and
> > OvS-only performance disable the other one at compilation time.
> >
> > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> > ---
> > net/openvswitch/Kconfig | 1 +
> > net/sched/Kconfig | 13 +++----------
> > 2 files changed, 4 insertions(+), 10 deletions(-)
> >
> > diff --git a/net/openvswitch/Kconfig b/net/openvswitch/Kconfig
> > index 22d7d5604b4c..bd407ea7c263 100644
> > --- a/net/openvswitch/Kconfig
> > +++ b/net/openvswitch/Kconfig
> > @@ -15,6 +15,7 @@ config OPENVSWITCH
> > select NET_MPLS_GSO
> > select DST_CACHE
> > select NET_NSH
> > + select NET_TC_SKB_EXT if NET_CLS_ACT
> > ---help---
> > Open vSwitch is a multilayer Ethernet switch targeted at virtualized
> > environments. In addition to supporting a variety of features
> > diff --git a/net/sched/Kconfig b/net/sched/Kconfig
> > index b3faafeafab9..f1062ef55098 100644
> > --- a/net/sched/Kconfig
> > +++ b/net/sched/Kconfig
> > @@ -719,6 +719,7 @@ config NET_EMATCH_IPT
> > config NET_CLS_ACT
> > bool "Actions"
> > select NET_CLS
> > + select NET_TC_SKB_EXT if OPENVSWITCH
>
> But how would that make much of a difference :( Distros are still going to
> enable all of this blindlessly. Given discussion in [0], could we just get
> rid of this tasteless hack altogether which is for such a narrow use case
> anyway?
Agreed. I take it you're opposed to the use of skb extensions here
in general? Distros would have enabled NET_TC_SKB_EXT even when it
was a config option.
> I thought idea of stuffing things into skb extensions are only justified if
> it's not enabled by default for everyone. :(
>
> [0] https://lore.kernel.org/netdev/CAHC9VhSz1_KA1tCJtNjwK26BOkGhKGbPT7v1O82mWPduvWwd4A@mail.gmail.com/T/#u
The skb ext allocation is only done with GOTO_CHAIN, which AFAIK only
has practical use for offload. We could perhaps add another static
branch there or move the OvS static branch out of the OvS module so
there are no linking issues?
I personally have little sympathy for this piece of code, it is perhaps
the purest form of a wobbly narrow-use construct pushed into TC for HW
offload.
Any suggestions on the way forward? :(
^ permalink raw reply
* Re: [PATCH net v3 09/11] net: core: add ignore flag to netdev_adjacent structure
From: Jakub Kicinski @ 2019-09-20 23:24 UTC (permalink / raw)
To: Taehee Yoo
Cc: davem, netdev, j.vosburgh, vfalico, andy, jiri, sd, roopa, saeedm,
manishc, rahulv, kys, haiyangz, stephen, sashal, hare, varun,
ubraun, kgraul, jay.vosburgh
In-Reply-To: <20190916134802.8252-10-ap420073@gmail.com>
On Mon, 16 Sep 2019 22:48:00 +0900, Taehee Yoo wrote:
> In order to link an adjacent node, netdev_upper_dev_link() is used
> and in order to unlink an adjacent node, netdev_upper_dev_unlink() is used.
> unlink operation does not fail, but link operation can fail.
>
> In order to exchange adjacent nodes, we should unlink an old adjacent
> node first. then, link a new adjacent node.
> If link operation is failed, we should link an old adjacent node again.
> But this link operation can fail too.
> It eventually breaks the adjacent link relationship.
>
> This patch adds an ignore flag into the netdev_adjacent structure.
> If this flag is set, netdev_upper_dev_link() ignores an old adjacent
> node for a moment.
> So we can skip unlink operation before link operation.
>
> Signed-off-by: Taehee Yoo <ap420073@gmail.com>
I think there's a number of local functions here which are missing the
static keyword, we'll get a flurry of fixes as soon as we apply this.
Could you please take a look at fixing that, I think W=1 should help,
-Wmissing-prototypes in particular, if I'm remembering right.
^ permalink raw reply
* [PATCH bpf] selftests/bpf: test_progs: fix client/server race in tcp_rtt
From: Stanislav Fomichev @ 2019-09-20 23:30 UTC (permalink / raw)
To: netdev, bpf; +Cc: davem, ast, daniel, Stanislav Fomichev
This is the same problem I found earlier in test_sockopt_inherit:
there is a race between server thread doing accept() and client
thread doing connect(). Let's explicitly synchronize them via
pthread conditional variable.
Fixes: b55873984dab ("selftests/bpf: test BPF_SOCK_OPS_RTT_CB")
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
tools/testing/selftests/bpf/prog_tests/tcp_rtt.c | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/tcp_rtt.c b/tools/testing/selftests/bpf/prog_tests/tcp_rtt.c
index fdc0b3614a9e..e64058906bcd 100644
--- a/tools/testing/selftests/bpf/prog_tests/tcp_rtt.c
+++ b/tools/testing/selftests/bpf/prog_tests/tcp_rtt.c
@@ -203,6 +203,9 @@ static int start_server(void)
return fd;
}
+static pthread_mutex_t server_started_mtx = PTHREAD_MUTEX_INITIALIZER;
+static pthread_cond_t server_started = PTHREAD_COND_INITIALIZER;
+
static void *server_thread(void *arg)
{
struct sockaddr_storage addr;
@@ -215,6 +218,10 @@ static void *server_thread(void *arg)
return NULL;
}
+ pthread_mutex_lock(&server_started_mtx);
+ pthread_cond_signal(&server_started);
+ pthread_mutex_unlock(&server_started_mtx);
+
client_fd = accept(fd, (struct sockaddr *)&addr, &len);
if (CHECK_FAIL(client_fd < 0)) {
perror("Failed to accept client");
@@ -248,7 +255,14 @@ void test_tcp_rtt(void)
if (CHECK_FAIL(server_fd < 0))
goto close_cgroup_fd;
- pthread_create(&tid, NULL, server_thread, (void *)&server_fd);
+ if (CHECK_FAIL(pthread_create(&tid, NULL, server_thread,
+ (void *)&server_fd)))
+ goto close_cgroup_fd;
+
+ pthread_mutex_lock(&server_started_mtx);
+ pthread_cond_wait(&server_started, &server_started_mtx);
+ pthread_mutex_unlock(&server_started_mtx);
+
CHECK_FAIL(run_test(cgroup_fd, server_fd));
close(server_fd);
close_cgroup_fd:
--
2.23.0.351.gc4317032e6-goog
^ permalink raw reply related
* Re: CONFIG_NET_TC_SKB_EXT
From: Daniel Borkmann @ 2019-09-20 23:40 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Vlad Buslov, David Miller, Paul Blakey, netdev@vger.kernel.org,
Jiri Pirko, Cong Wang, Jamal Hadi Salim, Pravin Shelar,
Simon Horman, Alexei Starovoitov, Or Gerlitz
In-Reply-To: <20190920155605.7c81c2af@cakuba.netronome.com>
On 9/21/19 12:56 AM, Jakub Kicinski wrote:
> On Sat, 21 Sep 2019 00:15:16 +0200, Daniel Borkmann wrote:
>> On 9/20/19 6:16 PM, Jakub Kicinski wrote:
>>> On Thu, 19 Sep 2019 15:13:55 +0000, Vlad Buslov wrote:
>>>> On Thu 19 Sep 2019 at 14:21, David Miller <davem@davemloft.net> wrote:
>>>>> As Linus pointed out, the Kconfig logic for CONFIG_NET_TC_SKB_EXT
>>>>> is really not acceptable.
>>>>>
>>>>> It should not be enabled by default at all.
>>>>>
>>>>> Instead the actual users should turn it on or depend upon it, which in
>>>>> this case seems to be OVS.
>>>>>
>>>>> Please fix this, thank you.
>>>>
>>>> Hi David,
>>>>
>>>> We are working on it, but Paul is OoO today. Is it okay if we send the
>>>> fix early next week?
>>>
>>> Doesn't really seem like we have too many ways forward here, right?
>>>
>>> How about this?
>>>
>>> ------>8-----------------------------------
>>>
>>> net: hide NET_TC_SKB_EXT as a config option
>>>
>>> Linus points out the NET_TC_SKB_EXT config option looks suspicious.
>>> Indeed, it should really be selected to ensure correct OvS operation
>>> if TC offload is used. Hopefully those who care about TC-only and
>>> OvS-only performance disable the other one at compilation time.
>>>
>>> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>>> ---
>>> net/openvswitch/Kconfig | 1 +
>>> net/sched/Kconfig | 13 +++----------
>>> 2 files changed, 4 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/net/openvswitch/Kconfig b/net/openvswitch/Kconfig
>>> index 22d7d5604b4c..bd407ea7c263 100644
>>> --- a/net/openvswitch/Kconfig
>>> +++ b/net/openvswitch/Kconfig
>>> @@ -15,6 +15,7 @@ config OPENVSWITCH
>>> select NET_MPLS_GSO
>>> select DST_CACHE
>>> select NET_NSH
>>> + select NET_TC_SKB_EXT if NET_CLS_ACT
>>> ---help---
>>> Open vSwitch is a multilayer Ethernet switch targeted at virtualized
>>> environments. In addition to supporting a variety of features
>>> diff --git a/net/sched/Kconfig b/net/sched/Kconfig
>>> index b3faafeafab9..f1062ef55098 100644
>>> --- a/net/sched/Kconfig
>>> +++ b/net/sched/Kconfig
>>> @@ -719,6 +719,7 @@ config NET_EMATCH_IPT
>>> config NET_CLS_ACT
>>> bool "Actions"
>>> select NET_CLS
>>> + select NET_TC_SKB_EXT if OPENVSWITCH
>>
>> But how would that make much of a difference :( Distros are still going to
>> enable all of this blindlessly. Given discussion in [0], could we just get
>> rid of this tasteless hack altogether which is for such a narrow use case
>> anyway?
>
> Agreed. I take it you're opposed to the use of skb extensions here
> in general? Distros would have enabled NET_TC_SKB_EXT even when it
> was a config option.
Yeah, as it stands, motivation for this extension is to tie tc [HW] offload
and OVS to work together in case of recirculation ... from commit:
Received packets will first travel though tc, and if they aren't stolen
by it, like in the above rule, they will continue to OvS datapath.
Since we already did some actions (action ct in this case) which might
modify the packets, and updated action stats, we would like to continue
the proccessing with the correct recirc_id in OvS (here recirc_id(2))
where we left off.
I would perhaps see more of a point in an skb extension if there is rather
generic use and also out of the SW datapath (and I really doubt anyone is
seriously using tc + OVS combo for non-HW workloads). Right now this feels
like duck taping.
Adding new skb extensions should really have a strong justification behind
it (close but slightly less strict to how we treat adding new members to
skb itself).
^ permalink raw reply
* Verify ACK packets in handshake in kernel module (Access TCP state table)
From: Swarm @ 2019-09-20 23:43 UTC (permalink / raw)
To: netdev
First time emailing to this mailing list so please let me know if I made a mistake in how I sent it. I'm trying to receive a notification from the kernel once it verifies an ACK packet in a handshake. Problem is, there is no API or kernel resource I've seen that supports this feature for both syncookies and normal handshakes. Where exactly in the kernel does the ACK get verified? If there isn't a way to be notified of it, where should I start adding that feature into the kernel?
^ permalink raw reply
* Re: [PATCH net v3 10/11] vxlan: add adjacent link to limit depth level
From: Jakub Kicinski @ 2019-09-20 23:48 UTC (permalink / raw)
To: Taehee Yoo
Cc: davem, netdev, j.vosburgh, vfalico, andy, jiri, sd, roopa, saeedm,
manishc, rahulv, kys, haiyangz, stephen, sashal, hare, varun,
ubraun, kgraul, jay.vosburgh
In-Reply-To: <20190916134802.8252-11-ap420073@gmail.com>
On Mon, 16 Sep 2019 22:48:01 +0900, Taehee Yoo wrote:
> Current vxlan code doesn't limit the number of nested devices.
> Nested devices would be handled recursively and this routine needs
> huge stack memory. So, unlimited nested devices could make
> stack overflow.
>
> In order to fix this issue, this patch adds adjacent links.
> The adjacent link APIs internally check the depth level.
> Fixes: acaf4e70997f ("net: vxlan: when lower dev unregisters remove vxlan dev as well")
> Signed-off-by: Taehee Yoo <ap420073@gmail.com>
Minor nit picks here, I hope you don't mind.
> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
> index 3d9bcc957f7d..0d5c8d22d8a4 100644
> --- a/drivers/net/vxlan.c
> +++ b/drivers/net/vxlan.c
> @@ -3567,6 +3567,8 @@ static int __vxlan_dev_create(struct net *net, struct net_device *dev,
> struct vxlan_net *vn = net_generic(net, vxlan_net_id);
> struct vxlan_dev *vxlan = netdev_priv(dev);
> struct vxlan_fdb *f = NULL;
> + struct net_device *remote_dev = NULL;
> + struct vxlan_rdst *dst = &vxlan->default_dst;
Especially in places where reverse christmas tree variable ordering is
adhered to, could you please preserve it? That'd mean:
struct vxlan_net *vn = net_generic(net, vxlan_net_id);
struct vxlan_dev *vxlan = netdev_priv(dev);
struct net_device *remote_dev = NULL;
struct vxlan_fdb *f = NULL;
bool unregister = false;
struct vxlan_rdst *dst;
int err;
dst = &vxlan->default_dst;
here.
> bool unregister = false;
> int err;
>
> @@ -3577,14 +3579,14 @@ static int __vxlan_dev_create(struct net *net, struct net_device *dev,
> dev->ethtool_ops = &vxlan_ethtool_ops;
>
> /* create an fdb entry for a valid default destination */
> - if (!vxlan_addr_any(&vxlan->default_dst.remote_ip)) {
> + if (!vxlan_addr_any(&dst->remote_ip)) {
> err = vxlan_fdb_create(vxlan, all_zeros_mac,
> - &vxlan->default_dst.remote_ip,
> + &dst->remote_ip,
> NUD_REACHABLE | NUD_PERMANENT,
> vxlan->cfg.dst_port,
> - vxlan->default_dst.remote_vni,
> - vxlan->default_dst.remote_vni,
> - vxlan->default_dst.remote_ifindex,
> + dst->remote_vni,
> + dst->remote_vni,
> + dst->remote_ifindex,
> NTF_SELF, &f);
> if (err)
> return err;
> @@ -3595,26 +3597,43 @@ static int __vxlan_dev_create(struct net *net, struct net_device *dev,
> goto errout;
> unregister = true;
>
> + if (dst->remote_ifindex) {
> + remote_dev = __dev_get_by_index(net, dst->remote_ifindex);
> + if (!remote_dev)
> + goto errout;
> +
> + err = netdev_upper_dev_link(remote_dev, dev, extack);
> + if (err)
> + goto errout;
> + }
> +
> err = rtnl_configure_link(dev, NULL);
> if (err)
> - goto errout;
> + goto unlink;
>
> if (f) {
> - vxlan_fdb_insert(vxlan, all_zeros_mac,
> - vxlan->default_dst.remote_vni, f);
> + vxlan_fdb_insert(vxlan, all_zeros_mac, dst->remote_vni, f);
>
> /* notify default fdb entry */
> err = vxlan_fdb_notify(vxlan, f, first_remote_rtnl(f),
> RTM_NEWNEIGH, true, extack);
> if (err) {
> vxlan_fdb_destroy(vxlan, f, false, false);
> + if (remote_dev)
> + netdev_upper_dev_unlink(remote_dev, dev);
> goto unregister;
> }
> }
>
> list_add(&vxlan->next, &vn->vxlan_list);
> + if (remote_dev) {
> + dst->remote_dev = remote_dev;
> + dev_hold(remote_dev);
> + }
> return 0;
> -
> +unlink:
> + if (remote_dev)
> + netdev_upper_dev_unlink(remote_dev, dev);
> errout:
> /* unregister_netdevice() destroys the default FDB entry with deletion
> * notification. But the addition notification was not sent yet, so
> @@ -3936,6 +3955,8 @@ static int vxlan_changelink(struct net_device *dev, struct nlattr *tb[],
> struct net_device *lowerdev;
> struct vxlan_config conf;
> int err;
> + bool linked = false;
> + bool disabled = false;
Same here.
> err = vxlan_nl2conf(tb, data, dev, &conf, true, extack);
> if (err)
> @@ -3946,6 +3967,16 @@ static int vxlan_changelink(struct net_device *dev, struct nlattr *tb[],
> if (err)
> return err;
>
> + if (lowerdev) {
> + if (dst->remote_dev && lowerdev != dst->remote_dev) {
> + netdev_adjacent_dev_disable(dst->remote_dev, dev);
> + disabled = true;
> + }
> + err = netdev_upper_dev_link(lowerdev, dev, extack);
> + if (err)
> + goto err;
would you mind naming the label errout? there is an err variable, and
other places in this file use errout
> + linked = true;
> + }
> /* handle default dst entry */
> if (!vxlan_addr_equal(&conf.remote_ip, &dst->remote_ip)) {
> u32 hash_index = fdb_head_index(vxlan, all_zeros_mac, conf.vni);
> @@ -3962,7 +3993,7 @@ static int vxlan_changelink(struct net_device *dev, struct nlattr *tb[],
> NTF_SELF, true, extack);
> if (err) {
> spin_unlock_bh(&vxlan->hash_lock[hash_index]);
> - return err;
> + goto err;
> }
> }
> if (!vxlan_addr_any(&dst->remote_ip))
> @@ -3979,8 +4010,24 @@ static int vxlan_changelink(struct net_device *dev, struct nlattr *tb[],
> if (conf.age_interval != vxlan->cfg.age_interval)
> mod_timer(&vxlan->age_timer, jiffies);
>
> + if (disabled) {
> + netdev_adjacent_dev_enable(dst->remote_dev, dev);
> + netdev_upper_dev_unlink(dst->remote_dev, dev);
> + dev_put(dst->remote_dev);
> + }
> + if (linked) {
> + dst->remote_dev = lowerdev;
> + dev_hold(dst->remote_dev);
> + }
> +
> vxlan_config_apply(dev, &conf, lowerdev, vxlan->net, true);
> return 0;
> +err:
> + if (linked)
> + netdev_upper_dev_unlink(lowerdev, dev);
> + if (disabled)
> + netdev_adjacent_dev_enable(dst->remote_dev, dev);
> + return err;
> }
>
> static void vxlan_dellink(struct net_device *dev, struct list_head *head)
> @@ -3991,6 +4038,10 @@ static void vxlan_dellink(struct net_device *dev, struct list_head *head)
>
> list_del(&vxlan->next);
> unregister_netdevice_queue(dev, head);
> + if (vxlan->default_dst.remote_dev) {
> + netdev_upper_dev_unlink(vxlan->default_dst.remote_dev, dev);
> + dev_put(vxlan->default_dst.remote_dev);
> + }
> }
>
> static size_t vxlan_get_size(const struct net_device *dev)
> diff --git a/include/net/vxlan.h b/include/net/vxlan.h
> index dc1583a1fb8a..08e237d7aa73 100644
> --- a/include/net/vxlan.h
> +++ b/include/net/vxlan.h
> @@ -197,6 +197,7 @@ struct vxlan_rdst {
> u8 offloaded:1;
> __be32 remote_vni;
> u32 remote_ifindex;
> + struct net_device *remote_dev;
> struct list_head list;
> struct rcu_head rcu;
> struct dst_cache dst_cache;
^ permalink raw reply
* Re: [PATCH net v3 09/11] net: core: add ignore flag to netdev_adjacent structure
From: Jakub Kicinski @ 2019-09-20 23:55 UTC (permalink / raw)
To: Taehee Yoo
Cc: davem, netdev, j.vosburgh, vfalico, andy, jiri, sd, roopa, saeedm,
manishc, rahulv, kys, haiyangz, stephen, sashal, hare, varun,
ubraun, kgraul, jay.vosburgh
In-Reply-To: <20190916134802.8252-10-ap420073@gmail.com>
On Mon, 16 Sep 2019 22:48:00 +0900, Taehee Yoo wrote:
> In order to link an adjacent node, netdev_upper_dev_link() is used
> and in order to unlink an adjacent node, netdev_upper_dev_unlink() is used.
> unlink operation does not fail, but link operation can fail.
>
> In order to exchange adjacent nodes, we should unlink an old adjacent
> node first. then, link a new adjacent node.
> If link operation is failed, we should link an old adjacent node again.
> But this link operation can fail too.
> It eventually breaks the adjacent link relationship.
>
> This patch adds an ignore flag into the netdev_adjacent structure.
> If this flag is set, netdev_upper_dev_link() ignores an old adjacent
> node for a moment.
> So we can skip unlink operation before link operation.
>
> Signed-off-by: Taehee Yoo <ap420073@gmail.com>
Could this perhaps be achieved by creating prepare, commit, and abort
helpers? That would make the API look slightly more canonical.
netdev_adjacent_change_prepare(old, new, dev)
netdev_adjacent_change_commit(old, new, dev)
netdev_adjacent_change_abort(old, new, dev)
The current naming makes the operation a little harder to follow if one
is just reading the vxlan code.
Please let me know if I didn't read the code closely enough to
understand why that's not fitting here.
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 5bb5756129af..4506810c301b 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -4319,6 +4319,10 @@ int netdev_master_upper_dev_link(struct net_device *dev,
> struct netlink_ext_ack *extack);
> void netdev_upper_dev_unlink(struct net_device *dev,
> struct net_device *upper_dev);
> +void netdev_adjacent_dev_disable(struct net_device *upper_dev,
> + struct net_device *lower_dev);
> +void netdev_adjacent_dev_enable(struct net_device *upper_dev,
> + struct net_device *lower_dev);
> void netdev_adjacent_rename_links(struct net_device *dev, char *oldname);
> void *netdev_lower_dev_get_private(struct net_device *dev,
> struct net_device *lower_dev);
^ permalink raw reply
* Re: [PATCH net v3 00/11] net: fix nested device bugs
From: Jakub Kicinski @ 2019-09-20 23:59 UTC (permalink / raw)
To: Taehee Yoo
Cc: davem, netdev, j.vosburgh, vfalico, andy, jiri, sd, roopa, saeedm,
manishc, rahulv, kys, haiyangz, stephen, sashal, hare, varun,
ubraun, kgraul, jay.vosburgh
In-Reply-To: <20190916134802.8252-1-ap420073@gmail.com>
On Mon, 16 Sep 2019 22:47:51 +0900, Taehee Yoo wrote:
> This patchset fixes several bugs that are related to nesting
> device infrastructure.
> Current nesting infrastructure code doesn't limit the depth level of
> devices. nested devices could be handled recursively. at that moment,
> it needs huge memory and stack overflow could occur.
> Below devices type have same bug.
> VLAN, BONDING, TEAM, MACSEC, MACVLAN and VXLAN.
Is this list exhaustive? Looks like qmi_wwan.c perhaps could also have
similar problem? Or virt_wifi? Perhaps worth CCing the authors of those
drivers and mentioning that?
Thank you for working on this!
^ permalink raw reply
* Re: CONFIG_NET_TC_SKB_EXT
From: Daniel Borkmann @ 2019-09-21 0:06 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Vlad Buslov, David Miller, Paul Blakey, netdev@vger.kernel.org,
Jiri Pirko, Cong Wang, Jamal Hadi Salim, Pravin Shelar,
Simon Horman, Alexei Starovoitov, Or Gerlitz
In-Reply-To: <20190920155605.7c81c2af@cakuba.netronome.com>
On 9/21/19 12:56 AM, Jakub Kicinski wrote:
[...]
>> I thought idea of stuffing things into skb extensions are only justified if
>> it's not enabled by default for everyone. :(
>>
>> [0] https://lore.kernel.org/netdev/CAHC9VhSz1_KA1tCJtNjwK26BOkGhKGbPT7v1O82mWPduvWwd4A@mail.gmail.com/T/#u
>
> The skb ext allocation is only done with GOTO_CHAIN, which AFAIK only
> has practical use for offload. We could perhaps add another static
> branch there or move the OvS static branch out of the OvS module so
> there are no linking issues?
>
> I personally have little sympathy for this piece of code, it is perhaps
> the purest form of a wobbly narrow-use construct pushed into TC for HW
> offload.
>
> Any suggestions on the way forward? :(
Presumably there are no clean solutions here, but on the top of my head for
this use case, you'd need to /own/ the underlying datapath anyway, so couldn't
you program the OVS key->recirc_id based on skb->mark (or alternatively via
skb->tc_index) which was previously set by tc ingress?
Thanks,
Daniel
^ permalink raw reply
* Re: [PATCH net] net/rds: Check laddr_check before calling it
From: Jakub Kicinski @ 2019-09-21 1:09 UTC (permalink / raw)
To: Ka-Cheong Poon; +Cc: netdev, santosh.shilimkar, davem, rds-devel
In-Reply-To: <1568734158-18021-1-git-send-email-ka-cheong.poon@oracle.com>
On Tue, 17 Sep 2019 08:29:18 -0700, Ka-Cheong Poon wrote:
> In rds_bind(), laddr_check is called without checking if it is NULL or
> not. And rs_transport should be reset if rds_add_bound() fails.
>
> Reported-by: syzbot+fae39afd2101a17ec624@syzkaller.appspotmail.com
> Signed-off-by: Ka-Cheong Poon <ka-cheong.poon@oracle.com>
Looks good, but could you please provide a fixes tag?
^ permalink raw reply
* Re: [PATCH] dt-bindings: net: dwmac: fix 'mac-mode' type
From: Jakub Kicinski @ 2019-09-21 1:11 UTC (permalink / raw)
To: Alexandru Ardelean
Cc: netdev, devicetree, linux-kernel, davem, robh+dt, peppe.cavallaro,
alexandre.torgue, andrew, f.fainelli
In-Reply-To: <20190917103052.13456-1-alexandru.ardelean@analog.com>
On Tue, 17 Sep 2019 13:30:52 +0300, Alexandru Ardelean wrote:
> The 'mac-mode' property is similar to 'phy-mode' and 'phy-connection-type',
> which are enums of mode strings.
>
> The 'dwmac' driver supports almost all modes declared in the 'phy-mode'
> enum (except for 1 or 2). But in general, there may be a case where
> 'mac-mode' becomes more generic and is moved as part of phylib or netdev.
>
> In any case, the 'mac-mode' field should be made an enum, and it also makes
> sense to just reference the 'phy-connection-type' from
> 'ethernet-controller.yaml'. That will also make it more future-proof for new
> modes.
>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Applied, thank you!
FWIW I had to add the Fixes tag by hand, either ozlabs patchwork or my
git-pw doesn't have the automagic handling there, yet.
^ permalink raw reply
* Re: [PATCH net] net/sched: act_sample: don't push mac header on ip6gre ingress
From: Jakub Kicinski @ 2019-09-21 1:12 UTC (permalink / raw)
To: Davide Caratti
Cc: Jiri Pirko, Cong Wang, Jamal Hadi Salim, David S. Miller, netdev,
Yotam Gigi
In-Reply-To: <c359067b4a84342ff24c6a3d089171de68489fcd.1568709449.git.dcaratti@redhat.com>
On Tue, 17 Sep 2019 11:30:55 +0200, Davide Caratti wrote:
> current 'sample' action doesn't push the mac header of ingress packets if
> they are received by a layer 3 tunnel (like gre or sit); but it forgot to
> check for gre over ipv6, so the following script:
>
> # tc q a dev $d clsact
> # tc f a dev $d ingress protocol ip flower ip_proto icmp action sample \
> > group 100 rate 1
> # psample -v -g 100
>
> dumps everything, including outer header and mac, when $d is a gre tunnel
> over ipv6. Fix this adding a missing label for ARPHRD_IP6GRE devices.
>
> Fixes: 5c5670fae430 ("net/sched: Introduce sample tc action")
> Signed-off-by: Davide Caratti <dcaratti@redhat.com>
Applied, queued for stable, thank you!
^ permalink raw reply
* Re: [PATCH net] selftests: Update fib_tests to handle missing ping6
From: Jakub Kicinski @ 2019-09-21 1:17 UTC (permalink / raw)
To: David Ahern; +Cc: davem, netdev, David Ahern
In-Reply-To: <20190917173021.19705-1-dsahern@kernel.org>
On Tue, 17 Sep 2019 10:30:21 -0700, David Ahern wrote:
> From: David Ahern <dsahern@gmail.com>
>
> Some distributions (e.g., debian buster) do not install ping6. Re-use
> the hook in pmtu.sh to detect this and fallback to ping.
>
> Fixes: a0e11da78f48 ("fib_tests: Add tests for metrics on routes")
> Signed-off-by: David Ahern <dsahern@gmail.com>
Applied, queued, thanks!
^ permalink raw reply
* Re: [PATCH] selftests: Update fib_nexthop_multiprefix to handle missing ping6
From: Jakub Kicinski @ 2019-09-21 1:22 UTC (permalink / raw)
To: David Ahern; +Cc: davem, netdev, David Ahern
In-Reply-To: <20190917173035.19753-1-dsahern@kernel.org>
On Tue, 17 Sep 2019 10:30:35 -0700, David Ahern wrote:
> From: David Ahern <dsahern@gmail.com>
>
> Some distributions (e.g., debian buster) do not install ping6. Re-use
> the hook in pmtu.sh to detect this and fallback to ping.
>
> Fixes: 735ab2f65dce ("selftests: Add test with multiple prefixes using single nexthop")
> Signed-off-by: David Ahern <dsahern@gmail.com>
Applied, queued, thanks!
^ permalink raw reply
* Re: [PATCH net] ipv4: Revert removal of rt_uses_gateway
From: Jakub Kicinski @ 2019-09-21 1:30 UTC (permalink / raw)
To: David Ahern; +Cc: davem, netdev, ja, David Ahern
In-Reply-To: <20190917173949.19982-1-dsahern@kernel.org>
On Tue, 17 Sep 2019 10:39:49 -0700, David Ahern wrote:
> From: David Ahern <dsahern@gmail.com>
>
> Julian noted that rt_uses_gateway has a more subtle use than 'is gateway
> set':
> https://lore.kernel.org/netdev/alpine.LFD.2.21.1909151104060.2546@ja.home.ssi.bg/
>
> Revert that part of the commit referenced in the Fixes tag.
>
> Currently, there are no u8 holes in 'struct rtable'. There is a 4-byte hole
> in the second cacheline which contains the gateway declaration. So move
> rt_gw_family down to the gateway declarations since they are always used
> together, and then re-use that u8 for rt_uses_gateway. End result is that
> rtable size is unchanged.
>
> Fixes: 1550c171935d ("ipv4: Prepare rtable for IPv6 gateway")
> Reported-by: Julian Anastasov <ja@ssi.bg>
> Signed-off-by: David Ahern <dsahern@gmail.com>
I'm assuming the mix of u8 and __u8 is intentional, since this is a partial revert :)
Applied, queued, thanks!
^ permalink raw reply
* Re: [PATCH net] ipv4: Revert removal of rt_uses_gateway
From: David Ahern @ 2019-09-21 1:32 UTC (permalink / raw)
To: Jakub Kicinski, David Ahern; +Cc: davem, netdev, ja
In-Reply-To: <20190920183041.7c57b973@cakuba.netronome.com>
On 9/20/19 7:30 PM, Jakub Kicinski wrote:
> I'm assuming the mix of u8 and __u8 is intentional, since this is a partial revert :)
original patch for rt_uses_gateway used __u8 so yes put that back.
not sure why I used u8 for the new field, but left it on the move.
^ 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