* Re: [PATCH net-next 3/3] net: stmmac: Introducing support for Page Pool
From: Ilias Apalodimas @ 2019-07-04 11:11 UTC (permalink / raw)
To: Jose Abreu
Cc: Jesper Dangaard Brouer, linux-kernel@vger.kernel.org,
netdev@vger.kernel.org, linux-stm32@st-md-mailman.stormreply.com,
linux-arm-kernel@lists.infradead.org, Joao Pinto,
David S . Miller, Giuseppe Cavallaro, Alexandre Torgue,
Maxime Coquelin, Maxime Ripard, Chen-Yu Tsai
In-Reply-To: <BN8PR12MB3266BC5322AADFAC49D9BAFAD3FA0@BN8PR12MB3266.namprd12.prod.outlook.com>
On Thu, Jul 04, 2019 at 10:13:37AM +0000, Jose Abreu wrote:
> From: Jesper Dangaard Brouer <brouer@redhat.com>
>
> > The page_pool DMA mapping cannot be "kept" when page traveling into the
> > network stack attached to an SKB. (Ilias and I have a long term plan[1]
> > to allow this, but you cannot do it ATM).
>
> The reason I recycle the page is this previous call to:
>
> skb_copy_to_linear_data()
>
> So, technically, I'm syncing to CPU the page(s) and then memcpy to a
> previously allocated SKB ... So it's safe to just recycle the mapping I
> think.
>
> Its kind of using bounce buffers and I do see performance gain in this
> (I think the reason is because my setup uses swiotlb for DMA mapping).
Maybe. Have you tested this on big/small packets?
Can you do a test with 64b/128b and 1024b for example?
The memcpy might be cheap for the small sized packets (and cheaper than the dma
map/unmap)
>
> Anyway, I'm open to some suggestions on how to improve this ...
>
> > Also remember that the page_pool requires you driver to do the DMA-sync
> > operation. I see a dma_sync_single_for_cpu(), but I didn't see a
> > dma_sync_single_for_device() (well, I noticed one getting removed).
> > (For some HW Ilias tells me that the dma_sync_single_for_device can be
> > elided, so maybe this can still be correct for you).
>
> My HW just needs descriptors refilled which are in different coherent
> region so I don't see any reason for dma_sync_single_for_device() ...
I am abit overloaded at the moment. I'll try to have a look at this and get back
to you
Cheers
/Ilias
^ permalink raw reply
* [PATCH v2] rtl8xxxu: Fix wifi low signal strength issue of RTL8723BU
From: Chris Chiu @ 2019-07-04 10:55 UTC (permalink / raw)
To: jes.sorensen, kvalo, davem; +Cc: linux-wireless, netdev, linux-kernel, linux
The WiFi tx power of RTL8723BU is extremely low after booting. So
the WiFi scan gives very limited AP list and it always fails to
connect to the selected AP. This module only supports 1x1 antenna
and the antenna is switched to bluetooth due to some incorrect
register settings.
Compare with the vendor driver https://github.com/lwfinger/rtl8723bu,
we realized that the 8723bu's enable_rf() does the same thing as
rtw_btcoex_HAL_Initialize() in vendor driver. And it by default
sets the antenna path to BTC_ANT_PATH_BT which we verified it's
the cause of the wifi weak tx power. The vendor driver will set
the antenna path to BTC_ANT_PATH_PTA in the consequent btcoexist
mechanism, by the function halbtc8723b1ant_PsTdma.
This commit hand over the antenna control to PTA(Packet Traffic
Arbitration), which compares the weight of bluetooth/wifi traffic
then determine whether to continue current wifi traffic or not.
After PTA take control, The wifi signal will be back to normal and
the bluetooth scan can also work at the same time. However, the
btcoexist still needs to be handled under different circumstances.
If there's a BT connection established, the wifi still fails to
connect until BT disconnected.
Signed-off-by: Chris Chiu <chiu@endlessm.com>
---
Note:
v2:
- Replace BIT(11) with the descriptive definition
- Meaningful comment for the REG_S0S1_PATH_SWITCH setting
drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c | 11 ++++++++---
drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 3 ++-
2 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c
index 3adb1d3d47ac..ceffe05bd65b 100644
--- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c
+++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c
@@ -1525,7 +1525,7 @@ static void rtl8723b_enable_rf(struct rtl8xxxu_priv *priv)
/*
* WLAN action by PTA
*/
- rtl8xxxu_write8(priv, REG_WLAN_ACT_CONTROL_8723B, 0x04);
+ rtl8xxxu_write8(priv, REG_WLAN_ACT_CONTROL_8723B, 0x0c);
/*
* BT select S0/S1 controlled by WiFi
@@ -1568,9 +1568,14 @@ static void rtl8723b_enable_rf(struct rtl8xxxu_priv *priv)
rtl8xxxu_gen2_h2c_cmd(priv, &h2c, sizeof(h2c.ant_sel_rsv));
/*
- * 0x280, 0x00, 0x200, 0x80 - not clear
+ * Different settings per different antenna position.
+ * Antenna Position: | Normal Inverse
+ * --------------------------------------------------
+ * Antenna switch to BT: | 0x280, 0x00
+ * Antenna switch to WiFi: | 0x0, 0x280
+ * Antenna switch to PTA: | 0x200, 0x80
*/
- rtl8xxxu_write32(priv, REG_S0S1_PATH_SWITCH, 0x00);
+ rtl8xxxu_write32(priv, REG_S0S1_PATH_SWITCH, 0x80);
/*
* Software control, antenna at WiFi side
diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
index 8136e268b4e6..c6c41fb962ff 100644
--- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
+++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
@@ -3891,12 +3891,13 @@ static int rtl8xxxu_init_device(struct ieee80211_hw *hw)
/* Check if MAC is already powered on */
val8 = rtl8xxxu_read8(priv, REG_CR);
+ val16 = rtl8xxxu_read16(priv, REG_SYS_CLKR);
/*
* Fix 92DU-VC S3 hang with the reason is that secondary mac is not
* initialized. First MAC returns 0xea, second MAC returns 0x00
*/
- if (val8 == 0xea)
+ if (val8 == 0xea || !(val16 & SYS_CLK_MAC_CLK_ENABLE))
macpower = false;
else
macpower = true;
--
2.11.0
^ permalink raw reply related
* Re: bug: tpacket_snd can cause data corruption
From: Frank de Brabander @ 2019-07-04 10:43 UTC (permalink / raw)
To: Willem de Bruijn; +Cc: David S . Miller, Willem de Bruijn, Network Development
In-Reply-To: <CAF=yD-+wHzfP6QWJzc=num_VaFvN3RYXV-c3+-VY8EjS87WEiA@mail.gmail.com>
On 03-07-19 18:07, Willem de Bruijn wrote:
> On Wed, Jul 3, 2019 at 7:08 AM Frank de Brabander <debrabander@gmail.com> wrote:
>> In commit 5cd8d46e a fix was applied for data corruption in
>> tpacket_snd. A selftest was added in commit 358be656 which
>> validates this fix.
>>
>> Unfortunately this bug still persists, although since this fix less
>> likely to trigger. This bug was initially observed using a PACKET_MMAP
>> application, but can also be seen by tweaking the kernel selftest.
>>
>> By tweaking the selftest txring_overwrite.c to run
>> as an infinite loop, the data corruption will still trigger. It
>> seems to occur faster by generating interrupts (e.g. by plugging
>> in USB devices). Tested with kernel version 5.2-RC7.
>>
>> Cause for this bug is still unclear.
> The cause of the original bug is well understood.
>
> The issue you report I expect is due to background traffic. And more
> about the test than the kernel implementation.
>
> Can you reproduce the issue when running the modified test in a
> network namespace (./in_netns.sh ./txring_overwrite)?
>
> I observe the issue report outside that, but not inside. That implies
> that what we're observing is random background traffic. The modified
> test then drops the unexpected packet because it mismatches on length.
> As a result the next read (the test always sends two packets, then
> reads both) will report a data mismatch. Because it is reading the
> first test packet, but expecting the second. Output with a bit more
> data:
>
> count: 200
> count: 300
> count: 400
> count: 500
> read: 90B != 100B
> wrong pattern: 0x61 != 0x62
> count: 600
> count: 700
> count: 800
> read: 90B != 100B
> wrong pattern: 0x61 != 0x62
> count: 900
> read: 90B != 100B
> wrong pattern: 0x61 != 0x62
>
> Notice the clear pattern.
>
> This does not trigger inside a network namespace, which is how
> kselftest invokes txring_override (from run_afpackettests).
I'm also able to reproduce the issue inside a network namespace.
I've added the extra logging, as seen in your output, for
mismatches on length. Running the test without ./in_netns.sh
is indeed as you describe:
read error: 66 != 100
wrong pattern: 0x61 != 0x62
read error: 66 != 100
wrong pattern: 0x61 != 0x62
read error: 74 != 100
read error: 66 != 100
wrong pattern: 0x53 != 0x61
wrong pattern: 0x53 != 0x62
read error: 66 != 100
read error: 66 != 100
read error: 66 != 100
wrong pattern: 0x61 != 0x62
read error: 95 != 100
read error: 95 != 100
wrong pattern: 0xffffffbe != 0x61
wrong pattern: 0x61 != 0x62
read error: 66 != 100
But even when running the test with ./in_netns.sh it shows
"wrong pattern", this time without length mismatches:
wrong pattern: 0x62 != 0x61
wrong pattern: 0x62 != 0x61
wrong pattern: 0x62 != 0x61
wrong pattern: 0x62 != 0x61
wrong pattern: 0x62 != 0x61
wrong pattern: 0x62 != 0x61
wrong pattern: 0x62 != 0x61
wrong pattern: 0x62 != 0x61
wrong pattern: 0x62 != 0x61
wrong pattern: 0x62 != 0x61
As already mentioned, it seems to trigger mainly (only ?) when
an USB device is connected. The PC I'm testing this on has an
USB hub with many ports and connected devices. When connecting
this USB hub, the amount of "wrong pattern" errors that are
shown seems to correlate to the amount of new devices
that the kernel should detect. Connecting in a single USB device
also triggers the error, but not on every attempt.
Unfortunately have not found any other way to force the
error to trigger. E.g. running stress-ng to generate CPU load or
timer interrupts does not seem to have any impact.
^ permalink raw reply
* Re: [PATCH net-next 3/3] net: stmmac: Introducing support for Page Pool
From: Ilias Apalodimas @ 2019-07-04 10:30 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: Jose Abreu, linux-kernel, netdev, linux-stm32, linux-arm-kernel,
Joao Pinto, David S . Miller, Giuseppe Cavallaro,
Alexandre Torgue, Maxime Coquelin, Maxime Ripard, Chen-Yu Tsai
In-Reply-To: <20190704120018.4523a119@carbon>
HI Jesper, Ivan,
> On Wed, 3 Jul 2019 12:37:50 +0200
> Jose Abreu <Jose.Abreu@synopsys.com> wrote:
>
> > @@ -3547,6 +3456,9 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
> >
> > napi_gro_receive(&ch->rx_napi, skb);
> >
> > + page_pool_recycle_direct(rx_q->page_pool, buf->page);
>
> This doesn't look correct.
>
> The page_pool DMA mapping cannot be "kept" when page traveling into the
> network stack attached to an SKB. (Ilias and I have a long term plan[1]
> to allow this, but you cannot do it ATM).
>
> You will have to call:
> page_pool_release_page(rx_q->page_pool, buf->page);
>
> This will do a DMA-unmap, and you will likely loose your performance
> gain :-(
>
>
> > + buf->page = NULL;
> > +
> > priv->dev->stats.rx_packets++;
> > priv->dev->stats.rx_bytes += frame_len;
> > }
>
> Also remember that the page_pool requires you driver to do the DMA-sync
> operation. I see a dma_sync_single_for_cpu(), but I didn't see a
> dma_sync_single_for_device() (well, I noticed one getting removed).
> (For some HW Ilias tells me that the dma_sync_single_for_device can be
> elided, so maybe this can still be correct for you).
On our case (and in the page_pool API in general) you have to track buffers when
both .ndo_xdp_xmit() and XDP_TX are used.
So the lifetime of a packet might be
1. page pool allocs packet. The API doesn't sync but i *think* you don't have to
explicitly since the CPU won't touch that buffer until the NAPI handler kicks
in. On the napi handler you need to dma_sync_single_for_cpu() and process the
packet.
2a) no XDP is required so the packet is unmapped and free'd
2b) .ndo_xdp_xmit is called so tyhe buffer need to be mapped/unmapped
2c) XDP_TX is called. In that case we re-use an Rx buffer so we need to
dma_sync_single_for_device()
2a and 2b won't cause any issues
In 2c the buffer will be recycled and fed back to the device with a *correct*
sync (for_device) and all those buffers are allocated as DMA_BIDIRECTIONAL.
So bvottom line i *think* we can skip the dma_sync_single_for_device() on the
initial allocation *only*. If am terribly wrong please let me know :)
Thanks
/Ilias
>
>
> [1] https://github.com/xdp-project/xdp-project/blob/master/areas/mem/page_pool02_SKB_return_callback.org
> --
> 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 v6 net-next 1/5] xdp: allow same allocator usage
From: Ivan Khoronzhuk @ 2019-07-04 10:22 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: grygorii.strashko, hawk, davem, ast, linux-kernel, linux-omap,
xdp-newbies, ilias.apalodimas, netdev, daniel, jakub.kicinski,
john.fastabend
In-Reply-To: <20190703194013.02842e42@carbon>
On Wed, Jul 03, 2019 at 07:40:13PM +0200, Jesper Dangaard Brouer wrote:
>On Wed, 3 Jul 2019 13:18:59 +0300
>Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
>
>> First of all, it is an absolute requirement that each RX-queue have
>> their own page_pool object/allocator. And this change is intendant
>> to handle special case, where a single RX-queue can receive packets
>> from two different net_devices.
>>
>> In order to protect against using same allocator for 2 different rx
>> queues, add queue_index to xdp_mem_allocator to catch the obvious
>> mistake where queue_index mismatch, as proposed by Jesper Dangaard
>> Brouer.
>>
>> Adding this on xdp allocator level allows drivers with such dependency
>> change the allocators w/o modifications.
>>
>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
>> ---
>> include/net/xdp_priv.h | 2 ++
>> net/core/xdp.c | 55 ++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 57 insertions(+)
>>
>> diff --git a/include/net/xdp_priv.h b/include/net/xdp_priv.h
>> index 6a8cba6ea79a..9858a4057842 100644
>> --- a/include/net/xdp_priv.h
>> +++ b/include/net/xdp_priv.h
>> @@ -18,6 +18,8 @@ struct xdp_mem_allocator {
>> struct rcu_head rcu;
>> struct delayed_work defer_wq;
>> unsigned long defer_warn;
>> + unsigned long refcnt;
>> + u32 queue_index;
>> };
>
>I don't like this approach, because I think we need to extend struct
>xdp_mem_allocator with a net_device pointer, for doing dev_hold(), to
>correctly handle lifetime issues. (As I tried to explain previously).
>This will be much harder after this change, which is why I proposed the
>other patch.
My concern comes not from zero also.
It's partly continuation of not answered questions from here:
https://lwn.net/ml/netdev/20190625122822.GC6485@khorivan/
"For me it's important to know only if it means that alloc.count is
freed at first call of __mem_id_disconnect() while shutdown.
The workqueue for the rest is connected only with ring cache protected
by ring lock and not supposed that alloc.count can be changed while
workqueue tries to shutdonwn the pool."
So patch you propose to leave works only because of luck, because fast
cache is cleared before workqueue is scheduled and no races between two
workqueues for fast cache later. I'm not really against this patch, but
I have to try smth better.
So, the patch is fine only because of specific of page_pool implementation.
I'm not sure that in future similar workqueue completion will be lucky for
another allocator (it easily can happen due to xdp frame can live longer
than an allocator). Similar problem can happen with other drivers having
same allocator, that can use zca (potentially can use smth similar),
af_xdp api allows to switch on it or some other allocators....
But not the essence. The concern about adding smth new to the allocator
later, like net device, can be solved with a little modification to the patch,
(despite here can be several more approaches) for instance, like this:
(by fact it's still the same, when mem_alloc instance per each register call
but with same void *allocator)
diff --git a/include/net/xdp_priv.h b/include/net/xdp_priv.h
index 6a8cba6ea79a..c7ad0f41e1b0 100644
--- a/include/net/xdp_priv.h
+++ b/include/net/xdp_priv.h
@@ -18,6 +18,8 @@ struct xdp_mem_allocator {
struct rcu_head rcu;
struct delayed_work defer_wq;
unsigned long defer_warn;
+ unsigned long *refcnt;
+ u32 queue_index;
};
#endif /* __LINUX_NET_XDP_PRIV_H__ */
diff --git a/net/core/xdp.c b/net/core/xdp.c
index 829377cc83db..a44e3e4c8307 100644
--- a/net/core/xdp.c
+++ b/net/core/xdp.c
@@ -64,9 +64,37 @@ static const struct rhashtable_params mem_id_rht_params = {
.obj_cmpfn = xdp_mem_id_cmp,
};
+static struct xdp_mem_allocator *xdp_allocator_find(void *allocator)
+{
+ struct xdp_mem_allocator *xae, *xa = NULL;
+ struct rhashtable_iter iter;
+
+ if (!allocator)
+ return xa;
+
+ rhashtable_walk_enter(mem_id_ht, &iter);
+ do {
+ rhashtable_walk_start(&iter);
+
+ while ((xae = rhashtable_walk_next(&iter)) && !IS_ERR(xae)) {
+ if (xae->allocator == allocator) {
+ xa = xae;
+ break;
+ }
+ }
+
+ rhashtable_walk_stop(&iter);
+
+ } while (xae == ERR_PTR(-EAGAIN));
+ rhashtable_walk_exit(&iter);
+
+ return xa;
+}
+
static void __xdp_mem_allocator_rcu_free(struct rcu_head *rcu)
{
struct xdp_mem_allocator *xa;
+ void *allocator;
xa = container_of(rcu, struct xdp_mem_allocator, rcu);
@@ -74,15 +102,27 @@ static void __xdp_mem_allocator_rcu_free(struct rcu_head *rcu)
if (xa->mem.type == MEM_TYPE_PAGE_POOL)
page_pool_free(xa->page_pool);
- /* Allow this ID to be reused */
- ida_simple_remove(&mem_id_pool, xa->mem.id);
+ kfree(xa->refcnt);
+ allocator = xa->allocator;
+ while (xa) {
+ xa = xdp_allocator_find(allocator);
+ if (!xa)
+ break;
+
+ mutex_lock(&mem_id_lock);
+ rhashtable_remove_fast(mem_id_ht, &xa->node, mem_id_rht_params);
+ mutex_unlock(&mem_id_lock);
- /* Poison memory */
- xa->mem.id = 0xFFFF;
- xa->mem.type = 0xF0F0;
- xa->allocator = (void *)0xDEAD9001;
+ /* Allow this ID to be reused */
+ ida_simple_remove(&mem_id_pool, xa->mem.id);
- kfree(xa);
+ /* Poison memory */
+ xa->mem.id = 0xFFFF;
+ xa->mem.type = 0xF0F0;
+ xa->allocator = (void *)0xDEAD9001;
+
+ kfree(xa);
+ }
}
static bool __mem_id_disconnect(int id, bool force)
@@ -98,6 +138,18 @@ static bool __mem_id_disconnect(int id, bool force)
WARN(1, "Request remove non-existing id(%d), driver bug?", id);
return true;
}
+
+ /* to avoid calling hash lookup twice, decrement refcnt here till it
+ * reaches zero, then it can be called from workqueue afterwards.
+ */
+ if (*xa->refcnt)
+ (*xa->refcnt)--;
+
+ if (*xa->refcnt) {
+ mutex_unlock(&mem_id_lock);
+ return true;
+ }
+
xa->disconnect_cnt++;
/* Detects in-flight packet-pages for page_pool */
@@ -106,8 +158,7 @@ static bool __mem_id_disconnect(int id, bool force)
trace_mem_disconnect(xa, safe_to_remove, force);
- if ((safe_to_remove || force) &&
- !rhashtable_remove_fast(mem_id_ht, &xa->node, mem_id_rht_params))
+ if (safe_to_remove || force)
call_rcu(&xa->rcu, __xdp_mem_allocator_rcu_free);
mutex_unlock(&mem_id_lock);
@@ -316,6 +367,7 @@ int xdp_rxq_info_reg_mem_model(struct xdp_rxq_info *xdp_rxq,
enum xdp_mem_type type, void *allocator)
{
struct xdp_mem_allocator *xdp_alloc;
+ unsigned long *refcnt = NULL;
gfp_t gfp = GFP_KERNEL;
int id, errno, ret;
void *ptr;
@@ -347,6 +399,19 @@ int xdp_rxq_info_reg_mem_model(struct xdp_rxq_info *xdp_rxq,
}
}
+ mutex_lock(&mem_id_lock);
+ xdp_alloc = xdp_allocator_find(allocator);
+ if (xdp_alloc) {
+ /* One allocator per queue is supposed only */
+ if (xdp_alloc->queue_index != xdp_rxq->queue_index) {
+ mutex_unlock(&mem_id_lock);
+ return -EINVAL;
+ }
+
+ refcnt = xdp_alloc->refcnt;
+ }
+ mutex_unlock(&mem_id_lock);
+
xdp_alloc = kzalloc(sizeof(*xdp_alloc), gfp);
if (!xdp_alloc)
return -ENOMEM;
@@ -360,6 +425,7 @@ int xdp_rxq_info_reg_mem_model(struct xdp_rxq_info *xdp_rxq,
xdp_rxq->mem.id = id;
xdp_alloc->mem = xdp_rxq->mem;
xdp_alloc->allocator = allocator;
+ xdp_alloc->queue_index = xdp_rxq->queue_index;
/* Insert allocator into ID lookup table */
ptr = rhashtable_insert_slow(mem_id_ht, &id, &xdp_alloc->node);
@@ -370,6 +436,16 @@ int xdp_rxq_info_reg_mem_model(struct xdp_rxq_info *xdp_rxq,
goto err;
}
+ if (!refcnt) {
+ refcnt = kzalloc(sizeof(*xdp_alloc->refcnt), gfp);
+ if (!refcnt) {
+ errno = -ENOMEM;
+ goto err;
+ }
+ }
+
+ (*refcnt)++;
+ xdp_alloc->refcnt = refcnt;
mutex_unlock(&mem_id_lock);
trace_mem_connect(xdp_alloc, xdp_rxq);
>
>
>> #endif /* __LINUX_NET_XDP_PRIV_H__ */
>> diff --git a/net/core/xdp.c b/net/core/xdp.c
>> index 829377cc83db..4f0ddbb3717a 100644
>> --- a/net/core/xdp.c
>> +++ b/net/core/xdp.c
>> @@ -98,6 +98,18 @@ static bool __mem_id_disconnect(int id, bool force)
>> WARN(1, "Request remove non-existing id(%d), driver bug?", id);
>> return true;
>> }
>> +
>> + /* to avoid calling hash lookup twice, decrement refcnt here till it
>> + * reaches zero, then it can be called from workqueue afterwards.
>> + */
>> + if (xa->refcnt)
>> + xa->refcnt--;
>> +
>> + if (xa->refcnt) {
>> + mutex_unlock(&mem_id_lock);
>> + return true;
>> + }
>> +
>> xa->disconnect_cnt++;
>>
>> /* Detects in-flight packet-pages for page_pool */
>> @@ -312,6 +324,33 @@ static bool __is_supported_mem_type(enum xdp_mem_type type)
>> return true;
>> }
>>
>> +static struct xdp_mem_allocator *xdp_allocator_find(void *allocator)
>> +{
>> + struct xdp_mem_allocator *xae, *xa = NULL;
>> + struct rhashtable_iter iter;
>> +
>> + if (!allocator)
>> + return xa;
>> +
>> + rhashtable_walk_enter(mem_id_ht, &iter);
>> + do {
>> + rhashtable_walk_start(&iter);
>> +
>> + while ((xae = rhashtable_walk_next(&iter)) && !IS_ERR(xae)) {
>> + if (xae->allocator == allocator) {
>> + xa = xae;
>> + break;
>> + }
>> + }
>> +
>> + rhashtable_walk_stop(&iter);
>> +
>> + } while (xae == ERR_PTR(-EAGAIN));
>> + rhashtable_walk_exit(&iter);
>> +
>> + return xa;
>> +}
>> +
>> int xdp_rxq_info_reg_mem_model(struct xdp_rxq_info *xdp_rxq,
>> enum xdp_mem_type type, void *allocator)
>> {
>> @@ -347,6 +386,20 @@ int xdp_rxq_info_reg_mem_model(struct xdp_rxq_info *xdp_rxq,
>> }
>> }
>>
>> + mutex_lock(&mem_id_lock);
>> + xdp_alloc = xdp_allocator_find(allocator);
>> + if (xdp_alloc) {
>> + /* One allocator per queue is supposed only */
>> + if (xdp_alloc->queue_index != xdp_rxq->queue_index)
>> + return -EINVAL;
>> +
>> + xdp_rxq->mem.id = xdp_alloc->mem.id;
>> + xdp_alloc->refcnt++;
>> + mutex_unlock(&mem_id_lock);
>> + return 0;
>> + }
>> + mutex_unlock(&mem_id_lock);
>> +
>> xdp_alloc = kzalloc(sizeof(*xdp_alloc), gfp);
>> if (!xdp_alloc)
>> return -ENOMEM;
>> @@ -360,6 +413,8 @@ int xdp_rxq_info_reg_mem_model(struct xdp_rxq_info *xdp_rxq,
>> xdp_rxq->mem.id = id;
>> xdp_alloc->mem = xdp_rxq->mem;
>> xdp_alloc->allocator = allocator;
>> + xdp_alloc->refcnt = 1;
>> + xdp_alloc->queue_index = xdp_rxq->queue_index;
>>
>> /* Insert allocator into ID lookup table */
>> ptr = rhashtable_insert_slow(mem_id_ht, &id, &xdp_alloc->node);
>
>
>
>--
>Best regards,
> Jesper Dangaard Brouer
> MSc.CS, Principal Kernel Engineer at Red Hat
> LinkedIn: http://www.linkedin.com/in/brouer
--
Regards,
Ivan Khoronzhuk
^ permalink raw reply related
* Re: [PATCH ipsec v2] xfrm interface: fix memory leak on creation
From: Steffen Klassert @ 2019-07-04 10:22 UTC (permalink / raw)
To: Nicolas Dichtel
Cc: davem, netdev, Lorenzo Colitti, Benedict Wong, Shannon Nelson,
Antony Antony, Eyal Birger, Julien Floret
In-Reply-To: <20190702155139.11399-1-nicolas.dichtel@6wind.com>
On Tue, Jul 02, 2019 at 05:51:39PM +0200, Nicolas Dichtel wrote:
> The following commands produce a backtrace and return an error but the xfrm
> interface is created (in the wrong netns):
> $ ip netns add foo
> $ ip netns add bar
> $ ip -n foo netns set bar 0
> $ ip -n foo link add xfrmi0 link-netnsid 0 type xfrm dev lo if_id 23
> RTNETLINK answers: Invalid argument
> $ ip -n bar link ls xfrmi0
> 2: xfrmi0@lo: <NOARP,M-DOWN> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
> link/none 00:00:00:00:00:00 brd 00:00:00:00:00:00
>
> Here is the backtrace:
> [ 79.879174] WARNING: CPU: 0 PID: 1178 at net/core/dev.c:8172 rollback_registered_many+0x86/0x3c1
> [ 79.880260] Modules linked in: xfrm_interface nfsv3 nfs_acl auth_rpcgss nfsv4 nfs lockd grace sunrpc fscache button parport_pc parport serio_raw evdev pcspkr loop ext4 crc16 mbcache jbd2 crc32c_generic ide_cd_mod ide_gd_mod cdrom ata_$
> eneric ata_piix libata scsi_mod 8139too piix psmouse i2c_piix4 ide_core 8139cp mii i2c_core floppy
> [ 79.883698] CPU: 0 PID: 1178 Comm: ip Not tainted 5.2.0-rc6+ #106
> [ 79.884462] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014
> [ 79.885447] RIP: 0010:rollback_registered_many+0x86/0x3c1
> [ 79.886120] Code: 01 e8 d7 7d c6 ff 0f 0b 48 8b 45 00 4c 8b 20 48 8d 58 90 49 83 ec 70 48 8d 7b 70 48 39 ef 74 44 8a 83 d0 04 00 00 84 c0 75 1f <0f> 0b e8 61 cd ff ff 48 b8 00 01 00 00 00 00 ad de 48 89 43 70 66
> [ 79.888667] RSP: 0018:ffffc900015ab740 EFLAGS: 00010246
> [ 79.889339] RAX: ffff8882353e5700 RBX: ffff8882353e56a0 RCX: ffff8882353e5710
> [ 79.890174] RDX: ffffc900015ab7e0 RSI: ffffc900015ab7e0 RDI: ffff8882353e5710
> [ 79.891029] RBP: ffffc900015ab7e0 R08: ffffc900015ab7e0 R09: ffffc900015ab7e0
> [ 79.891866] R10: ffffc900015ab7a0 R11: ffffffff82233fec R12: ffffc900015ab770
> [ 79.892728] R13: ffffffff81eb7ec0 R14: ffff88822ed6cf00 R15: 00000000ffffffea
> [ 79.893557] FS: 00007ff350f31740(0000) GS:ffff888237a00000(0000) knlGS:0000000000000000
> [ 79.894581] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 79.895317] CR2: 00000000006c8580 CR3: 000000022c272000 CR4: 00000000000006f0
> [ 79.896137] Call Trace:
> [ 79.896464] unregister_netdevice_many+0x12/0x6c
> [ 79.896998] __rtnl_newlink+0x6e2/0x73b
> [ 79.897446] ? __kmalloc_node_track_caller+0x15e/0x185
> [ 79.898039] ? pskb_expand_head+0x5f/0x1fe
> [ 79.898556] ? stack_access_ok+0xd/0x2c
> [ 79.899009] ? deref_stack_reg+0x12/0x20
> [ 79.899462] ? stack_access_ok+0xd/0x2c
> [ 79.899927] ? stack_access_ok+0xd/0x2c
> [ 79.900404] ? __module_text_address+0x9/0x4f
> [ 79.900910] ? is_bpf_text_address+0x5/0xc
> [ 79.901390] ? kernel_text_address+0x67/0x7b
> [ 79.901884] ? __kernel_text_address+0x1a/0x25
> [ 79.902397] ? unwind_get_return_address+0x12/0x23
> [ 79.903122] ? __cmpxchg_double_slab.isra.37+0x46/0x77
> [ 79.903772] rtnl_newlink+0x43/0x56
> [ 79.904217] rtnetlink_rcv_msg+0x200/0x24c
>
> In fact, each time a xfrm interface was created, a netdev was allocated
> by __rtnl_newlink()/rtnl_create_link() and then another one by
> xfrmi_newlink()/xfrmi_create(). Only the second one was registered, it's
> why the previous commands produce a backtrace: dev_change_net_namespace()
> was called on a netdev with reg_state set to NETREG_UNINITIALIZED (the
> first one).
>
> CC: Lorenzo Colitti <lorenzo@google.com>
> CC: Benedict Wong <benedictwong@google.com>
> CC: Steffen Klassert <steffen.klassert@secunet.com>
> CC: Shannon Nelson <shannon.nelson@oracle.com>
> CC: Antony Antony <antony@phenome.org>
> CC: Eyal Birger <eyal.birger@gmail.com>
> Fixes: f203b76d7809 ("xfrm: Add virtual xfrm interfaces")
> Reported-by: Julien Floret <julien.floret@6wind.com>
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Applied, thanks a lot!
^ permalink raw reply
* Re: [PATCH ipsec] xfrm: policy: fix bydst hlist corruption on hash rebuild
From: Steffen Klassert @ 2019-07-04 10:21 UTC (permalink / raw)
To: Florian Westphal; +Cc: netdev, syzkaller-bugs, syzbot+0165480d4ef07360eeda
In-Reply-To: <20190702104600.9744-1-fw@strlen.de>
On Tue, Jul 02, 2019 at 12:46:00PM +0200, Florian Westphal wrote:
> syzbot reported following spat:
>
> BUG: KASAN: use-after-free in __write_once_size include/linux/compiler.h:221
> BUG: KASAN: use-after-free in hlist_del_rcu include/linux/rculist.h:455
> BUG: KASAN: use-after-free in xfrm_hash_rebuild+0xa0d/0x1000 net/xfrm/xfrm_policy.c:1318
> Write of size 8 at addr ffff888095e79c00 by task kworker/1:3/8066
> Workqueue: events xfrm_hash_rebuild
> Call Trace:
> __write_once_size include/linux/compiler.h:221 [inline]
> hlist_del_rcu include/linux/rculist.h:455 [inline]
> xfrm_hash_rebuild+0xa0d/0x1000 net/xfrm/xfrm_policy.c:1318
> process_one_work+0x814/0x1130 kernel/workqueue.c:2269
> Allocated by task 8064:
> __kmalloc+0x23c/0x310 mm/slab.c:3669
> kzalloc include/linux/slab.h:742 [inline]
> xfrm_hash_alloc+0x38/0xe0 net/xfrm/xfrm_hash.c:21
> xfrm_policy_init net/xfrm/xfrm_policy.c:4036 [inline]
> xfrm_net_init+0x269/0xd60 net/xfrm/xfrm_policy.c:4120
> ops_init+0x336/0x420 net/core/net_namespace.c:130
> setup_net+0x212/0x690 net/core/net_namespace.c:316
>
> The faulting address is the address of the old chain head,
> free'd by xfrm_hash_resize().
>
> In xfrm_hash_rehash(), chain heads get re-initialized without
> any hlist_del_rcu:
>
> for (i = hmask; i >= 0; i--)
> INIT_HLIST_HEAD(odst + i);
>
> Then, hlist_del_rcu() gets called on the about to-be-reinserted policy
> when iterating the per-net list of policies.
>
> hlist_del_rcu() will then make chain->first be nonzero again:
>
> static inline void __hlist_del(struct hlist_node *n)
> {
> struct hlist_node *next = n->next; // address of next element in list
> struct hlist_node **pprev = n->pprev;// location of previous elem, this
> // can point at chain->first
> WRITE_ONCE(*pprev, next); // chain->first points to next elem
> if (next)
> next->pprev = pprev;
>
> Then, when we walk chainlist to find insertion point, we may find a
> non-empty list even though we're supposedly reinserting the first
> policy to an empty chain.
>
> To fix this first unlink all exact and inexact policies instead of
> zeroing the list heads.
>
> Add the commands equivalent to the syzbot reproducer to xfrm_policy.sh,
> without fix KASAN catches the corruption as it happens, SLUB poisoning
> detects it a bit later.
>
> Reported-by: syzbot+0165480d4ef07360eeda@syzkaller.appspotmail.com
> Fixes: 1548bc4e0512 ("xfrm: policy: delete inexact policies from inexact list on hash rebuild")
> Signed-off-by: Florian Westphal <fw@strlen.de>
Applied, thanks Florian!
^ permalink raw reply
* Re: [PATCH v2 1/3] vsock/virtio: use RCU to avoid use-after-free on the_virtio_vsock
From: Stefan Hajnoczi @ 2019-07-04 10:17 UTC (permalink / raw)
To: Stefano Garzarella
Cc: Jason Wang, netdev, kvm, virtualization, Michael S. Tsirkin,
David S. Miller, linux-kernel
In-Reply-To: <20190703104135.wg34dobv64k7u4jo@steredhat>
[-- Attachment #1: Type: text/plain, Size: 723 bytes --]
On Wed, Jul 03, 2019 at 12:41:35PM +0200, Stefano Garzarella wrote:
> On Wed, Jul 03, 2019 at 05:53:58PM +0800, Jason Wang wrote:
> > On 2019/6/28 下午8:36, Stefano Garzarella wrote:
> > Another more interesting question, I believe we will do singleton for
> > virtio_vsock structure. Then what's the point of using vdev->priv to access
> > the_virtio_vsock? It looks to me we can it brings extra troubles for doing
> > synchronization.
>
> I thought about it when I tried to use RCU to stop the worker and I
> think make sense. Maybe can be another series after this will be merged.
>
> @Stefan, what do you think about that?
Yes, let's make it a singleton and keep no other references to it.
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* RE: [PATCH net-next 3/3] net: stmmac: Introducing support for Page Pool
From: Jose Abreu @ 2019-07-04 10:13 UTC (permalink / raw)
To: Jesper Dangaard Brouer, Jose Abreu
Cc: linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
linux-stm32@st-md-mailman.stormreply.com,
linux-arm-kernel@lists.infradead.org, Joao Pinto,
David S . Miller, Giuseppe Cavallaro, Alexandre Torgue,
Maxime Coquelin, Maxime Ripard, Chen-Yu Tsai, Ilias Apalodimas
In-Reply-To: <20190704120018.4523a119@carbon>
From: Jesper Dangaard Brouer <brouer@redhat.com>
> The page_pool DMA mapping cannot be "kept" when page traveling into the
> network stack attached to an SKB. (Ilias and I have a long term plan[1]
> to allow this, but you cannot do it ATM).
The reason I recycle the page is this previous call to:
skb_copy_to_linear_data()
So, technically, I'm syncing to CPU the page(s) and then memcpy to a
previously allocated SKB ... So it's safe to just recycle the mapping I
think.
Its kind of using bounce buffers and I do see performance gain in this
(I think the reason is because my setup uses swiotlb for DMA mapping).
Anyway, I'm open to some suggestions on how to improve this ...
> Also remember that the page_pool requires you driver to do the DMA-sync
> operation. I see a dma_sync_single_for_cpu(), but I didn't see a
> dma_sync_single_for_device() (well, I noticed one getting removed).
> (For some HW Ilias tells me that the dma_sync_single_for_device can be
> elided, so maybe this can still be correct for you).
My HW just needs descriptors refilled which are in different coherent
region so I don't see any reason for dma_sync_single_for_device() ...
^ permalink raw reply
* Re: [PATCH net-next 3/3] net: stmmac: Introducing support for Page Pool
From: Jesper Dangaard Brouer @ 2019-07-04 10:00 UTC (permalink / raw)
To: Jose Abreu
Cc: brouer, linux-kernel, netdev, linux-stm32, linux-arm-kernel,
Joao Pinto, David S . Miller, Giuseppe Cavallaro,
Alexandre Torgue, Maxime Coquelin, Maxime Ripard, Chen-Yu Tsai,
Ilias Apalodimas
In-Reply-To: <1b254bb7fc6044c5e6e2fdd9e00088d1d13a808b.1562149883.git.joabreu@synopsys.com>
On Wed, 3 Jul 2019 12:37:50 +0200
Jose Abreu <Jose.Abreu@synopsys.com> wrote:
> @@ -3547,6 +3456,9 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
>
> napi_gro_receive(&ch->rx_napi, skb);
>
> + page_pool_recycle_direct(rx_q->page_pool, buf->page);
This doesn't look correct.
The page_pool DMA mapping cannot be "kept" when page traveling into the
network stack attached to an SKB. (Ilias and I have a long term plan[1]
to allow this, but you cannot do it ATM).
You will have to call:
page_pool_release_page(rx_q->page_pool, buf->page);
This will do a DMA-unmap, and you will likely loose your performance
gain :-(
> + buf->page = NULL;
> +
> priv->dev->stats.rx_packets++;
> priv->dev->stats.rx_bytes += frame_len;
> }
Also remember that the page_pool requires you driver to do the DMA-sync
operation. I see a dma_sync_single_for_cpu(), but I didn't see a
dma_sync_single_for_device() (well, I noticed one getting removed).
(For some HW Ilias tells me that the dma_sync_single_for_device can be
elided, so maybe this can still be correct for you).
[1] https://github.com/xdp-project/xdp-project/blob/master/areas/mem/page_pool02_SKB_return_callback.org
--
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 v6 net-next 5/5] net: ethernet: ti: cpsw: add XDP support
From: Ivan Khoronzhuk @ 2019-07-04 9:53 UTC (permalink / raw)
To: Ilias Apalodimas
Cc: Jesper Dangaard Brouer, grygorii.strashko, hawk, davem, ast,
linux-kernel, linux-omap, xdp-newbies, netdev, daniel,
jakub.kicinski, john.fastabend
In-Reply-To: <20190704094938.GA27382@apalos>
On Thu, Jul 04, 2019 at 12:49:38PM +0300, Ilias Apalodimas wrote:
>On Thu, Jul 04, 2019 at 12:43:30PM +0300, Ivan Khoronzhuk wrote:
>> On Thu, Jul 04, 2019 at 12:39:02PM +0300, Ilias Apalodimas wrote:
>> >On Thu, Jul 04, 2019 at 11:19:39AM +0200, Jesper Dangaard Brouer wrote:
>> >>On Wed, 3 Jul 2019 13:19:03 +0300
>> >>Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
>> >>
>> >>> Add XDP support based on rx page_pool allocator, one frame per page.
>> >>> Page pool allocator is used with assumption that only one rx_handler
>> >>> is running simultaneously. DMA map/unmap is reused from page pool
>> >>> despite there is no need to map whole page.
>> >>>
>> >>> Due to specific of cpsw, the same TX/RX handler can be used by 2
>> >>> network devices, so special fields in buffer are added to identify
>> >>> an interface the frame is destined to. Thus XDP works for both
>> >>> interfaces, that allows to test xdp redirect between two interfaces
>> >>> easily. Aslo, each rx queue have own page pools, but common for both
>> >>> netdevs.
>> >>>
>> >>> XDP prog is common for all channels till appropriate changes are added
>> >>> in XDP infrastructure. Also, once page_pool recycling becomes part of
>> >>> skb netstack some simplifications can be added, like removing
>> >>> page_pool_release_page() before skb receive.
>> >>>
>> >>> In order to keep rx_dev while redirect, that can be somehow used in
>> >>> future, do flush in rx_handler, that allows to keep rx dev the same
>> >>> while reidrect. It allows to conform with tracing rx_dev pointed
>> >>> by Jesper.
>> >>
>> >>So, you simply call xdp_do_flush_map() after each xdp_do_redirect().
>> >>It will kill RX-bulk and performance, but I guess it will work.
>> >>
>> >>I guess, we can optimized it later, by e.g. in function calling
>> >>cpsw_run_xdp() have a variable that detect if net_device changed
>> >>(priv->ndev) and then call xdp_do_flush_map() when needed.
>> >I tried something similar on the netsec driver on my initial development.
>> >On the 1gbit speed NICs i saw no difference between flushing per packet vs
>> >flushing on the end of the NAPI handler.
>> >The latter is obviously better but since the performance impact is negligible on
>> >this particular NIC, i don't think this should be a blocker.
>> >Please add a clear comment on this and why you do that on this driver,
>> >so people won't go ahead and copy/paste this approach
>> Sry, but I did this already, is it not enouph?
>The flush *must* happen there to avoid messing the following layers. The comment
>says something like 'just to be sure'. It's not something that might break, it's
>something that *will* break the code and i don't think that's clear with the
>current comment.
>
>So i'd prefer something like
>'We must flush here, per packet, instead of doing it in bulk at the end of
>the napi handler.The RX devices on this particular hardware is sharing a
>common queue, so the incoming device might change per packet'
Sounds good, will replace on it.
--
Regards,
Ivan Khoronzhuk
^ permalink raw reply
* Re: [PATCH v6 net-next 5/5] net: ethernet: ti: cpsw: add XDP support
From: Ilias Apalodimas @ 2019-07-04 9:49 UTC (permalink / raw)
To: Jesper Dangaard Brouer, grygorii.strashko, hawk, davem, ast,
linux-kernel, linux-omap, xdp-newbies, netdev, daniel,
jakub.kicinski, john.fastabend
In-Reply-To: <20190704094329.GA19839@khorivan>
On Thu, Jul 04, 2019 at 12:43:30PM +0300, Ivan Khoronzhuk wrote:
> On Thu, Jul 04, 2019 at 12:39:02PM +0300, Ilias Apalodimas wrote:
> >On Thu, Jul 04, 2019 at 11:19:39AM +0200, Jesper Dangaard Brouer wrote:
> >>On Wed, 3 Jul 2019 13:19:03 +0300
> >>Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
> >>
> >>> Add XDP support based on rx page_pool allocator, one frame per page.
> >>> Page pool allocator is used with assumption that only one rx_handler
> >>> is running simultaneously. DMA map/unmap is reused from page pool
> >>> despite there is no need to map whole page.
> >>>
> >>> Due to specific of cpsw, the same TX/RX handler can be used by 2
> >>> network devices, so special fields in buffer are added to identify
> >>> an interface the frame is destined to. Thus XDP works for both
> >>> interfaces, that allows to test xdp redirect between two interfaces
> >>> easily. Aslo, each rx queue have own page pools, but common for both
> >>> netdevs.
> >>>
> >>> XDP prog is common for all channels till appropriate changes are added
> >>> in XDP infrastructure. Also, once page_pool recycling becomes part of
> >>> skb netstack some simplifications can be added, like removing
> >>> page_pool_release_page() before skb receive.
> >>>
> >>> In order to keep rx_dev while redirect, that can be somehow used in
> >>> future, do flush in rx_handler, that allows to keep rx dev the same
> >>> while reidrect. It allows to conform with tracing rx_dev pointed
> >>> by Jesper.
> >>
> >>So, you simply call xdp_do_flush_map() after each xdp_do_redirect().
> >>It will kill RX-bulk and performance, but I guess it will work.
> >>
> >>I guess, we can optimized it later, by e.g. in function calling
> >>cpsw_run_xdp() have a variable that detect if net_device changed
> >>(priv->ndev) and then call xdp_do_flush_map() when needed.
> >I tried something similar on the netsec driver on my initial development.
> >On the 1gbit speed NICs i saw no difference between flushing per packet vs
> >flushing on the end of the NAPI handler.
> >The latter is obviously better but since the performance impact is negligible on
> >this particular NIC, i don't think this should be a blocker.
> >Please add a clear comment on this and why you do that on this driver,
> >so people won't go ahead and copy/paste this approach
> Sry, but I did this already, is it not enouph?
The flush *must* happen there to avoid messing the following layers. The comment
says something like 'just to be sure'. It's not something that might break, it's
something that *will* break the code and i don't think that's clear with the
current comment.
So i'd prefer something like
'We must flush here, per packet, instead of doing it in bulk at the end of
the napi handler.The RX devices on this particular hardware is sharing a
common queue, so the incoming device might change per packet'
Thanks
/Ilias
>
> --
> Regards,
> Ivan Khoronzhuk
^ permalink raw reply
* Re: [PATCH net-next 3/3] net: stmmac: Introducing support for Page Pool
From: Jesper Dangaard Brouer @ 2019-07-04 9:48 UTC (permalink / raw)
To: Jose Abreu
Cc: brouer, linux-kernel, netdev, linux-stm32, linux-arm-kernel,
Joao Pinto, David S . Miller, Giuseppe Cavallaro,
Alexandre Torgue, Maxime Coquelin, Maxime Ripard, Chen-Yu Tsai,
Ilias Apalodimas
In-Reply-To: <1b254bb7fc6044c5e6e2fdd9e00088d1d13a808b.1562149883.git.joabreu@synopsys.com>
On Wed, 3 Jul 2019 12:37:50 +0200
Jose Abreu <Jose.Abreu@synopsys.com> wrote:
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -1197,26 +1197,14 @@ static int stmmac_init_rx_buffers(struct stmmac_priv *priv, struct dma_desc *p,
> int i, gfp_t flags, u32 queue)
> {
> struct stmmac_rx_queue *rx_q = &priv->rx_queue[queue];
> - struct sk_buff *skb;
> + struct stmmac_rx_buffer *buf = &rx_q->buf_pool[i];
>
> - skb = __netdev_alloc_skb_ip_align(priv->dev, priv->dma_buf_sz, flags);
> - if (!skb) {
> - netdev_err(priv->dev,
> - "%s: Rx init fails; skb is NULL\n", __func__);
> + buf->page = page_pool_dev_alloc_pages(rx_q->page_pool);
> + if (!buf->page)
> return -ENOMEM;
> - }
> - rx_q->rx_skbuff[i] = skb;
> - rx_q->rx_skbuff_dma[i] = dma_map_single(priv->device, skb->data,
> - priv->dma_buf_sz,
> - DMA_FROM_DEVICE);
> - if (dma_mapping_error(priv->device, rx_q->rx_skbuff_dma[i])) {
> - netdev_err(priv->dev, "%s: DMA mapping error\n", __func__);
> - dev_kfree_skb_any(skb);
> - return -EINVAL;
> - }
> -
> - stmmac_set_desc_addr(priv, p, rx_q->rx_skbuff_dma[i]);
>
> + buf->addr = buf->page->dma_addr;
We/Ilias added a wrapper/helper function for accessing dma_addr, as it
will help us later identifying users.
page_pool_get_dma_addr(page)
> + stmmac_set_desc_addr(priv, p, buf->addr);
> if (priv->dma_buf_sz == BUF_SIZE_16KiB)
> stmmac_init_desc3(priv, p);
>
--
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 v6 net-next 5/5] net: ethernet: ti: cpsw: add XDP support
From: Ivan Khoronzhuk @ 2019-07-04 9:45 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: grygorii.strashko, hawk, davem, ast, linux-kernel, linux-omap,
xdp-newbies, ilias.apalodimas, netdev, daniel, jakub.kicinski,
john.fastabend
In-Reply-To: <20190704111939.5d845071@carbon>
On Thu, Jul 04, 2019 at 11:19:39AM +0200, Jesper Dangaard Brouer wrote:
>On Wed, 3 Jul 2019 13:19:03 +0300
>Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
>
>> Add XDP support based on rx page_pool allocator, one frame per page.
>> Page pool allocator is used with assumption that only one rx_handler
>> is running simultaneously. DMA map/unmap is reused from page pool
>> despite there is no need to map whole page.
>>
>> Due to specific of cpsw, the same TX/RX handler can be used by 2
>> network devices, so special fields in buffer are added to identify
>> an interface the frame is destined to. Thus XDP works for both
>> interfaces, that allows to test xdp redirect between two interfaces
>> easily. Aslo, each rx queue have own page pools, but common for both
>> netdevs.
>>
>> XDP prog is common for all channels till appropriate changes are added
>> in XDP infrastructure. Also, once page_pool recycling becomes part of
>> skb netstack some simplifications can be added, like removing
>> page_pool_release_page() before skb receive.
>>
>> In order to keep rx_dev while redirect, that can be somehow used in
>> future, do flush in rx_handler, that allows to keep rx dev the same
>> while reidrect. It allows to conform with tracing rx_dev pointed
>> by Jesper.
>
>So, you simply call xdp_do_flush_map() after each xdp_do_redirect().
>It will kill RX-bulk and performance, but I guess it will work.
>
>I guess, we can optimized it later, by e.g. in function calling
>cpsw_run_xdp() have a variable that detect if net_device changed
>(priv->ndev) and then call xdp_do_flush_map() when needed.
It's problem of cpsw already and can be optimized locally by own
bulk queues for instance, if it will be simple if really needed ofc.
>
>
>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
>> ---
>> drivers/net/ethernet/ti/Kconfig | 1 +
>> drivers/net/ethernet/ti/cpsw.c | 485 ++++++++++++++++++++++---
>> drivers/net/ethernet/ti/cpsw_ethtool.c | 66 +++-
>> drivers/net/ethernet/ti/cpsw_priv.h | 7 +
>> 4 files changed, 502 insertions(+), 57 deletions(-)
>>
>[...]
>> +static int cpsw_run_xdp(struct cpsw_priv *priv, int ch, struct xdp_buff *xdp,
>> + struct page *page)
>> +{
>> + struct cpsw_common *cpsw = priv->cpsw;
>> + struct net_device *ndev = priv->ndev;
>> + int ret = CPSW_XDP_CONSUMED;
>> + struct xdp_frame *xdpf;
>> + struct bpf_prog *prog;
>> + u32 act;
>> +
>> + rcu_read_lock();
>> +
>> + prog = READ_ONCE(priv->xdp_prog);
>> + if (!prog) {
>> + ret = CPSW_XDP_PASS;
>> + goto out;
>> + }
>> +
>> + act = bpf_prog_run_xdp(prog, xdp);
>> + switch (act) {
>> + case XDP_PASS:
>> + ret = CPSW_XDP_PASS;
>> + break;
>> + case XDP_TX:
>> + xdpf = convert_to_xdp_frame(xdp);
>> + if (unlikely(!xdpf))
>> + goto drop;
>> +
>> + cpsw_xdp_tx_frame(priv, xdpf, page);
>> + break;
>> + case XDP_REDIRECT:
>> + if (xdp_do_redirect(ndev, xdp, prog))
>> + goto drop;
>> +
>> + /* as flush requires rx_dev to be per NAPI handle and there
>> + * is can be two devices putting packets on bulk queue,
>> + * do flush here avoid this just for sure.
>> + */
>> + xdp_do_flush_map();
>
>> + break;
>> + default:
>> + bpf_warn_invalid_xdp_action(act);
>> + /* fall through */
>> + case XDP_ABORTED:
>> + trace_xdp_exception(ndev, prog, act);
>> + /* fall through -- handle aborts by dropping packet */
>> + case XDP_DROP:
>> + goto drop;
>> + }
>> +out:
>> + rcu_read_unlock();
>> + return ret;
>> +drop:
>> + rcu_read_unlock();
>> + page_pool_recycle_direct(cpsw->page_pool[ch], page);
>> + return ret;
>> +}
>
>--
>Best regards,
> Jesper Dangaard Brouer
> MSc.CS, Principal Kernel Engineer at Red Hat
> LinkedIn: http://www.linkedin.com/in/brouer
--
Regards,
Ivan Khoronzhuk
^ permalink raw reply
* Re: [PATCH v6 net-next 5/5] net: ethernet: ti: cpsw: add XDP support
From: Ivan Khoronzhuk @ 2019-07-04 9:43 UTC (permalink / raw)
To: Ilias Apalodimas
Cc: Jesper Dangaard Brouer, grygorii.strashko, hawk, davem, ast,
linux-kernel, linux-omap, xdp-newbies, netdev, daniel,
jakub.kicinski, john.fastabend
In-Reply-To: <20190704093902.GA26927@apalos>
On Thu, Jul 04, 2019 at 12:39:02PM +0300, Ilias Apalodimas wrote:
>On Thu, Jul 04, 2019 at 11:19:39AM +0200, Jesper Dangaard Brouer wrote:
>> On Wed, 3 Jul 2019 13:19:03 +0300
>> Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
>>
>> > Add XDP support based on rx page_pool allocator, one frame per page.
>> > Page pool allocator is used with assumption that only one rx_handler
>> > is running simultaneously. DMA map/unmap is reused from page pool
>> > despite there is no need to map whole page.
>> >
>> > Due to specific of cpsw, the same TX/RX handler can be used by 2
>> > network devices, so special fields in buffer are added to identify
>> > an interface the frame is destined to. Thus XDP works for both
>> > interfaces, that allows to test xdp redirect between two interfaces
>> > easily. Aslo, each rx queue have own page pools, but common for both
>> > netdevs.
>> >
>> > XDP prog is common for all channels till appropriate changes are added
>> > in XDP infrastructure. Also, once page_pool recycling becomes part of
>> > skb netstack some simplifications can be added, like removing
>> > page_pool_release_page() before skb receive.
>> >
>> > In order to keep rx_dev while redirect, that can be somehow used in
>> > future, do flush in rx_handler, that allows to keep rx dev the same
>> > while reidrect. It allows to conform with tracing rx_dev pointed
>> > by Jesper.
>>
>> So, you simply call xdp_do_flush_map() after each xdp_do_redirect().
>> It will kill RX-bulk and performance, but I guess it will work.
>>
>> I guess, we can optimized it later, by e.g. in function calling
>> cpsw_run_xdp() have a variable that detect if net_device changed
>> (priv->ndev) and then call xdp_do_flush_map() when needed.
>I tried something similar on the netsec driver on my initial development.
>On the 1gbit speed NICs i saw no difference between flushing per packet vs
>flushing on the end of the NAPI handler.
>The latter is obviously better but since the performance impact is negligible on
>this particular NIC, i don't think this should be a blocker.
>Please add a clear comment on this and why you do that on this driver,
>so people won't go ahead and copy/paste this approach
Sry, but I did this already, is it not enouph?
--
Regards,
Ivan Khoronzhuk
^ permalink raw reply
* Re: [PATCH net-next 3/3] net: stmmac: Introducing support for Page Pool
From: Jesper Dangaard Brouer @ 2019-07-04 9:39 UTC (permalink / raw)
To: Jose Abreu
Cc: brouer, linux-kernel, netdev, linux-stm32, linux-arm-kernel,
Joao Pinto, David S . Miller, Giuseppe Cavallaro,
Alexandre Torgue, Maxime Coquelin, Maxime Ripard, Chen-Yu Tsai
In-Reply-To: <1b254bb7fc6044c5e6e2fdd9e00088d1d13a808b.1562149883.git.joabreu@synopsys.com>
On Wed, 3 Jul 2019 12:37:50 +0200
Jose Abreu <Jose.Abreu@synopsys.com> wrote:
> @@ -1498,8 +1479,9 @@ static void free_dma_rx_desc_resources(struct stmmac_priv *priv)
> sizeof(struct dma_extended_desc),
> rx_q->dma_erx, rx_q->dma_rx_phy);
>
> - kfree(rx_q->rx_skbuff_dma);
> - kfree(rx_q->rx_skbuff);
> + kfree(rx_q->buf_pool);
> + if (rx_q->page_pool)
> + page_pool_request_shutdown(rx_q->page_pool);
> }
> }
>
The page_pool_request_shutdown() API return indication if there are any
in-flight frames/pages, to know when it is safe to call
page_pool_free(), which you are also missing a call to.
This page_pool_request_shutdown() is only intended to be called from
xdp_rxq_info_unreg() code, that handles and schedule a work queue if it
need to wait for in-flight frames/pages.
--
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 v6 net-next 5/5] net: ethernet: ti: cpsw: add XDP support
From: Ilias Apalodimas @ 2019-07-04 9:39 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: Ivan Khoronzhuk, grygorii.strashko, hawk, davem, ast,
linux-kernel, linux-omap, xdp-newbies, netdev, daniel,
jakub.kicinski, john.fastabend
In-Reply-To: <20190704111939.5d845071@carbon>
On Thu, Jul 04, 2019 at 11:19:39AM +0200, Jesper Dangaard Brouer wrote:
> On Wed, 3 Jul 2019 13:19:03 +0300
> Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
>
> > Add XDP support based on rx page_pool allocator, one frame per page.
> > Page pool allocator is used with assumption that only one rx_handler
> > is running simultaneously. DMA map/unmap is reused from page pool
> > despite there is no need to map whole page.
> >
> > Due to specific of cpsw, the same TX/RX handler can be used by 2
> > network devices, so special fields in buffer are added to identify
> > an interface the frame is destined to. Thus XDP works for both
> > interfaces, that allows to test xdp redirect between two interfaces
> > easily. Aslo, each rx queue have own page pools, but common for both
> > netdevs.
> >
> > XDP prog is common for all channels till appropriate changes are added
> > in XDP infrastructure. Also, once page_pool recycling becomes part of
> > skb netstack some simplifications can be added, like removing
> > page_pool_release_page() before skb receive.
> >
> > In order to keep rx_dev while redirect, that can be somehow used in
> > future, do flush in rx_handler, that allows to keep rx dev the same
> > while reidrect. It allows to conform with tracing rx_dev pointed
> > by Jesper.
>
> So, you simply call xdp_do_flush_map() after each xdp_do_redirect().
> It will kill RX-bulk and performance, but I guess it will work.
>
> I guess, we can optimized it later, by e.g. in function calling
> cpsw_run_xdp() have a variable that detect if net_device changed
> (priv->ndev) and then call xdp_do_flush_map() when needed.
I tried something similar on the netsec driver on my initial development.
On the 1gbit speed NICs i saw no difference between flushing per packet vs
flushing on the end of the NAPI handler.
The latter is obviously better but since the performance impact is negligible on
this particular NIC, i don't think this should be a blocker.
Please add a clear comment on this and why you do that on this driver,
so people won't go ahead and copy/paste this approach
Thanks
/Ilias
>
>
> > Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> > ---
> > drivers/net/ethernet/ti/Kconfig | 1 +
> > drivers/net/ethernet/ti/cpsw.c | 485 ++++++++++++++++++++++---
> > drivers/net/ethernet/ti/cpsw_ethtool.c | 66 +++-
> > drivers/net/ethernet/ti/cpsw_priv.h | 7 +
> > 4 files changed, 502 insertions(+), 57 deletions(-)
> >
> [...]
> > +static int cpsw_run_xdp(struct cpsw_priv *priv, int ch, struct xdp_buff *xdp,
> > + struct page *page)
> > +{
> > + struct cpsw_common *cpsw = priv->cpsw;
> > + struct net_device *ndev = priv->ndev;
> > + int ret = CPSW_XDP_CONSUMED;
> > + struct xdp_frame *xdpf;
> > + struct bpf_prog *prog;
> > + u32 act;
> > +
> > + rcu_read_lock();
> > +
> > + prog = READ_ONCE(priv->xdp_prog);
> > + if (!prog) {
> > + ret = CPSW_XDP_PASS;
> > + goto out;
> > + }
> > +
> > + act = bpf_prog_run_xdp(prog, xdp);
> > + switch (act) {
> > + case XDP_PASS:
> > + ret = CPSW_XDP_PASS;
> > + break;
> > + case XDP_TX:
> > + xdpf = convert_to_xdp_frame(xdp);
> > + if (unlikely(!xdpf))
> > + goto drop;
> > +
> > + cpsw_xdp_tx_frame(priv, xdpf, page);
> > + break;
> > + case XDP_REDIRECT:
> > + if (xdp_do_redirect(ndev, xdp, prog))
> > + goto drop;
> > +
> > + /* as flush requires rx_dev to be per NAPI handle and there
> > + * is can be two devices putting packets on bulk queue,
> > + * do flush here avoid this just for sure.
> > + */
> > + xdp_do_flush_map();
>
> > + break;
> > + default:
> > + bpf_warn_invalid_xdp_action(act);
> > + /* fall through */
> > + case XDP_ABORTED:
> > + trace_xdp_exception(ndev, prog, act);
> > + /* fall through -- handle aborts by dropping packet */
> > + case XDP_DROP:
> > + goto drop;
> > + }
> > +out:
> > + rcu_read_unlock();
> > + return ret;
> > +drop:
> > + rcu_read_unlock();
> > + page_pool_recycle_direct(cpsw->page_pool[ch], page);
> > + return ret;
> > +}
>
> --
> Best regards,
> Jesper Dangaard Brouer
> MSc.CS, Principal Kernel Engineer at Red Hat
> LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* [PATCH net] r8152: set RTL8152_UNPLUG only for real disconnection
From: Hayes Wang @ 2019-07-04 9:36 UTC (permalink / raw)
To: netdev; +Cc: nic_swsd, linux-kernel, linux-usb, Hayes Wang
Set the flag of RTL8152_UNPLUG if and only if the device is unplugged.
Some error codes sometimes don't mean the real disconnection of usb device.
For those situations, set the flag of RTL8152_UNPLUG causes the driver skips
some flows of disabling the device, and it let the device stay at incorrect
state.
Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
drivers/net/usb/r8152.c | 27 ++++++++++++++++-----------
1 file changed, 16 insertions(+), 11 deletions(-)
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index e887ac86fbef..39e0768d734d 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -28,7 +28,7 @@
#define NETNEXT_VERSION "09"
/* Information for net */
-#define NET_VERSION "9"
+#define NET_VERSION "10"
#define DRIVER_VERSION "v1." NETNEXT_VERSION "." NET_VERSION
#define DRIVER_AUTHOR "Realtek linux nic maintainers <nic_swsd@realtek.com>"
@@ -825,6 +825,14 @@ int set_registers(struct r8152 *tp, u16 value, u16 index, u16 size, void *data)
return ret;
}
+static void rtl_set_unplug(struct r8152 *tp)
+{
+ if (tp->udev->state == USB_STATE_NOTATTACHED) {
+ set_bit(RTL8152_UNPLUG, &tp->flags);
+ smp_mb__after_atomic();
+ }
+}
+
static int generic_ocp_read(struct r8152 *tp, u16 index, u16 size,
void *data, u16 type)
{
@@ -863,7 +871,7 @@ static int generic_ocp_read(struct r8152 *tp, u16 index, u16 size,
}
if (ret == -ENODEV)
- set_bit(RTL8152_UNPLUG, &tp->flags);
+ rtl_set_unplug(tp);
return ret;
}
@@ -933,7 +941,7 @@ static int generic_ocp_write(struct r8152 *tp, u16 index, u16 byteen,
error1:
if (ret == -ENODEV)
- set_bit(RTL8152_UNPLUG, &tp->flags);
+ rtl_set_unplug(tp);
return ret;
}
@@ -1321,7 +1329,7 @@ static void read_bulk_callback(struct urb *urb)
napi_schedule(&tp->napi);
return;
case -ESHUTDOWN:
- set_bit(RTL8152_UNPLUG, &tp->flags);
+ rtl_set_unplug(tp);
netif_device_detach(tp->netdev);
return;
case -ENOENT:
@@ -1441,7 +1449,7 @@ static void intr_callback(struct urb *urb)
resubmit:
res = usb_submit_urb(urb, GFP_ATOMIC);
if (res == -ENODEV) {
- set_bit(RTL8152_UNPLUG, &tp->flags);
+ rtl_set_unplug(tp);
netif_device_detach(tp->netdev);
} else if (res) {
netif_err(tp, intr, tp->netdev,
@@ -2036,7 +2044,7 @@ static void tx_bottom(struct r8152 *tp)
struct net_device *netdev = tp->netdev;
if (res == -ENODEV) {
- set_bit(RTL8152_UNPLUG, &tp->flags);
+ rtl_set_unplug(tp);
netif_device_detach(netdev);
} else {
struct net_device_stats *stats = &netdev->stats;
@@ -2110,7 +2118,7 @@ int r8152_submit_rx(struct r8152 *tp, struct rx_agg *agg, gfp_t mem_flags)
ret = usb_submit_urb(agg->urb, mem_flags);
if (ret == -ENODEV) {
- set_bit(RTL8152_UNPLUG, &tp->flags);
+ rtl_set_unplug(tp);
netif_device_detach(tp->netdev);
} else if (ret) {
struct urb *urb = agg->urb;
@@ -5355,10 +5363,7 @@ static void rtl8152_disconnect(struct usb_interface *intf)
usb_set_intfdata(intf, NULL);
if (tp) {
- struct usb_device *udev = tp->udev;
-
- if (udev->state == USB_STATE_NOTATTACHED)
- set_bit(RTL8152_UNPLUG, &tp->flags);
+ rtl_set_unplug(tp);
netif_napi_del(&tp->napi);
unregister_netdev(tp->netdev);
--
2.21.0
^ permalink raw reply related
* Re: [PATCH net-next v6 09/15] ethtool: generic handlers for GET requests
From: Michal Kubecek @ 2019-07-04 9:28 UTC (permalink / raw)
To: netdev
Cc: Jiri Pirko, David Miller, Jakub Kicinski, Andrew Lunn,
Florian Fainelli, John Linville, Stephen Hemminger, Johannes Berg,
linux-kernel
In-Reply-To: <20190704084913.GA18546@nanopsycho>
On Thu, Jul 04, 2019 at 10:49:13AM +0200, Jiri Pirko wrote:
> Tue, Jul 02, 2019 at 01:50:24PM CEST, mkubecek@suse.cz wrote:
>
> [...]
>
>
> >+/* The structure holding data for unified processing GET requests consists of
> >+ * two parts: request info and reply data. Request info is related to client
> >+ * request and for dump request it stays constant through all processing;
> >+ * reply data contains data for composing a reply message. When processing
> >+ * a dump request, request info is filled only once but reply data is filled
> >+ * from scratch for each reply message.
> >+ *
> >+ * +-----------------+-----------------+------------------+-----------------+
> >+ * | common_req_info | specific info | ethnl_reply_data | specific data |
> >+ * +-----------------+-----------------+------------------+-----------------+
> >+ * |<---------- request info --------->|<----------- reply data ----------->|
> >+ *
> >+ * Request info always starts at offset 0 with struct ethnl_req_info which
> >+ * holds information from parsing the common header. It may be followed by
> >+ * other members for request attributes specific for current message type.
> >+ * Reply data starts with struct ethnl_reply_data which may be followed by
> >+ * other members holding data needed to compose a message.
> >+ */
> >+
>
> [...]
>
>
> >+/**
> >+ * struct get_request_ops - unified handling of GET requests
> >+ * @request_cmd: command id for request (GET)
> >+ * @reply_cmd: command id for reply (GET_REPLY)
> >+ * @hdr_attr: attribute type for request header
> >+ * @max_attr: maximum (top level) attribute type
> >+ * @data_size: total length of data structure
> >+ * @repdata_offset: offset of "reply data" part (struct ethnl_reply_data)
>
> For example, this looks quite scarry for me. You have one big chunk of
> data (according to the scheme above) specific for cmd with reply starting
> at arbitrary offset.
We can split it into two structures, one for request related data with
struct ethnl_req_info embedded at offset 0 and one for reply related
data with struct ethnl_reply_data embedded at offset 0. It would be
probably more convenient to have pointer to request info from reply data
then. The code would get a bit simpler in few places at the expense of
an extra kmalloc().
Michal
^ permalink raw reply
* Re: [PATCH v2 1/3] vsock/virtio: use RCU to avoid use-after-free on the_virtio_vsock
From: Stefano Garzarella @ 2019-07-04 9:20 UTC (permalink / raw)
To: Jason Wang
Cc: Stefan Hajnoczi, netdev, kvm, virtualization, Michael S. Tsirkin,
David S. Miller, linux-kernel
In-Reply-To: <07e5bc00-ebde-4dac-d38c-f008fa230b5f@redhat.com>
On Thu, Jul 04, 2019 at 11:58:00AM +0800, Jason Wang wrote:
>
> On 2019/7/3 下午6:41, Stefano Garzarella wrote:
> > On Wed, Jul 03, 2019 at 05:53:58PM +0800, Jason Wang wrote:
> > > On 2019/6/28 下午8:36, Stefano Garzarella wrote:
> > > > Some callbacks used by the upper layers can run while we are in the
> > > > .remove(). A potential use-after-free can happen, because we free
> > > > the_virtio_vsock without knowing if the callbacks are over or not.
> > > >
> > > > To solve this issue we move the assignment of the_virtio_vsock at the
> > > > end of .probe(), when we finished all the initialization, and at the
> > > > beginning of .remove(), before to release resources.
> > > > For the same reason, we do the same also for the vdev->priv.
> > > >
> > > > We use RCU to be sure that all callbacks that use the_virtio_vsock
> > > > ended before freeing it. This is not required for callbacks that
> > > > use vdev->priv, because after the vdev->config->del_vqs() we are sure
> > > > that they are ended and will no longer be invoked.
> > > >
> > > > We also take the mutex during the .remove() to avoid that .probe() can
> > > > run while we are resetting the device.
> > > >
> > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > > > ---
> > > > net/vmw_vsock/virtio_transport.c | 67 +++++++++++++++++++++-----------
> > > > 1 file changed, 44 insertions(+), 23 deletions(-)
> > > >
> > > > diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
> > > > index 9c287e3e393c..7ad510ec12e0 100644
> > > > --- a/net/vmw_vsock/virtio_transport.c
> > > > +++ b/net/vmw_vsock/virtio_transport.c
> > > > @@ -65,19 +65,22 @@ struct virtio_vsock {
> > > > u32 guest_cid;
> > > > };
> > > > -static struct virtio_vsock *virtio_vsock_get(void)
> > > > -{
> > > > - return the_virtio_vsock;
> > > > -}
> > > > -
> > > > static u32 virtio_transport_get_local_cid(void)
> > > > {
> > > > - struct virtio_vsock *vsock = virtio_vsock_get();
> > > > + struct virtio_vsock *vsock;
> > > > + u32 ret;
> > > > - if (!vsock)
> > > > - return VMADDR_CID_ANY;
> > > > + rcu_read_lock();
> > > > + vsock = rcu_dereference(the_virtio_vsock);
> > > > + if (!vsock) {
> > > > + ret = VMADDR_CID_ANY;
> > > > + goto out_rcu;
> > > > + }
> > > > - return vsock->guest_cid;
> > > > + ret = vsock->guest_cid;
> > > > +out_rcu:
> > > > + rcu_read_unlock();
> > > > + return ret;
> > > > }
> > > > static void virtio_transport_loopback_work(struct work_struct *work)
> > > > @@ -197,14 +200,18 @@ virtio_transport_send_pkt(struct virtio_vsock_pkt *pkt)
> > > > struct virtio_vsock *vsock;
> > > > int len = pkt->len;
> > > > - vsock = virtio_vsock_get();
> > > > + rcu_read_lock();
> > > > + vsock = rcu_dereference(the_virtio_vsock);
> > > > if (!vsock) {
> > > > virtio_transport_free_pkt(pkt);
> > > > - return -ENODEV;
> > > > + len = -ENODEV;
> > > > + goto out_rcu;
> > > > }
> > > > - if (le64_to_cpu(pkt->hdr.dst_cid) == vsock->guest_cid)
> > > > - return virtio_transport_send_pkt_loopback(vsock, pkt);
> > > > + if (le64_to_cpu(pkt->hdr.dst_cid) == vsock->guest_cid) {
> > > > + len = virtio_transport_send_pkt_loopback(vsock, pkt);
> > > > + goto out_rcu;
> > > > + }
> > > > if (pkt->reply)
> > > > atomic_inc(&vsock->queued_replies);
> > > > @@ -214,6 +221,9 @@ virtio_transport_send_pkt(struct virtio_vsock_pkt *pkt)
> > > > spin_unlock_bh(&vsock->send_pkt_list_lock);
> > > > queue_work(virtio_vsock_workqueue, &vsock->send_pkt_work);
> > > > +
> > > > +out_rcu:
> > > > + rcu_read_unlock();
> > > > return len;
> > > > }
> > > > @@ -222,12 +232,14 @@ virtio_transport_cancel_pkt(struct vsock_sock *vsk)
> > > > {
> > > > struct virtio_vsock *vsock;
> > > > struct virtio_vsock_pkt *pkt, *n;
> > > > - int cnt = 0;
> > > > + int cnt = 0, ret;
> > > > LIST_HEAD(freeme);
> > > > - vsock = virtio_vsock_get();
> > > > + rcu_read_lock();
> > > > + vsock = rcu_dereference(the_virtio_vsock);
> > > > if (!vsock) {
> > > > - return -ENODEV;
> > > > + ret = -ENODEV;
> > > > + goto out_rcu;
> > > > }
> > > > spin_lock_bh(&vsock->send_pkt_list_lock);
> > > > @@ -255,7 +267,11 @@ virtio_transport_cancel_pkt(struct vsock_sock *vsk)
> > > > queue_work(virtio_vsock_workqueue, &vsock->rx_work);
> > > > }
> > > > - return 0;
> > > > + ret = 0;
> > > > +
> > > > +out_rcu:
> > > > + rcu_read_unlock();
> > > > + return ret;
> > > > }
> > > > static void virtio_vsock_rx_fill(struct virtio_vsock *vsock)
> > > > @@ -590,8 +606,6 @@ static int virtio_vsock_probe(struct virtio_device *vdev)
> > > > vsock->rx_buf_max_nr = 0;
> > > > atomic_set(&vsock->queued_replies, 0);
> > > > - vdev->priv = vsock;
> > > > - the_virtio_vsock = vsock;
> > > > mutex_init(&vsock->tx_lock);
> > > > mutex_init(&vsock->rx_lock);
> > > > mutex_init(&vsock->event_lock);
> > > > @@ -613,6 +627,9 @@ static int virtio_vsock_probe(struct virtio_device *vdev)
> > > > virtio_vsock_event_fill(vsock);
> > > > mutex_unlock(&vsock->event_lock);
> > > > + vdev->priv = vsock;
> > > > + rcu_assign_pointer(the_virtio_vsock, vsock);
> > >
> > > You probably need to use rcu_dereference_protected() to access
> > > the_virtio_vsock in the function in order to survive from sparse.
> > >
> > Ooo, thanks!
> >
> > Do you mean when we check if the_virtio_vsock is not null at the beginning of
> > virtio_vsock_probe()?
>
>
> I mean instead of:
>
> /* Only one virtio-vsock device per guest is supported */
> if (the_virtio_vsock) {
> ret = -EBUSY;
> goto out;
> }
>
> you should use:
>
> if (rcu_dereference_protected(the_virtio_vosck,
> lock_dep_is_held(&the_virtio_vsock_mutex))
>
> ...
Okay, thanks for confirming! I'll send a v3 to fix this!
>
>
> >
> > > > +
> > > > mutex_unlock(&the_virtio_vsock_mutex);
> > > > return 0;
> > > > @@ -627,6 +644,12 @@ static void virtio_vsock_remove(struct virtio_device *vdev)
> > > > struct virtio_vsock *vsock = vdev->priv;
> > > > struct virtio_vsock_pkt *pkt;
> > > > + mutex_lock(&the_virtio_vsock_mutex);
> > > > +
> > > > + vdev->priv = NULL;
> > > > + rcu_assign_pointer(the_virtio_vsock, NULL);
> > >
> > > This is still suspicious, can we access the_virtio_vsock through vdev->priv?
> > > If yes, we may still get use-after-free since it was not protected by RCU.
> > We will free the object only after calling the del_vqs(), so we are sure
> > that the vq_callbacks ended and will no longer be invoked.
> > So, IIUC it shouldn't happen.
>
>
> Yes, but any dereference that is not done in vq_callbacks will be very
> dangerous in the future.
Right.
Do you think make sense to continue with this series in order to fix the
hot-unplug issue, then I'll work to refactor the driver code to use the refcnt
(as you suggested in patch 2) and singleton for the_virtio_vsock?
Thanks,
Stefano
^ permalink raw reply
* Re: [PATCH v6 net-next 5/5] net: ethernet: ti: cpsw: add XDP support
From: Jesper Dangaard Brouer @ 2019-07-04 9:19 UTC (permalink / raw)
To: Ivan Khoronzhuk
Cc: grygorii.strashko, hawk, davem, ast, linux-kernel, linux-omap,
xdp-newbies, ilias.apalodimas, netdev, daniel, jakub.kicinski,
john.fastabend, brouer
In-Reply-To: <20190703101903.8411-6-ivan.khoronzhuk@linaro.org>
On Wed, 3 Jul 2019 13:19:03 +0300
Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
> Add XDP support based on rx page_pool allocator, one frame per page.
> Page pool allocator is used with assumption that only one rx_handler
> is running simultaneously. DMA map/unmap is reused from page pool
> despite there is no need to map whole page.
>
> Due to specific of cpsw, the same TX/RX handler can be used by 2
> network devices, so special fields in buffer are added to identify
> an interface the frame is destined to. Thus XDP works for both
> interfaces, that allows to test xdp redirect between two interfaces
> easily. Aslo, each rx queue have own page pools, but common for both
> netdevs.
>
> XDP prog is common for all channels till appropriate changes are added
> in XDP infrastructure. Also, once page_pool recycling becomes part of
> skb netstack some simplifications can be added, like removing
> page_pool_release_page() before skb receive.
>
> In order to keep rx_dev while redirect, that can be somehow used in
> future, do flush in rx_handler, that allows to keep rx dev the same
> while reidrect. It allows to conform with tracing rx_dev pointed
> by Jesper.
So, you simply call xdp_do_flush_map() after each xdp_do_redirect().
It will kill RX-bulk and performance, but I guess it will work.
I guess, we can optimized it later, by e.g. in function calling
cpsw_run_xdp() have a variable that detect if net_device changed
(priv->ndev) and then call xdp_do_flush_map() when needed.
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> ---
> drivers/net/ethernet/ti/Kconfig | 1 +
> drivers/net/ethernet/ti/cpsw.c | 485 ++++++++++++++++++++++---
> drivers/net/ethernet/ti/cpsw_ethtool.c | 66 +++-
> drivers/net/ethernet/ti/cpsw_priv.h | 7 +
> 4 files changed, 502 insertions(+), 57 deletions(-)
>
[...]
> +static int cpsw_run_xdp(struct cpsw_priv *priv, int ch, struct xdp_buff *xdp,
> + struct page *page)
> +{
> + struct cpsw_common *cpsw = priv->cpsw;
> + struct net_device *ndev = priv->ndev;
> + int ret = CPSW_XDP_CONSUMED;
> + struct xdp_frame *xdpf;
> + struct bpf_prog *prog;
> + u32 act;
> +
> + rcu_read_lock();
> +
> + prog = READ_ONCE(priv->xdp_prog);
> + if (!prog) {
> + ret = CPSW_XDP_PASS;
> + goto out;
> + }
> +
> + act = bpf_prog_run_xdp(prog, xdp);
> + switch (act) {
> + case XDP_PASS:
> + ret = CPSW_XDP_PASS;
> + break;
> + case XDP_TX:
> + xdpf = convert_to_xdp_frame(xdp);
> + if (unlikely(!xdpf))
> + goto drop;
> +
> + cpsw_xdp_tx_frame(priv, xdpf, page);
> + break;
> + case XDP_REDIRECT:
> + if (xdp_do_redirect(ndev, xdp, prog))
> + goto drop;
> +
> + /* as flush requires rx_dev to be per NAPI handle and there
> + * is can be two devices putting packets on bulk queue,
> + * do flush here avoid this just for sure.
> + */
> + xdp_do_flush_map();
> + break;
> + default:
> + bpf_warn_invalid_xdp_action(act);
> + /* fall through */
> + case XDP_ABORTED:
> + trace_xdp_exception(ndev, prog, act);
> + /* fall through -- handle aborts by dropping packet */
> + case XDP_DROP:
> + goto drop;
> + }
> +out:
> + rcu_read_unlock();
> + return ret;
> +drop:
> + rcu_read_unlock();
> + page_pool_recycle_direct(cpsw->page_pool[ch], page);
> + return ret;
> +}
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* [PATCH][net-next] net: remove unused parameter from skb_checksum_try_convert
From: Li RongQing @ 2019-07-04 9:03 UTC (permalink / raw)
To: netdev
the check parameter is never used
Signed-off-by: Li RongQing <lirongqing@baidu.com>
---
include/linux/skbuff.h | 8 +++-----
net/ipv4/gre_demux.c | 2 +-
net/ipv4/udp.c | 3 +--
net/ipv6/udp.c | 3 +--
4 files changed, 6 insertions(+), 10 deletions(-)
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index c922ac8a8bd6..f0b5adeb644d 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -3914,18 +3914,16 @@ static inline bool __skb_checksum_convert_check(struct sk_buff *skb)
return (skb->ip_summed == CHECKSUM_NONE && skb->csum_valid);
}
-static inline void __skb_checksum_convert(struct sk_buff *skb,
- __sum16 check, __wsum pseudo)
+static inline void __skb_checksum_convert(struct sk_buff *skb, __wsum pseudo)
{
skb->csum = ~pseudo;
skb->ip_summed = CHECKSUM_COMPLETE;
}
-#define skb_checksum_try_convert(skb, proto, check, compute_pseudo) \
+#define skb_checksum_try_convert(skb, proto, compute_pseudo) \
do { \
if (__skb_checksum_convert_check(skb)) \
- __skb_checksum_convert(skb, check, \
- compute_pseudo(skb, proto)); \
+ __skb_checksum_convert(skb, compute_pseudo(skb, proto)); \
} while (0)
static inline void skb_remcsum_adjust_partial(struct sk_buff *skb, void *ptr,
diff --git a/net/ipv4/gre_demux.c b/net/ipv4/gre_demux.c
index 293acfb36376..44bfeecac33e 100644
--- a/net/ipv4/gre_demux.c
+++ b/net/ipv4/gre_demux.c
@@ -83,7 +83,7 @@ int gre_parse_header(struct sk_buff *skb, struct tnl_ptk_info *tpi,
options = (__be32 *)(greh + 1);
if (greh->flags & GRE_CSUM) {
if (!skb_checksum_simple_validate(skb)) {
- skb_checksum_try_convert(skb, IPPROTO_GRE, 0,
+ skb_checksum_try_convert(skb, IPPROTO_GRE,
null_compute_pseudo);
} else if (csum_err) {
*csum_err = true;
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 1b971bd95786..c21862ba9c02 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -2224,8 +2224,7 @@ static int udp_unicast_rcv_skb(struct sock *sk, struct sk_buff *skb,
int ret;
if (inet_get_convert_csum(sk) && uh->check && !IS_UDPLITE(sk))
- skb_checksum_try_convert(skb, IPPROTO_UDP, uh->check,
- inet_compute_pseudo);
+ skb_checksum_try_convert(skb, IPPROTO_UDP, inet_compute_pseudo);
ret = udp_queue_rcv_skb(sk, skb);
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index 66ca5a4b17c4..4406e059da68 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -826,8 +826,7 @@ static int udp6_unicast_rcv_skb(struct sock *sk, struct sk_buff *skb,
int ret;
if (inet_get_convert_csum(sk) && uh->check && !IS_UDPLITE(sk))
- skb_checksum_try_convert(skb, IPPROTO_UDP, uh->check,
- ip6_compute_pseudo);
+ skb_checksum_try_convert(skb, IPPROTO_UDP, ip6_compute_pseudo);
ret = udpv6_queue_rcv_skb(sk, skb);
--
2.16.2
^ permalink raw reply related
* [PATCH] tools bpftool: Fix json dump crash on powerpc
From: Jiri Olsa @ 2019-07-04 8:58 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann
Cc: Michael Petlan, netdev, bpf, Martin KaFai Lau
Michael reported crash with by bpf program in json mode on powerpc:
# bpftool prog -p dump jited id 14
[{
"name": "0xd00000000a9aa760",
"insns": [{
"pc": "0x0",
"operation": "nop",
"operands": [null
]
},{
"pc": "0x4",
"operation": "nop",
"operands": [null
]
},{
"pc": "0x8",
"operation": "mflr",
Segmentation fault (core dumped)
The code is assuming char pointers in format, which is not always
true at least for powerpc. Fixing this by dumping the whole string
into buffer based on its format.
Please note that libopcodes code does not check return values from
fprintf callback, so there's no point to return error in case of
allocation failure.
Reported-by: Michael Petlan <mpetlan@redhat.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
tools/bpf/bpftool/jit_disasm.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/tools/bpf/bpftool/jit_disasm.c b/tools/bpf/bpftool/jit_disasm.c
index 3ef3093560ba..05fa6dc970f8 100644
--- a/tools/bpf/bpftool/jit_disasm.c
+++ b/tools/bpf/bpftool/jit_disasm.c
@@ -11,6 +11,8 @@
* Licensed under the GNU General Public License, version 2.0 (GPLv2)
*/
+#define _GNU_SOURCE
+#include <stdio.h>
#include <stdarg.h>
#include <stdint.h>
#include <stdio.h>
@@ -44,11 +46,13 @@ static int fprintf_json(void *out, const char *fmt, ...)
char *s;
va_start(ap, fmt);
+ if (vasprintf(&s, fmt, ap) < 0)
+ return 0;
+ va_end(ap);
+
if (!oper_count) {
int i;
- s = va_arg(ap, char *);
-
/* Strip trailing spaces */
i = strlen(s) - 1;
while (s[i] == ' ')
@@ -61,11 +65,10 @@ static int fprintf_json(void *out, const char *fmt, ...)
} else if (!strcmp(fmt, ",")) {
/* Skip */
} else {
- s = va_arg(ap, char *);
jsonw_string(json_wtr, s);
oper_count++;
}
- va_end(ap);
+ free(s);
return 0;
}
--
2.21.0
^ permalink raw reply related
* [PATCH bpf-next] tools: bpftool: add "prog run" subcommand to test-run programs
From: Quentin Monnet @ 2019-07-04 8:56 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann
Cc: bpf, netdev, oss-drivers, Quentin Monnet
Add a new "bpftool prog run" subcommand to run a loaded program on input
data (and possibly with input context) passed by the user.
Print output data (and output context if relevant) into a file or into
the console. Print return value and duration for the test run into the
console.
A "repeat" argument can be passed to run the program several times in a
row.
The command does not perform any kind of verification based on program
type (Is this program type allowed to use an input context?) or on data
consistency (Can I work with empty input data?), this is left to the
kernel.
Example invocation:
# perl -e 'print "\x0" x 14' | ./bpftool prog run \
pinned /sys/fs/bpf/sample_ret0 \
data_in - data_out - repeat 5
0000000 0000 0000 0000 0000 0000 0000 0000 | ........ ......
Return value: 0, duration (average): 260ns
When one of data_in or ctx_in is "-", bpftool reads from standard input,
in binary format. Other formats (JSON, hexdump) might be supported (via
an optional command line keyword like "data_fmt_in") in the future if
relevant, but this would require doing more parsing in bpftool.
Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
.../bpftool/Documentation/bpftool-prog.rst | 34 ++
tools/bpf/bpftool/bash-completion/bpftool | 28 +-
tools/bpf/bpftool/main.c | 29 ++
tools/bpf/bpftool/main.h | 1 +
tools/bpf/bpftool/prog.c | 348 +++++++++++++++++-
tools/include/linux/sizes.h | 48 +++
6 files changed, 485 insertions(+), 3 deletions(-)
create mode 100644 tools/include/linux/sizes.h
diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
index 1df637f85f94..7a374b3c851d 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
@@ -29,6 +29,7 @@ PROG COMMANDS
| **bpftool** **prog attach** *PROG* *ATTACH_TYPE* [*MAP*]
| **bpftool** **prog detach** *PROG* *ATTACH_TYPE* [*MAP*]
| **bpftool** **prog tracelog**
+| **bpftool** **prog run** *PROG* **data_in** *FILE* [**data_out** *FILE* [**data_size_out** *L*]] [**ctx_in** *FILE* [**ctx_out** *FILE* [**ctx_size_out** *M*]]] [**repeat** *N*]
| **bpftool** **prog help**
|
| *MAP* := { **id** *MAP_ID* | **pinned** *FILE* }
@@ -146,6 +147,39 @@ DESCRIPTION
streaming data from BPF programs to user space, one can use
perf events (see also **bpftool-map**\ (8)).
+ **bpftool prog run** *PROG* **data_in** *FILE* [**data_out** *FILE* [**data_size_out** *L*]] [**ctx_in** *FILE* [**ctx_out** *FILE* [**ctx_size_out** *M*]]] [**repeat** *N*]
+ Run BPF program *PROG* in the kernel testing infrastructure
+ for BPF, meaning that the program works on the data and
+ context provided by the user, and not on actual packets or
+ monitored functions etc. Return value and duration for the
+ test run are printed out to the console.
+
+ Input data is read from the *FILE* passed with **data_in**.
+ If this *FILE* is "**-**", input data is read from standard
+ input. Input context, if any, is read from *FILE* passed with
+ **ctx_in**. Again, "**-**" can be used to read from standard
+ input, but only if standard input is not already in use for
+ input data. If a *FILE* is passed with **data_out**, output
+ data is written to that file. Similarly, output context is
+ written to the *FILE* passed with **ctx_out**. For both
+ output flows, "**-**" can be used to print to the standard
+ output (as plain text, or JSON if relevant option was
+ passed). If output keywords are omitted, output data and
+ context are discarded. Keywords **data_size_out** and
+ **ctx_size_out** are used to pass the size (in bytes) for the
+ output buffers to the kernel, although the default of 32 kB
+ should be more than enough for most cases.
+
+ Keyword **repeat** is used to indicate the number of
+ consecutive runs to perform. Note that output data and
+ context printed to files correspond to the last of those
+ runs. The duration printed out at the end of the runs is an
+ average over all runs performed by the command.
+
+ Not all program types support test run. Among those which do,
+ not all of them can take the **ctx_in**/**ctx_out**
+ arguments. bpftool does not perform checks on program types.
+
**bpftool prog help**
Print short help message.
diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
index ba37095e1f62..965a8658cca3 100644
--- a/tools/bpf/bpftool/bash-completion/bpftool
+++ b/tools/bpf/bpftool/bash-completion/bpftool
@@ -408,10 +408,34 @@ _bpftool()
tracelog)
return 0
;;
+ run)
+ if [[ ${#words[@]} -lt 5 ]]; then
+ _filedir
+ return 0
+ fi
+ case $prev in
+ id)
+ _bpftool_get_prog_ids
+ return 0
+ ;;
+ data_in|data_out|ctx_in|ctx_out)
+ _filedir
+ return 0
+ ;;
+ repeat|data_size_out|ctx_size_out)
+ return 0
+ ;;
+ *)
+ _bpftool_once_attr 'data_in data_out data_size_out \
+ ctx_in ctx_out ctx_size_out repeat'
+ return 0
+ ;;
+ esac
+ ;;
*)
[[ $prev == $object ]] && \
- COMPREPLY=( $( compgen -W 'dump help pin attach detach load \
- show list tracelog' -- "$cur" ) )
+ COMPREPLY=( $( compgen -W 'dump help pin attach detach \
+ load show list tracelog run' -- "$cur" ) )
;;
esac
;;
diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
index 4879f6395c7e..e916ff25697f 100644
--- a/tools/bpf/bpftool/main.c
+++ b/tools/bpf/bpftool/main.c
@@ -117,6 +117,35 @@ bool is_prefix(const char *pfx, const char *str)
return !memcmp(str, pfx, strlen(pfx));
}
+/* Last argument MUST be NULL pointer */
+int detect_common_prefix(const char *arg, ...)
+{
+ unsigned int count = 0;
+ const char *ref;
+ char msg[256];
+ va_list ap;
+
+ snprintf(msg, sizeof(msg), "ambiguous prefix: '%s' could be '", arg);
+ va_start(ap, arg);
+ while ((ref = va_arg(ap, const char *))) {
+ if (!is_prefix(arg, ref))
+ continue;
+ count++;
+ if (count > 1)
+ strncat(msg, "' or '", sizeof(msg) - strlen(msg) - 1);
+ strncat(msg, ref, sizeof(msg) - strlen(msg) - 1);
+ }
+ va_end(ap);
+ strncat(msg, "'", sizeof(msg) - strlen(msg) - 1);
+
+ if (count >= 2) {
+ p_err(msg);
+ return -1;
+ }
+
+ return 0;
+}
+
void fprint_hex(FILE *f, void *arg, unsigned int n, const char *sep)
{
unsigned char *data = arg;
diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
index 9c5d9c80f71e..3ef0d9051e10 100644
--- a/tools/bpf/bpftool/main.h
+++ b/tools/bpf/bpftool/main.h
@@ -101,6 +101,7 @@ void p_err(const char *fmt, ...);
void p_info(const char *fmt, ...);
bool is_prefix(const char *pfx, const char *str);
+int detect_common_prefix(const char *arg, ...);
void fprint_hex(FILE *f, void *arg, unsigned int n, const char *sep);
void usage(void) __noreturn;
diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
index 9b0db5d14e31..8dcbaa0a8ab1 100644
--- a/tools/bpf/bpftool/prog.c
+++ b/tools/bpf/bpftool/prog.c
@@ -15,6 +15,7 @@
#include <sys/stat.h>
#include <linux/err.h>
+#include <linux/sizes.h>
#include <bpf.h>
#include <btf.h>
@@ -748,6 +749,344 @@ static int do_detach(int argc, char **argv)
return 0;
}
+static int check_single_stdin(char *file_in, char *other_file_in)
+{
+ if (file_in && other_file_in &&
+ !strcmp(file_in, "-") && !strcmp(other_file_in, "-")) {
+ p_err("cannot use standard input for both data_in and ctx_in");
+ return -1;
+ }
+
+ return 0;
+}
+
+static int get_run_data(const char *fname, void **data_ptr, unsigned int *size)
+{
+ size_t block_size = 256;
+ size_t buf_size = block_size;
+ size_t nb_read = 0;
+ void *tmp;
+ FILE *f;
+
+ if (!fname) {
+ *data_ptr = NULL;
+ *size = 0;
+ return 0;
+ }
+
+ if (!strcmp(fname, "-"))
+ f = stdin;
+ else
+ f = fopen(fname, "r");
+ if (!f) {
+ p_err("failed to open %s: %s", fname, strerror(errno));
+ return -1;
+ }
+
+ *data_ptr = malloc(block_size);
+ if (!*data_ptr) {
+ p_err("failed to allocate memory for data_in/ctx_in: %s",
+ strerror(errno));
+ goto err_fclose;
+ }
+
+ while ((nb_read += fread(*data_ptr + nb_read, 1, block_size, f))) {
+ if (feof(f))
+ break;
+ if (ferror(f)) {
+ p_err("failed to read data_in/ctx_in from %s: %s",
+ fname, strerror(errno));
+ goto err_free;
+ }
+ if (nb_read > buf_size - block_size) {
+ if (buf_size == UINT32_MAX) {
+ p_err("data_in/ctx_in is too long (max: %d)",
+ UINT32_MAX);
+ goto err_free;
+ }
+ /* No space for fread()-ing next chunk; realloc() */
+ buf_size *= 2;
+ tmp = realloc(*data_ptr, buf_size);
+ if (!tmp) {
+ p_err("failed to reallocate data_in/ctx_in: %s",
+ strerror(errno));
+ goto err_free;
+ }
+ *data_ptr = tmp;
+ }
+ }
+ if (f != stdin)
+ fclose(f);
+
+ *size = nb_read;
+ return 0;
+
+err_free:
+ free(*data_ptr);
+ *data_ptr = NULL;
+err_fclose:
+ if (f != stdin)
+ fclose(f);
+ return -1;
+}
+
+static void hex_print(void *data, unsigned int size, FILE *f)
+{
+ size_t i, j;
+ char c;
+
+ for (i = 0; i < size; i += 16) {
+ /* Row offset */
+ fprintf(f, "%07zx\t", i);
+
+ /* Hexadecimal values */
+ for (j = i; j < i + 16 && j < size; j++)
+ fprintf(f, "%02x%s", *(uint8_t *)(data + j),
+ j % 2 ? " " : "");
+ for (; j < i + 16; j++)
+ fprintf(f, " %s", j % 2 ? " " : "");
+
+ /* ASCII values (if relevant), '.' otherwise */
+ fprintf(f, "| ");
+ for (j = i; j < i + 16 && j < size; j++) {
+ c = *(char *)(data + j);
+ if (c < ' ' || c > '~')
+ c = '.';
+ fprintf(f, "%c%s", c, j == i + 7 ? " " : "");
+ }
+
+ fprintf(f, "\n");
+ }
+}
+
+static int
+print_run_output(void *data, unsigned int size, const char *fname,
+ const char *json_key)
+{
+ size_t nb_written;
+ FILE *f;
+
+ if (!fname)
+ return 0;
+
+ if (!strcmp(fname, "-")) {
+ f = stdout;
+ if (json_output) {
+ jsonw_name(json_wtr, json_key);
+ print_data_json(data, size);
+ } else {
+ hex_print(data, size, f);
+ }
+ return 0;
+ }
+
+ f = fopen(fname, "w");
+ if (!f) {
+ p_err("failed to open %s: %s", fname, strerror(errno));
+ return -1;
+ }
+
+ nb_written = fwrite(data, 1, size, f);
+ fclose(f);
+ if (nb_written != size) {
+ p_err("failed to write output data/ctx: %s", strerror(errno));
+ return -1;
+ }
+
+ return 0;
+}
+
+static int alloc_run_data(void **data_ptr, unsigned int size_out)
+{
+ *data_ptr = calloc(size_out, 1);
+ if (!*data_ptr) {
+ p_err("failed to allocate memory for output data/ctx: %s",
+ strerror(errno));
+ return -1;
+ }
+
+ return 0;
+}
+
+static int do_run(int argc, char **argv)
+{
+ char *data_fname_in = NULL, *data_fname_out = NULL;
+ char *ctx_fname_in = NULL, *ctx_fname_out = NULL;
+ struct bpf_prog_test_run_attr test_attr = {0};
+ const unsigned int default_size = SZ_32K;
+ void *data_in = NULL, *data_out = NULL;
+ void *ctx_in = NULL, *ctx_out = NULL;
+ unsigned int repeat = 1;
+ int fd, err;
+
+ if (!REQ_ARGS(4))
+ return -1;
+
+ fd = prog_parse_fd(&argc, &argv);
+ if (fd < 0)
+ return -1;
+
+ while (argc) {
+ if (detect_common_prefix(*argv, "data_in", "data_out",
+ "data_size_out", NULL))
+ return -1;
+ if (detect_common_prefix(*argv, "ctx_in", "ctx_out",
+ "ctx_size_out", NULL))
+ return -1;
+
+ if (is_prefix(*argv, "data_in")) {
+ NEXT_ARG();
+ if (!REQ_ARGS(1))
+ return -1;
+
+ data_fname_in = GET_ARG();
+ if (check_single_stdin(data_fname_in, ctx_fname_in))
+ return -1;
+ } else if (is_prefix(*argv, "data_out")) {
+ NEXT_ARG();
+ if (!REQ_ARGS(1))
+ return -1;
+
+ data_fname_out = GET_ARG();
+ } else if (is_prefix(*argv, "data_size_out")) {
+ char *endptr;
+
+ NEXT_ARG();
+ if (!REQ_ARGS(1))
+ return -1;
+
+ test_attr.data_size_out = strtoul(*argv, &endptr, 0);
+ if (*endptr) {
+ p_err("can't parse %s as output data size",
+ *argv);
+ return -1;
+ }
+ NEXT_ARG();
+ } else if (is_prefix(*argv, "ctx_in")) {
+ NEXT_ARG();
+ if (!REQ_ARGS(1))
+ return -1;
+
+ ctx_fname_in = GET_ARG();
+ if (check_single_stdin(ctx_fname_in, data_fname_in))
+ return -1;
+ } else if (is_prefix(*argv, "ctx_out")) {
+ NEXT_ARG();
+ if (!REQ_ARGS(1))
+ return -1;
+
+ ctx_fname_out = GET_ARG();
+ } else if (is_prefix(*argv, "ctx_size_out")) {
+ char *endptr;
+
+ NEXT_ARG();
+ if (!REQ_ARGS(1))
+ return -1;
+
+ test_attr.ctx_size_out = strtoul(*argv, &endptr, 0);
+ if (*endptr) {
+ p_err("can't parse %s as output context size",
+ *argv);
+ return -1;
+ }
+ NEXT_ARG();
+ } else if (is_prefix(*argv, "repeat")) {
+ char *endptr;
+
+ NEXT_ARG();
+ if (!REQ_ARGS(1))
+ return -1;
+
+ repeat = strtoul(*argv, &endptr, 0);
+ if (*endptr) {
+ p_err("can't parse %s as repeat number",
+ *argv);
+ return -1;
+ }
+ NEXT_ARG();
+ } else {
+ p_err("expected no more arguments, 'data_in', 'data_out', 'data_size_out', 'ctx_in', 'ctx_out', 'ctx_size_out' or 'repeat', got: '%s'?",
+ *argv);
+ return -1;
+ }
+ }
+
+ err = get_run_data(data_fname_in, &data_in, &test_attr.data_size_in);
+ if (err)
+ return -1;
+
+ if (data_in) {
+ if (!test_attr.data_size_out)
+ test_attr.data_size_out = default_size;
+ err = alloc_run_data(&data_out, test_attr.data_size_out);
+ if (err)
+ goto free_data_in;
+ }
+
+ err = get_run_data(ctx_fname_in, &ctx_in, &test_attr.ctx_size_in);
+ if (err)
+ goto free_data_out;
+
+ if (ctx_in) {
+ if (!test_attr.ctx_size_out)
+ test_attr.ctx_size_out = default_size;
+ err = alloc_run_data(&ctx_out, test_attr.ctx_size_out);
+ if (err)
+ goto free_ctx_in;
+ }
+
+ test_attr.prog_fd = fd;
+ test_attr.repeat = repeat;
+ test_attr.data_in = data_in;
+ test_attr.data_out = data_out;
+ test_attr.ctx_in = ctx_in;
+ test_attr.ctx_out = ctx_out;
+
+ err = bpf_prog_test_run_xattr(&test_attr);
+ if (err) {
+ p_err("failed to run program: %s", strerror(errno));
+ goto free_ctx_out;
+ }
+
+ err = 0;
+
+ if (json_output)
+ jsonw_start_object(json_wtr); /* root */
+
+ /* Do not exit on errors occurring when printing output data/context,
+ * we still want to print return value and duration for program run.
+ */
+ if (test_attr.data_size_out)
+ err += print_run_output(test_attr.data_out,
+ test_attr.data_size_out,
+ data_fname_out, "data_out");
+ if (test_attr.ctx_size_out)
+ err += print_run_output(test_attr.ctx_out,
+ test_attr.ctx_size_out,
+ ctx_fname_out, "ctx_out");
+
+ if (json_output) {
+ jsonw_uint_field(json_wtr, "retval", test_attr.retval);
+ jsonw_uint_field(json_wtr, "duration", test_attr.duration);
+ jsonw_end_object(json_wtr); /* root */
+ } else {
+ fprintf(stdout, "Return value: %u, duration%s: %uns\n",
+ test_attr.retval,
+ repeat > 1 ? " (average)" : "", test_attr.duration);
+ }
+
+free_ctx_out:
+ free(ctx_out);
+free_ctx_in:
+ free(ctx_in);
+free_data_out:
+ free(data_out);
+free_data_in:
+ free(data_in);
+
+ return err;
+}
+
static int load_with_options(int argc, char **argv, bool first_prog_only)
{
struct bpf_object_load_attr load_attr = { 0 };
@@ -1058,6 +1397,11 @@ static int do_help(int argc, char **argv)
" [pinmaps MAP_DIR]\n"
" %s %s attach PROG ATTACH_TYPE [MAP]\n"
" %s %s detach PROG ATTACH_TYPE [MAP]\n"
+ " %s %s run PROG \\\n"
+ " data_in FILE \\\n"
+ " [data_out FILE [data_size_out L]] \\\n"
+ " [ctx_in FILE [ctx_out FILE [ctx_size_out M]]] \\\n"
+ " [repeat N]\n"
" %s %s tracelog\n"
" %s %s help\n"
"\n"
@@ -1079,7 +1423,8 @@ static int do_help(int argc, char **argv)
"",
bin_name, argv[-2], bin_name, argv[-2], bin_name, argv[-2],
bin_name, argv[-2], bin_name, argv[-2], bin_name, argv[-2],
- bin_name, argv[-2], bin_name, argv[-2], bin_name, argv[-2]);
+ bin_name, argv[-2], bin_name, argv[-2], bin_name, argv[-2],
+ bin_name, argv[-2]);
return 0;
}
@@ -1095,6 +1440,7 @@ static const struct cmd cmds[] = {
{ "attach", do_attach },
{ "detach", do_detach },
{ "tracelog", do_tracelog },
+ { "run", do_run },
{ 0 }
};
diff --git a/tools/include/linux/sizes.h b/tools/include/linux/sizes.h
new file mode 100644
index 000000000000..1cbb4c4d016e
--- /dev/null
+++ b/tools/include/linux/sizes.h
@@ -0,0 +1,48 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * include/linux/sizes.h
+ */
+#ifndef __LINUX_SIZES_H__
+#define __LINUX_SIZES_H__
+
+#include <linux/const.h>
+
+#define SZ_1 0x00000001
+#define SZ_2 0x00000002
+#define SZ_4 0x00000004
+#define SZ_8 0x00000008
+#define SZ_16 0x00000010
+#define SZ_32 0x00000020
+#define SZ_64 0x00000040
+#define SZ_128 0x00000080
+#define SZ_256 0x00000100
+#define SZ_512 0x00000200
+
+#define SZ_1K 0x00000400
+#define SZ_2K 0x00000800
+#define SZ_4K 0x00001000
+#define SZ_8K 0x00002000
+#define SZ_16K 0x00004000
+#define SZ_32K 0x00008000
+#define SZ_64K 0x00010000
+#define SZ_128K 0x00020000
+#define SZ_256K 0x00040000
+#define SZ_512K 0x00080000
+
+#define SZ_1M 0x00100000
+#define SZ_2M 0x00200000
+#define SZ_4M 0x00400000
+#define SZ_8M 0x00800000
+#define SZ_16M 0x01000000
+#define SZ_32M 0x02000000
+#define SZ_64M 0x04000000
+#define SZ_128M 0x08000000
+#define SZ_256M 0x10000000
+#define SZ_512M 0x20000000
+
+#define SZ_1G 0x40000000
+#define SZ_2G 0x80000000
+
+#define SZ_4G _AC(0x100000000, ULL)
+
+#endif /* __LINUX_SIZES_H__ */
--
2.17.1
^ permalink raw reply related
* Re: i.mx6ul with DSA in multi chip addressing mode - no MDIO access
From: Benjamin Beckmeyer @ 2019-07-04 8:54 UTC (permalink / raw)
To: Andrew Lunn; +Cc: netdev
In-Reply-To: <20190703155518.GE18473@lunn.ch>
On 03.07.19 17:55, Andrew Lunn wrote:
> On Wed, Jul 03, 2019 at 03:10:34PM +0200, Benjamin Beckmeyer wrote:
>> Hey folks,
>>
>> I'm having a problem with a custom i.mx6ul board. When DSA is loaded I can't
>> get access to the switch via MDIO, but the DSA is working properly. I set up
>> a bridge for testing and the switch is in forwarding mode and i can ping the
>> board. But the MDIO access isn't working at address 2 for the switch. When I
>> delete the DSA from the devicetree and start the board up, I can access the
>> switch via MDIO.
>>
>> With DSA up and running:
>>
>> mii -i 2 0 0x9800
>> mii -i 2 1
>> phyid:2, reg:0x01 -> 0x4000
>> mii -i 2 0 0x9803
>> mii -i 2 1
>> phyid:2, reg:0x01 -> 0x4000
>> mii -i 2 1 0x1883
>> mii -i 2 1
>> phyid:2, reg:0x01 -> 0x4000
> Hi Benjamin
>
> I'm guessing that the driver is also using register 0 and 1 at the
> same time you are, e.g. to poll the PHYs for link status etc.
>
> There are trace points for MDIO, so you can get the kernel to log all
> registers access. That should confirm if i'm right.
>
> Andrew
Hi Andrew,
you were absolutly right. The bus is really busy the whole time, I've
checked that with the tracepoints in mdio_access.
But I'm still wondering why isn't that with a single chip addressing
mode configured switch? I mean, okay, the switch has more ports, but
I've checked the accesses for both. The 6321(single chip addressing
mode) has around 4-5 accesses to the MDIO bus and the 6390(multi chip
addressing mode) has around 600 accesses per second.
Thanks,
Benjamin
^ 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