* Re: [PATCH net-next v2 08/12] xdp: tracking page_pool resources and safe removal
From: Jesper Dangaard Brouer @ 2019-06-25 11:27 UTC (permalink / raw)
To: Ivan Khoronzhuk
Cc: netdev, Ilias Apalodimas, Toke Høiland-Jørgensen,
Tariq Toukan, toshiaki.makita1, grygorii.strashko, mcroce, brouer
In-Reply-To: <20190625105013.GA6485@khorivan>
On Tue, 25 Jun 2019 13:50:14 +0300
Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
> Hi Jesper,
>
> Could you please clarify one question.
>
> On Tue, Jun 18, 2019 at 03:05:47PM +0200, Jesper Dangaard Brouer wrote:
> >This patch is needed before we can allow drivers to use page_pool for
> >DMA-mappings. Today with page_pool and XDP return API, it is possible to
> >remove the page_pool object (from rhashtable), while there are still
> >in-flight packet-pages. This is safely handled via RCU and failed lookups in
> >__xdp_return() fallback to call put_page(), when page_pool object is gone.
> >In-case page is still DMA mapped, this will result in page note getting
> >correctly DMA unmapped.
> >
> >To solve this, the page_pool is extended with tracking in-flight pages. And
> >XDP disconnect system queries page_pool and waits, via workqueue, for all
> >in-flight pages to be returned.
> >
> >To avoid killing performance when tracking in-flight pages, the implement
> >use two (unsigned) counters, that in placed on different cache-lines, and
> >can be used to deduct in-flight packets. This is done by mapping the
> >unsigned "sequence" counters onto signed Two's complement arithmetic
> >operations. This is e.g. used by kernel's time_after macros, described in
> >kernel commit 1ba3aab3033b and 5a581b367b5, and also explained in RFC1982.
> >
> >The trick is these two incrementing counters only need to be read and
> >compared, when checking if it's safe to free the page_pool structure. Which
> >will only happen when driver have disconnected RX/alloc side. Thus, on a
> >non-fast-path.
> >
> >It is chosen that page_pool tracking is also enabled for the non-DMA
> >use-case, as this can be used for statistics later.
> >
> >After this patch, using page_pool requires more strict resource "release",
> >e.g. via page_pool_release_page() that was introduced in this patchset, and
> >previous patches implement/fix this more strict requirement.
> >
> >Drivers no-longer call page_pool_destroy(). Drivers already call
> >xdp_rxq_info_unreg() which call xdp_rxq_info_unreg_mem_model(), which will
> >attempt to disconnect the mem id, and if attempt fails schedule the
> >disconnect for later via delayed workqueue.
> >
> >Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> >Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> >---
> > drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 3 -
> > include/net/page_pool.h | 41 ++++++++++---
> > net/core/page_pool.c | 62 +++++++++++++++-----
> > net/core/xdp.c | 65 +++++++++++++++++++--
> > 4 files changed, 136 insertions(+), 35 deletions(-)
> >
> >diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> >index 2f647be292b6..6c9d4d7defbc 100644
> >--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> >+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> >@@ -643,9 +643,6 @@ static void mlx5e_free_rq(struct mlx5e_rq *rq)
> > }
> >
> > xdp_rxq_info_unreg(&rq->xdp_rxq);
> >- if (rq->page_pool)
> >- page_pool_destroy(rq->page_pool);
> >-
> > mlx5_wq_destroy(&rq->wq_ctrl);
> > }
> >
> >diff --git a/include/net/page_pool.h b/include/net/page_pool.h
> >index 754d980700df..f09b3f1994e6 100644
> >--- a/include/net/page_pool.h
> >+++ b/include/net/page_pool.h
> >@@ -16,14 +16,16 @@
> > * page_pool_alloc_pages() call. Drivers should likely use
> > * page_pool_dev_alloc_pages() replacing dev_alloc_pages().
> > *
> >- * If page_pool handles DMA mapping (use page->private), then API user
> >- * is responsible for invoking page_pool_put_page() once. In-case of
> >- * elevated refcnt, the DMA state is released, assuming other users of
> >- * the page will eventually call put_page().
> >+ * API keeps track of in-flight pages, in-order to let API user know
> >+ * when it is safe to dealloactor page_pool object. Thus, API users
> >+ * must make sure to call page_pool_release_page() when a page is
> >+ * "leaving" the page_pool. Or call page_pool_put_page() where
> >+ * appropiate. For maintaining correct accounting.
> > *
> >- * If no DMA mapping is done, then it can act as shim-layer that
> >- * fall-through to alloc_page. As no state is kept on the page, the
> >- * regular put_page() call is sufficient.
> >+ * API user must only call page_pool_put_page() once on a page, as it
> >+ * will either recycle the page, or in case of elevated refcnt, it
> >+ * will release the DMA mapping and in-flight state accounting. We
> >+ * hope to lift this requirement in the future.
> > */
> > #ifndef _NET_PAGE_POOL_H
> > #define _NET_PAGE_POOL_H
> >@@ -66,9 +68,10 @@ struct page_pool_params {
> > };
> >
> > struct page_pool {
> >- struct rcu_head rcu;
> > struct page_pool_params p;
> >
> >+ u32 pages_state_hold_cnt;
> >+
> > /*
> > * Data structure for allocation side
> > *
> >@@ -96,6 +99,8 @@ struct page_pool {
> > * TODO: Implement bulk return pages into this structure.
> > */
> > struct ptr_ring ring;
> >+
> >+ atomic_t pages_state_release_cnt;
> > };
> >
> > struct page *page_pool_alloc_pages(struct page_pool *pool, gfp_t gfp);
> >@@ -109,8 +114,6 @@ static inline struct page *page_pool_dev_alloc_pages(struct page_pool *pool)
> >
> > struct page_pool *page_pool_create(const struct page_pool_params *params);
> >
> >-void page_pool_destroy(struct page_pool *pool);
> >-
> > void __page_pool_free(struct page_pool *pool);
> > static inline void page_pool_free(struct page_pool *pool)
> > {
> >@@ -143,6 +146,24 @@ static inline void page_pool_recycle_direct(struct page_pool *pool,
> > __page_pool_put_page(pool, page, true);
> > }
> >
> >+/* API user MUST have disconnected alloc-side (not allowed to call
> >+ * page_pool_alloc_pages()) before calling this. The free-side can
> >+ * still run concurrently, to handle in-flight packet-pages.
> >+ *
> >+ * A request to shutdown can fail (with false) if there are still
> >+ * in-flight packet-pages.
> >+ */
> >+bool __page_pool_request_shutdown(struct page_pool *pool);
> >+static inline bool page_pool_request_shutdown(struct page_pool *pool)
> >+{
> >+ /* When page_pool isn't compiled-in, net/core/xdp.c doesn't
> >+ * allow registering MEM_TYPE_PAGE_POOL, but shield linker.
> >+ */
> >+#ifdef CONFIG_PAGE_POOL
> >+ return __page_pool_request_shutdown(pool);
> >+#endif
> >+}
>
> The free side can ran in softirq, that means fast cache recycle is accessed.
> And it increments not atomic pool->alloc.count.
>
> For instance While redirect, for remote interface, while .ndo_xdp_xmit the
> xdp_return_frame_rx_napi(xdpf) is called everywhere in error path ....
>
> In the same time, simultaneously, the work queue can try one more
> time to clear cash, calling __page_pool_request_shutdown()....
>
> Question, what prevents pool->alloc.count to be corrupted by race,
> causing to wrong array num and as result wrong page to be unmapped/put ....or
> even page leak. alloc.count usage is not protected,
> __page_pool_request_shutdown() is called not from same rx NAPI, even not from
> NAPI.
>
> Here, while alloc cache empty procedure in __page_pool_request_shutdown():
You forgot to copy this comment, which explains:
/* Empty alloc cache, assume caller made sure this is
* no-longer in use, and page_pool_alloc_pages() cannot be
* call concurrently.
*/
> while (pool->alloc.count) {
> page = pool->alloc.cache[--pool->alloc.count];
> __page_pool_return_page(pool, page);
> }
>
> For me seems all works fine, but I can't find what have I missed?
You have missed that, it is the drivers responsibility to "disconnect"
the xdp_rxq_info before calling shutdown. Which means that it is not
allowed to be used for RX, while the driver is shutting down a
RX-queue. For drivers this is very natural, else other things will
break.
> ...
>
> Same question about how xdp frame should be returned for drivers running
> tx napi exclusively, it can be still softirq but another CPU? What API
> should be used to return xdp frame.
You have to use the normal xdp_return_frame() which doesn't do "direct"
return.
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* Re: [RFC PATCH 0/2] enable broadcom tagging for bcm531x5 switches
From: Benedikt Spranger @ 2019-06-25 11:20 UTC (permalink / raw)
To: Florian Fainelli; +Cc: netdev, Sebastian Andrzej Siewior, Kurt Kanzenbach
In-Reply-To: <f1f45e1d-5079-ab24-87d8-99b8c6710a08@gmail.com>
On Sat, 22 Jun 2019 19:24:10 -0700
Florian Fainelli <f.fainelli@gmail.com> wrote:
> Something like this should take care of that (untested). You might
> have to explicitly set the IMP port (port 8) in B53_UC_FWD_EN and
> B53_MC_FWD_EN, though since you turn on management mode, this may not
> be required. I might have some time tomorrow to test that on a Lamobo
> R1.
The patch work fine in a manual setup, but not with automagicaly setup
of $DISTRO. But that is a very different story. Trying to debug whats
going wrong there.
Thanks for the patch.
The whole setup of the switch seem to be desperate fragile. Any chance
to get that more robust?
Regards
Bene Spranger
^ permalink raw reply
* Re: [PATCH RFC net-next 1/5] net: dsa: mt7530: Convert to PHYLINK API
From: René van Dorst @ 2019-06-25 11:31 UTC (permalink / raw)
To: Russell King - ARM Linux admin
Cc: sean.wang, f.fainelli, davem, matthias.bgg, andrew,
vivien.didelot, frank-w, netdev, linux-mediatek, linux-mips
In-Reply-To: <20190624153950.hdsuhrvfd77heyor@shell.armlinux.org.uk>
Quoting Russell King - ARM Linux admin <linux@armlinux.org.uk>:
Hi Russel,
Thanks for your review, see also my comments and questions below.
> Hi,
>
> On Mon, Jun 24, 2019 at 04:52:47PM +0200, René van Dorst wrote:
>> Convert mt7530 to PHYLINK API
>>
>> Signed-off-by: René van Dorst <opensource@vdorst.com>
>> ---
>> drivers/net/dsa/mt7530.c | 237 +++++++++++++++++++++++++++++----------
>> drivers/net/dsa/mt7530.h | 9 ++
>> 2 files changed, 187 insertions(+), 59 deletions(-)
>>
>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>> index 3181e95586d6..9c5e4dd00826 100644
>> --- a/drivers/net/dsa/mt7530.c
>> +++ b/drivers/net/dsa/mt7530.c
>> @@ -13,7 +13,7 @@
>> #include <linux/of_mdio.h>
>> #include <linux/of_net.h>
>> #include <linux/of_platform.h>
>> -#include <linux/phy.h>
>> +#include <linux/phylink.h>
>> #include <linux/regmap.h>
>> #include <linux/regulator/consumer.h>
>> #include <linux/reset.h>
>> @@ -633,63 +633,6 @@ mt7530_get_sset_count(struct dsa_switch *ds,
>> int port, int sset)
>> return ARRAY_SIZE(mt7530_mib);
>> }
>>
>> -static void mt7530_adjust_link(struct dsa_switch *ds, int port,
>> - struct phy_device *phydev)
>> -{
>> - struct mt7530_priv *priv = ds->priv;
>> -
>> - if (phy_is_pseudo_fixed_link(phydev)) {
>> - dev_dbg(priv->dev, "phy-mode for master device = %x\n",
>> - phydev->interface);
>> -
>> - /* Setup TX circuit incluing relevant PAD and driving */
>> - mt7530_pad_clk_setup(ds, phydev->interface);
>> -
>> - if (priv->id == ID_MT7530) {
>> - /* Setup RX circuit, relevant PAD and driving on the
>> - * host which must be placed after the setup on the
>> - * device side is all finished.
>> - */
>> - mt7623_pad_clk_setup(ds);
>> - }
>> - } else {
>> - u16 lcl_adv = 0, rmt_adv = 0;
>> - u8 flowctrl;
>> - u32 mcr = PMCR_USERP_LINK | PMCR_FORCE_MODE;
>> -
>> - switch (phydev->speed) {
>> - case SPEED_1000:
>> - mcr |= PMCR_FORCE_SPEED_1000;
>> - break;
>> - case SPEED_100:
>> - mcr |= PMCR_FORCE_SPEED_100;
>> - break;
>> - }
>> -
>> - if (phydev->link)
>> - mcr |= PMCR_FORCE_LNK;
>> -
>> - if (phydev->duplex) {
>> - mcr |= PMCR_FORCE_FDX;
>> -
>> - if (phydev->pause)
>> - rmt_adv = LPA_PAUSE_CAP;
>> - if (phydev->asym_pause)
>> - rmt_adv |= LPA_PAUSE_ASYM;
>> -
>> - lcl_adv = linkmode_adv_to_lcl_adv_t(
>> - phydev->advertising);
>> - flowctrl = mii_resolve_flowctrl_fdx(lcl_adv, rmt_adv);
>> -
>> - if (flowctrl & FLOW_CTRL_TX)
>> - mcr |= PMCR_TX_FC_EN;
>> - if (flowctrl & FLOW_CTRL_RX)
>> - mcr |= PMCR_RX_FC_EN;
>> - }
>> - mt7530_write(priv, MT7530_PMCR_P(port), mcr);
>> - }
>> -}
>> -
>> static int
>> mt7530_cpu_port_enable(struct mt7530_priv *priv,
>> int port)
>> @@ -1323,6 +1266,178 @@ mt7530_setup(struct dsa_switch *ds)
>> return 0;
>> }
>>
>> +static void mt7530_phylink_mac_config(struct dsa_switch *ds, int port,
>> + unsigned int mode,
>> + const struct phylink_link_state *state)
>> +{
>> + struct mt7530_priv *priv = ds->priv;
>> + u32 mcr = PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | PMCR_BACKOFF_EN |
>> + PMCR_BACKPR_EN | PMCR_TX_EN | PMCR_RX_EN;
>> +
>> + switch (port) {
>> + case 0: /* Internal phy */
>> + case 1:
>> + case 2:
>> + case 3:
>> + case 4:
>> + if (state->interface != PHY_INTERFACE_MODE_GMII)
>> + goto unsupported;
>> + break;
>> + /* case 5: Port 5 is not supported! */
>> + case 6: /* 1st cpu port */
>> + if (state->interface != PHY_INTERFACE_MODE_RGMII &&
>> + state->interface != PHY_INTERFACE_MODE_TRGMII)
>> + goto unsupported;
>> +
>> + /* Setup TX circuit incluing relevant PAD and driving */
>> + mt7530_pad_clk_setup(ds, state->interface);
>> +
>> + if (priv->id == ID_MT7530) {
>> + /* Setup RX circuit, relevant PAD and driving on the
>> + * host which must be placed after the setup on the
>> + * device side is all finished.
>> + */
>> + mt7623_pad_clk_setup(ds);
>> + }
>> + break;
>> + default:
>> + dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port);
>> + return;
>> + }
>> +
>> + if (!state->an_enabled || mode == MLO_AN_FIXED) {
>> + mcr |= PMCR_FORCE_MODE;
>> +
>> + if (state->speed == SPEED_1000)
>> + mcr |= PMCR_FORCE_SPEED_1000;
>> + if (state->speed == SPEED_100)
>> + mcr |= PMCR_FORCE_SPEED_100;
>> + if (state->duplex == DUPLEX_FULL)
>> + mcr |= PMCR_FORCE_FDX;
>> + if (state->link || mode == MLO_AN_FIXED)
>> + mcr |= PMCR_FORCE_LNK;
>
> This should be removed - state->link is not for use in mac_config.
> Even in fixed mode, the link can be brought up/down by means of a
> gpio, and this should be dealt with via the mac_link_* functions.
Maybe I understand it wrong, but is it the intention that in
phylink_mac_config with modes MLO_AN_FIXED and MLO_AN_PHY the MAC is always
forces into a certain speed/mode/interface. So it never auto-negotiate because
phylink select the best configuration for you?
Also the PMCR_FORCE_LNK is only set in phylink_link_up() or can I do it here
and do nothing phylink_link_up()?
Other question:
Where does the MAC enable/disable TX and RX fits best? port_{enable,disable}?
Or only mac_config() and port_disable?
>
>> + if (state->pause || phylink_test(state->advertising, Pause))
>> + mcr |= PMCR_TX_FC_EN | PMCR_RX_FC_EN;
>> + if (state->pause & MLO_PAUSE_TX)
>> + mcr |= PMCR_TX_FC_EN;
>> + if (state->pause & MLO_PAUSE_RX)
>> + mcr |= PMCR_RX_FC_EN;
>
> This is clearly wrong - if any bit in state->pause is set, then we
> end up with both PMCR_TX_FC_EN | PMCR_RX_FC_EN set. If we have Pause
> Pause set in the advertising mask, then both are set. This doesn't
> seem right - are these bits setting the advertisement, or are they
> telling the MAC to use flow control?
Last one, tell the MAC to use flow control.
Qoute of the datasheet:
If you want to disable TX and RX flow control disable bit 4 and 5
MAC Control Register:
BIT(4): Force TX FC:
0: Disabled
1: Let the MAC transmit a pause frame when operating in full-duplex
mode with low internal free memory page count.
BIT(5): Force RX FC:
0: Disabled
1: Let the MA accept a pause frame when operating in full-duplex
mode.
On the current driver both bits are set in a forced-link situation.
If we always forces the MAC mode I think I always set these bits and don't
anything with the Pause modes? Is that the right way to do it?
>
>> + }
>> +
>> + mt7530_write(priv, MT7530_PMCR_P(port), mcr);
>> +
>> + return;
>> +
>> +unsupported:
>> + dev_err(ds->dev, "%s: P%d: Unsupported phy_interface mode: %d (%s)\n",
>> + __func__, port, state->interface, phy_modes(state->interface));
>> +}
>> +
>> +static void mt7530_phylink_mac_link_down(struct dsa_switch *ds, int port,
>> + unsigned int mode,
>> + phy_interface_t interface)
>> +{
>> + /* Do nothing */
>> +}
>> +
>> +static void mt7530_phylink_mac_link_up(struct dsa_switch *ds, int port,
>> + unsigned int mode,
>> + phy_interface_t interface,
>> + struct phy_device *phydev)
>> +{
>> + /* Do nothing */
>> +}
>
> These two are where you should be forcing the link up or down if
> required (basically, inband modes should let the link come up/down
> irrespective of these functions, otherwise it should be forced.)
>
OK
>> +
>> +static void mt7530_phylink_validate(struct dsa_switch *ds, int port,
>> + unsigned long *supported,
>> + struct phylink_link_state *state)
>> +{
>> + __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
>> +
>> + switch (port) {
>> + case 0: /* Internal phy */
>> + case 1:
>> + case 2:
>> + case 3:
>> + case 4:
>> + if (state->interface != PHY_INTERFACE_MODE_NA &&
>> + state->interface != PHY_INTERFACE_MODE_GMII)
>> + goto unsupported;
>> + break;
>> + /* case 5: Port 5 not supported! */
>> + case 6: /* 1st cpu port */
>> + if (state->interface != PHY_INTERFACE_MODE_RGMII &&
>> + state->interface != PHY_INTERFACE_MODE_TRGMII)
>
> PHY_INTERFACE_MODE_NA ?
You mean PHY_INTERFACE_MODE_NA is missing?
PHY_INTERFACE_MODE_TRGMII is a valid mode for this port.
>
>> + goto unsupported;
>> + break;
>> + default:
>> + linkmode_zero(supported);
>> + dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port);
>> + return;
>> + }
>> +
>> + phylink_set(mask, Autoneg);
>> + phylink_set(mask, Pause);
>> + phylink_set(mask, Asym_Pause);
>> + phylink_set(mask, MII);
>> +
>> + phylink_set(mask, 10baseT_Half);
>> + phylink_set(mask, 10baseT_Full);
>> + phylink_set(mask, 100baseT_Half);
>> + phylink_set(mask, 100baseT_Full);
>> + phylink_set(mask, 1000baseT_Full);
>> + phylink_set(mask, 1000baseT_Half);
>
> You seem to be missing phylink_set_port_modes() here.
OK
>
>> +
>> + linkmode_and(supported, supported, mask);
>> + linkmode_and(state->advertising, state->advertising, mask);
>> + return;
>> +
>> +unsupported:
>> + linkmode_zero(supported);
>> + dev_err(ds->dev, "%s: unsupported interface mode: [0x%x] %s\n",
>> + __func__, state->interface, phy_modes(state->interface));
>
> Not a good idea to print this at error level; sometimes we just probe
> for support.
>
> Eg, think about a SFP cage, and a SFP is plugged in that uses a PHY
> interface mode that the MAC can't support - we detect that by the
> validation failing, and printing a more meaningful message in phylink
> itself.
OK, It will be removed.
>
>> +}
>> +
>> +static int
>> +mt7530_phylink_mac_link_state(struct dsa_switch *ds, int port,
>> + struct phylink_link_state *state)
>> +{
>> + struct mt7530_priv *priv = ds->priv;
>> + u32 pmsr;
>> +
>> + if (port < 0 || port >= MT7530_NUM_PORTS)
>> + return -EINVAL;
>> +
>> + pmsr = mt7530_read(priv, MT7530_PMSR_P(port));
>> +
>> + state->link = (pmsr & PMSR_LINK);
>> + state->an_complete = state->link;
>> + state->duplex = (pmsr & PMSR_DPX) >> 1;
>> +
>> + switch (pmsr & (PMSR_SPEED_1000 | PMSR_SPEED_100)) {
>> + case 0:
>> + state->speed = SPEED_10;
>> + break;
>> + case PMSR_SPEED_100:
>> + state->speed = SPEED_100;
>> + break;
>> + case PMSR_SPEED_1000:
>> + state->speed = SPEED_1000;
>> + break;
>> + default:
>> + state->speed = SPEED_UNKNOWN;
>> + break;
>> + }
>> +
>> + state->pause = 0;
>> + if (pmsr & PMSR_RX_FC)
>> + state->pause |= MLO_PAUSE_RX;
>> + if (pmsr & PMSR_TX_FC)
>> + state->pause |= MLO_PAUSE_TX;
>> +
>> + return 1;
>> +}
>> +
>> static const struct dsa_switch_ops mt7530_switch_ops = {
>> .get_tag_protocol = mtk_get_tag_protocol,
>> .setup = mt7530_setup,
>> @@ -1331,7 +1446,6 @@ static const struct dsa_switch_ops
>> mt7530_switch_ops = {
>> .phy_write = mt7530_phy_write,
>> .get_ethtool_stats = mt7530_get_ethtool_stats,
>> .get_sset_count = mt7530_get_sset_count,
>> - .adjust_link = mt7530_adjust_link,
>> .port_enable = mt7530_port_enable,
>> .port_disable = mt7530_port_disable,
>> .port_stp_state_set = mt7530_stp_state_set,
>> @@ -1344,6 +1458,11 @@ static const struct dsa_switch_ops
>> mt7530_switch_ops = {
>> .port_vlan_prepare = mt7530_port_vlan_prepare,
>> .port_vlan_add = mt7530_port_vlan_add,
>> .port_vlan_del = mt7530_port_vlan_del,
>> + .phylink_validate = mt7530_phylink_validate,
>> + .phylink_mac_link_state = mt7530_phylink_mac_link_state,
>> + .phylink_mac_config = mt7530_phylink_mac_config,
>> + .phylink_mac_link_down = mt7530_phylink_mac_link_down,
>> + .phylink_mac_link_up = mt7530_phylink_mac_link_up,
>> };
>>
>> static const struct of_device_id mt7530_of_match[] = {
>> diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
>> index bfac90f48102..41d9a132ac70 100644
>> --- a/drivers/net/dsa/mt7530.h
>> +++ b/drivers/net/dsa/mt7530.h
>> @@ -198,6 +198,7 @@ enum mt7530_vlan_port_attr {
>> #define PMCR_FORCE_SPEED_100 BIT(2)
>> #define PMCR_FORCE_FDX BIT(1)
>> #define PMCR_FORCE_LNK BIT(0)
>> +#define PMCR_FORCE_LNK_DOWN PMCR_FORCE_MODE
>> #define PMCR_COMMON_LINK (PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | \
>> PMCR_BACKOFF_EN | PMCR_BACKPR_EN | \
>> PMCR_TX_EN | PMCR_RX_EN | \
>> @@ -218,6 +219,14 @@ enum mt7530_vlan_port_attr {
>> PMCR_TX_FC_EN | PMCR_RX_FC_EN)
>>
>> #define MT7530_PMSR_P(x) (0x3008 + (x) * 0x100)
>> +#define PMSR_EEE1G BIT(7)
>> +#define PMSR_EEE100M BIT(6)
>> +#define PMSR_RX_FC BIT(5)
>> +#define PMSR_TX_FC BIT(4)
>> +#define PMSR_SPEED_1000 BIT(3)
>> +#define PMSR_SPEED_100 BIT(2)
>> +#define PMSR_DPX BIT(1)
>> +#define PMSR_LINK BIT(0)
>>
>> /* Register for MIB */
>> #define MT7530_PORT_MIB_COUNTER(x) (0x4000 + (x) * 0x100)
>> --
>> 2.20.1
>>
>>
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> According to speedtest.net: 11.9Mbps down 500kbps up
Greats,
René
^ permalink raw reply
* Re: [PATCH iproute2] iproute: Pass RTM_F_CLONED on dump to fetch cached routes to be flushed
From: Stefano Brivio @ 2019-06-25 11:40 UTC (permalink / raw)
To: David Ahern
Cc: Stephen Hemminger, David Miller, Jianlin Shi, Wei Wang,
Martin KaFai Lau, Eric Dumazet, Matti Vaittinen, netdev
In-Reply-To: <209bcb66-2d57-eecf-d1a0-cc86af034e95@gmail.com>
On Mon, 24 Jun 2019 15:55:49 -0600
David Ahern <dsahern@gmail.com> wrote:
> On 6/14/19 7:33 PM, Stefano Brivio wrote:
> > diff --git a/ip/iproute.c b/ip/iproute.c
> > index 2b3dcc5dbd53..192442b42062 100644
> > --- a/ip/iproute.c
> > +++ b/ip/iproute.c
> > @@ -1602,6 +1602,16 @@ static int save_route_prep(void)
> > return 0;
> > }
> >
> > +static int iproute_flush_flags(struct nlmsghdr *nlh, int reqlen)
>
> rename that to iproute_flush_filter to be consistent with
> iproute_dump_filter.
I originally wanted to make it obvious that it's not an actual filter,
but:
> Actually, why can't the flush code use iproute_dump_filter?
...come on. That would be too simple.
No, my original understanding was that strict checking didn't imply
filtering. It does, and the current kernel implementation matches,
now also for RTM_F_CACHED. So yes, we can use it, and it doesn't cause
any unexpected behaviours with older kernels either. Sending v2. Thanks
for pointing this out.
--
Stefano
^ permalink raw reply
* [PATCH iproute2 v2] iproute: Set flags and attributes on dump to get IPv6 cached routes to be flushed
From: Stefano Brivio @ 2019-06-25 11:41 UTC (permalink / raw)
To: Stephen Hemminger
Cc: David Miller, Jianlin Shi, Wei Wang, David Ahern,
Martin KaFai Lau, Eric Dumazet, Matti Vaittinen, netdev
With a current (5.1) kernel version, IPv6 exception routes can't be listed
(ip -6 route list cache) or flushed (ip -6 route flush cache). Kernel
support for this is being added back. Relevant net-next commits:
564c91f7e563 fib_frontend, ip6_fib: Select routes or exceptions dump from RTM_F_CLONED
ef11209d4219 Revert "net/ipv6: Bail early if user only wants cloned entries"
3401bfb1638e ipv6/route: Don't match on fc_nh_id if not set in ip6_route_del()
bf9a8a061ddc ipv6/route: Change return code of rt6_dump_route() for partial node dumps
1e47b4837f3b ipv6: Dump route exceptions if requested
40cb35d5dc04 ip6_fib: Don't discard nodes with valid routing information in fib6_locate_1()
However, to allow the kernel to filter routes based on the RTM_F_CLONED
flag, we need to make sure this flag is always passed when we want cached
routes to be dumped, and we can also pass table and output interface
attributes to have the kernel filtering on them, if requested by the user.
Use the existing iproute_dump_filter() as a filter for the dump request in
iproute_flush(). This way, 'ip -6 route flush cache' works again.
v2: Instead of creating a separate 'filter' function dealing with
RTM_F_CACHED only, use the existing iproute_dump_filter() and get
table and oif kernel filtering for free. Suggested by David Ahern.
Fixes: aba5acdfdb34 ("(Logical change 1.3)")
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
ip/iproute.c | 50 +++++++++++++++++++++++++-------------------------
1 file changed, 25 insertions(+), 25 deletions(-)
diff --git a/ip/iproute.c b/ip/iproute.c
index 2b3dcc5dbd53..1669e0138259 100644
--- a/ip/iproute.c
+++ b/ip/iproute.c
@@ -1602,6 +1602,30 @@ static int save_route_prep(void)
return 0;
}
+static int iproute_dump_filter(struct nlmsghdr *nlh, int reqlen)
+{
+ struct rtmsg *rtm = NLMSG_DATA(nlh);
+ int err;
+
+ rtm->rtm_protocol = filter.protocol;
+ if (filter.cloned)
+ rtm->rtm_flags |= RTM_F_CLONED;
+
+ if (filter.tb) {
+ err = addattr32(nlh, reqlen, RTA_TABLE, filter.tb);
+ if (err)
+ return err;
+ }
+
+ if (filter.oif) {
+ err = addattr32(nlh, reqlen, RTA_OIF, filter.oif);
+ if (err)
+ return err;
+ }
+
+ return 0;
+}
+
static int iproute_flush(int family, rtnl_filter_t filter_fn)
{
time_t start = time(0);
@@ -1624,7 +1648,7 @@ static int iproute_flush(int family, rtnl_filter_t filter_fn)
filter.flushe = sizeof(flushb);
for (;;) {
- if (rtnl_routedump_req(&rth, family, NULL) < 0) {
+ if (rtnl_routedump_req(&rth, family, iproute_dump_filter) < 0) {
perror("Cannot send dump request");
return -2;
}
@@ -1664,30 +1688,6 @@ static int iproute_flush(int family, rtnl_filter_t filter_fn)
}
}
-static int iproute_dump_filter(struct nlmsghdr *nlh, int reqlen)
-{
- struct rtmsg *rtm = NLMSG_DATA(nlh);
- int err;
-
- rtm->rtm_protocol = filter.protocol;
- if (filter.cloned)
- rtm->rtm_flags |= RTM_F_CLONED;
-
- if (filter.tb) {
- err = addattr32(nlh, reqlen, RTA_TABLE, filter.tb);
- if (err)
- return err;
- }
-
- if (filter.oif) {
- err = addattr32(nlh, reqlen, RTA_OIF, filter.oif);
- if (err)
- return err;
- }
-
- return 0;
-}
-
static int iproute_list_flush_or_save(int argc, char **argv, int action)
{
int dump_family = preferred_family;
--
2.20.1
^ permalink raw reply related
* [[PATCH net-next]] net: core: xdp: make __mem_id_disconnect to be static
From: Ivan Khoronzhuk @ 2019-06-25 11:42 UTC (permalink / raw)
To: netdev, davem; +Cc: xdp-newbies, linux-kernel, ast, hawk, Ivan Khoronzhuk
Add missed static for local __mem_id_disconnect().
Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
---
net/core/xdp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/core/xdp.c b/net/core/xdp.c
index b29d7b513a18..829377cc83db 100644
--- a/net/core/xdp.c
+++ b/net/core/xdp.c
@@ -85,7 +85,7 @@ static void __xdp_mem_allocator_rcu_free(struct rcu_head *rcu)
kfree(xa);
}
-bool __mem_id_disconnect(int id, bool force)
+static bool __mem_id_disconnect(int id, bool force)
{
struct xdp_mem_allocator *xa;
bool safe_to_remove = true;
--
2.17.1
^ permalink raw reply related
* Re: [PATCH RFC net-next 1/5] net: dsa: mt7530: Convert to PHYLINK API
From: René van Dorst @ 2019-06-25 11:43 UTC (permalink / raw)
To: Daniel Santos
Cc: sean.wang, f.fainelli, linux, davem, matthias.bgg, andrew,
vivien.didelot, frank-w, netdev, linux-mediatek, linux-mips
In-Reply-To: <13c67cb7-b33e-f2b1-9d1e-d2882e0ff076@pobox.com>
Quoting Daniel Santos <daniel.santos@pobox.com>:
Hi Daniel,
> On 6/24/19 9:52 AM, René van Dorst wrote:
>> Convert mt7530 to PHYLINK API
>>
>> Signed-off-by: René van Dorst <opensource@vdorst.com>
>> ---
>> drivers/net/dsa/mt7530.c | 237 +++++++++++++++++++++++++++++----------
>> drivers/net/dsa/mt7530.h | 9 ++
>> 2 files changed, 187 insertions(+), 59 deletions(-)
>>
>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>> index 3181e95586d6..9c5e4dd00826 100644
>> --- a/drivers/net/dsa/mt7530.c
>> +++ b/drivers/net/dsa/mt7530.c
>> @@ -13,7 +13,7 @@
>> #include <linux/of_mdio.h>
>> #include <linux/of_net.h>
>> #include <linux/of_platform.h>
>> -#include <linux/phy.h>
>> +#include <linux/phylink.h>
>> #include <linux/regmap.h>
>> #include <linux/regulator/consumer.h>
>> #include <linux/reset.h>
>> @@ -633,63 +633,6 @@ mt7530_get_sset_count(struct dsa_switch *ds,
>> int port, int sset)
>> return ARRAY_SIZE(mt7530_mib);
>> }
>>
>> -static void mt7530_adjust_link(struct dsa_switch *ds, int port,
>> - struct phy_device *phydev)
>> -{
>> - struct mt7530_priv *priv = ds->priv;
>> -
>> - if (phy_is_pseudo_fixed_link(phydev)) {
>> - dev_dbg(priv->dev, "phy-mode for master device = %x\n",
>> - phydev->interface);
>> -
>> - /* Setup TX circuit incluing relevant PAD and driving */
>> - mt7530_pad_clk_setup(ds, phydev->interface);
>> -
>> - if (priv->id == ID_MT7530) {
>> - /* Setup RX circuit, relevant PAD and driving on the
>> - * host which must be placed after the setup on the
>> - * device side is all finished.
>> - */
>> - mt7623_pad_clk_setup(ds);
>> - }
>> - } else {
>> - u16 lcl_adv = 0, rmt_adv = 0;
>> - u8 flowctrl;
>> - u32 mcr = PMCR_USERP_LINK | PMCR_FORCE_MODE;
>> -
>> - switch (phydev->speed) {
>> - case SPEED_1000:
>> - mcr |= PMCR_FORCE_SPEED_1000;
>> - break;
>> - case SPEED_100:
>> - mcr |= PMCR_FORCE_SPEED_100;
>> - break;
>> - }
>> -
>> - if (phydev->link)
>> - mcr |= PMCR_FORCE_LNK;
>> -
>> - if (phydev->duplex) {
>> - mcr |= PMCR_FORCE_FDX;
>> -
>> - if (phydev->pause)
>> - rmt_adv = LPA_PAUSE_CAP;
>> - if (phydev->asym_pause)
>> - rmt_adv |= LPA_PAUSE_ASYM;
>> -
>> - lcl_adv = linkmode_adv_to_lcl_adv_t(
>> - phydev->advertising);
>> - flowctrl = mii_resolve_flowctrl_fdx(lcl_adv, rmt_adv);
>> -
>> - if (flowctrl & FLOW_CTRL_TX)
>> - mcr |= PMCR_TX_FC_EN;
>> - if (flowctrl & FLOW_CTRL_RX)
>> - mcr |= PMCR_RX_FC_EN;
>> - }
>> - mt7530_write(priv, MT7530_PMCR_P(port), mcr);
>> - }
>> -}
>> -
>> static int
>> mt7530_cpu_port_enable(struct mt7530_priv *priv,
>> int port)
>> @@ -1323,6 +1266,178 @@ mt7530_setup(struct dsa_switch *ds)
>> return 0;
>> }
>>
>> +static void mt7530_phylink_mac_config(struct dsa_switch *ds, int port,
>> + unsigned int mode,
>> + const struct phylink_link_state *state)
>> +{
>> + struct mt7530_priv *priv = ds->priv;
>> + u32 mcr = PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | PMCR_BACKOFF_EN |
>> + PMCR_BACKPR_EN | PMCR_TX_EN | PMCR_RX_EN;
>> +
>> + switch (port) {
>> + case 0: /* Internal phy */
>> + case 1:
>> + case 2:
>> + case 3:
>> + case 4:
>> + if (state->interface != PHY_INTERFACE_MODE_GMII)
>> + goto unsupported;
>> + break;
>> + /* case 5: Port 5 is not supported! */
>> + case 6: /* 1st cpu port */
>> + if (state->interface != PHY_INTERFACE_MODE_RGMII &&
>> + state->interface != PHY_INTERFACE_MODE_TRGMII)
>> + goto unsupported;
>> +
>> + /* Setup TX circuit incluing relevant PAD and driving */
>> + mt7530_pad_clk_setup(ds, state->interface);
>> +
>> + if (priv->id == ID_MT7530) {
>> + /* Setup RX circuit, relevant PAD and driving on the
>> + * host which must be placed after the setup on the
>> + * device side is all finished.
>> + */
>> + mt7623_pad_clk_setup(ds);
>> + }
>> + break;
>> + default:
>> + dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port);
>> + return;
>> + }
>> +
>> + if (!state->an_enabled || mode == MLO_AN_FIXED) {
>> + mcr |= PMCR_FORCE_MODE;
>> +
>> + if (state->speed == SPEED_1000)
>> + mcr |= PMCR_FORCE_SPEED_1000;
>> + if (state->speed == SPEED_100)
>> + mcr |= PMCR_FORCE_SPEED_100;
>
> I would suggest using the defines
>
> #define PMCR_FORCE_SPEED 0x0000000c /* or PMCR_FORCE_SPEED_MASK */
> #define PMCR_FORCE_SPEED_10 0x00000000
> #define PMCR_FORCE_SPEED_100 0x00000004
> #define PMCR_FORCE_SPEED_1000 0x00000008
>
> and a switch statement such as
>
> switch (state->speed) {
> case SPEED_10:
> mcr |= PMCR_FORCE_SPEED_10;
> break;
> case SPEED_100:
> mcr |= PMCR_FORCE_SPEED_100;
> break;
> case SPEED_1000:
> mcr |= PMCR_FORCE_SPEED_1000;
> break;
> }
>
> This will compile the same (i.e, the mcr |= 0 will optimize away, etc.),
> while alleviating the need to intimately know the hardware in order to
> easily understand what the code is doing at a glance. It's also better
> form as we're treating the two bits as a composite value, rather than
> two separate bits.
I will change it based on your surgestion.
>
>
>> + if (state->duplex == DUPLEX_FULL)
>> + mcr |= PMCR_FORCE_FDX;
>> + if (state->link || mode == MLO_AN_FIXED)
>> + mcr |= PMCR_FORCE_LNK;
>> + if (state->pause || phylink_test(state->advertising, Pause))
>> + mcr |= PMCR_TX_FC_EN | PMCR_RX_FC_EN;
>> + if (state->pause & MLO_PAUSE_TX)
>> + mcr |= PMCR_TX_FC_EN;
>> + if (state->pause & MLO_PAUSE_RX)
>> + mcr |= PMCR_RX_FC_EN;
>> + }
>> +
>> + mt7530_write(priv, MT7530_PMCR_P(port), mcr);
>> +
>> + return;
>> +
>> +unsupported:
>> + dev_err(ds->dev, "%s: P%d: Unsupported phy_interface mode: %d (%s)\n",
>> + __func__, port, state->interface, phy_modes(state->interface));
>> +}
>> +
>> +static void mt7530_phylink_mac_link_down(struct dsa_switch *ds, int port,
>> + unsigned int mode,
>> + phy_interface_t interface)
>> +{
>> + /* Do nothing */
>> +}
>> +
>> +static void mt7530_phylink_mac_link_up(struct dsa_switch *ds, int port,
>> + unsigned int mode,
>> + phy_interface_t interface,
>> + struct phy_device *phydev)
>> +{
>> + /* Do nothing */
>> +}
>> +
>> +static void mt7530_phylink_validate(struct dsa_switch *ds, int port,
>> + unsigned long *supported,
>> + struct phylink_link_state *state)
>> +{
>> + __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
>> +
>> + switch (port) {
>> + case 0: /* Internal phy */
>> + case 1:
>> + case 2:
>> + case 3:
>> + case 4:
>> + if (state->interface != PHY_INTERFACE_MODE_NA &&
>> + state->interface != PHY_INTERFACE_MODE_GMII)
>> + goto unsupported;
>> + break;
>> + /* case 5: Port 5 not supported! */
>> + case 6: /* 1st cpu port */
>> + if (state->interface != PHY_INTERFACE_MODE_RGMII &&
>> + state->interface != PHY_INTERFACE_MODE_TRGMII)
>> + goto unsupported;
>> + break;
>> + default:
>> + linkmode_zero(supported);
>> + dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port);
>> + return;
>> + }
>> +
>> + phylink_set(mask, Autoneg);
>> + phylink_set(mask, Pause);
>> + phylink_set(mask, Asym_Pause);
>> + phylink_set(mask, MII);
>> +
>> + phylink_set(mask, 10baseT_Half);
>> + phylink_set(mask, 10baseT_Full);
>> + phylink_set(mask, 100baseT_Half);
>> + phylink_set(mask, 100baseT_Full);
>> + phylink_set(mask, 1000baseT_Full);
>> + phylink_set(mask, 1000baseT_Half);
>> +
>> + linkmode_and(supported, supported, mask);
>> + linkmode_and(state->advertising, state->advertising, mask);
>> + return;
>> +
>> +unsupported:
>> + linkmode_zero(supported);
>> + dev_err(ds->dev, "%s: unsupported interface mode: [0x%x] %s\n",
>> + __func__, state->interface, phy_modes(state->interface));
>> +}
>> +
>> +static int
>> +mt7530_phylink_mac_link_state(struct dsa_switch *ds, int port,
>> + struct phylink_link_state *state)
>> +{
>> + struct mt7530_priv *priv = ds->priv;
>> + u32 pmsr;
>> +
>> + if (port < 0 || port >= MT7530_NUM_PORTS)
>> + return -EINVAL;
>> +
>> + pmsr = mt7530_read(priv, MT7530_PMSR_P(port));
>> +
>> + state->link = (pmsr & PMSR_LINK);
>> + state->an_complete = state->link;
>> + state->duplex = (pmsr & PMSR_DPX) >> 1;
>> +
>> + switch (pmsr & (PMSR_SPEED_1000 | PMSR_SPEED_100)) {
>> + case 0:
>> + state->speed = SPEED_10;
>> + break;
>> + case PMSR_SPEED_100:
>> + state->speed = SPEED_100;
>> + break;
>> + case PMSR_SPEED_1000:
>> + state->speed = SPEED_1000;
>> + break;
>> + default:
>> + state->speed = SPEED_UNKNOWN;
>> + break;
>> + }
>> +
>
> Same as above: add PMSR_SPEED_10, and and with PMSR_SPEED (or
> PMSR_SPEED_MASK if you prefer).
Same as above.
>
>> + state->pause = 0;
>> + if (pmsr & PMSR_RX_FC)
>> + state->pause |= MLO_PAUSE_RX;
>> + if (pmsr & PMSR_TX_FC)
>> + state->pause |= MLO_PAUSE_TX;
>> +
>> + return 1;
>> +}
>> +
>> static const struct dsa_switch_ops mt7530_switch_ops = {
>> .get_tag_protocol = mtk_get_tag_protocol,
>> .setup = mt7530_setup,
>> @@ -1331,7 +1446,6 @@ static const struct dsa_switch_ops
>> mt7530_switch_ops = {
>> .phy_write = mt7530_phy_write,
>> .get_ethtool_stats = mt7530_get_ethtool_stats,
>> .get_sset_count = mt7530_get_sset_count,
>> - .adjust_link = mt7530_adjust_link,
>> .port_enable = mt7530_port_enable,
>> .port_disable = mt7530_port_disable,
>> .port_stp_state_set = mt7530_stp_state_set,
>> @@ -1344,6 +1458,11 @@ static const struct dsa_switch_ops
>> mt7530_switch_ops = {
>> .port_vlan_prepare = mt7530_port_vlan_prepare,
>> .port_vlan_add = mt7530_port_vlan_add,
>> .port_vlan_del = mt7530_port_vlan_del,
>> + .phylink_validate = mt7530_phylink_validate,
>> + .phylink_mac_link_state = mt7530_phylink_mac_link_state,
>> + .phylink_mac_config = mt7530_phylink_mac_config,
>> + .phylink_mac_link_down = mt7530_phylink_mac_link_down,
>> + .phylink_mac_link_up = mt7530_phylink_mac_link_up,
>> };
>>
>> static const struct of_device_id mt7530_of_match[] = {
>> diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
>> index bfac90f48102..41d9a132ac70 100644
>> --- a/drivers/net/dsa/mt7530.h
>> +++ b/drivers/net/dsa/mt7530.h
>> @@ -198,6 +198,7 @@ enum mt7530_vlan_port_attr {
>> #define PMCR_FORCE_SPEED_100 BIT(2)
>> #define PMCR_FORCE_FDX BIT(1)
>> #define PMCR_FORCE_LNK BIT(0)
>> +#define PMCR_FORCE_LNK_DOWN PMCR_FORCE_MODE
>> #define PMCR_COMMON_LINK (PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | \
>> PMCR_BACKOFF_EN | PMCR_BACKPR_EN | \
>> PMCR_TX_EN | PMCR_RX_EN | \
>> @@ -218,6 +219,14 @@ enum mt7530_vlan_port_attr {
>> PMCR_TX_FC_EN | PMCR_RX_FC_EN)
>>
>> #define MT7530_PMSR_P(x) (0x3008 + (x) * 0x100)
>> +#define PMSR_EEE1G BIT(7)
>> +#define PMSR_EEE100M BIT(6)
>> +#define PMSR_RX_FC BIT(5)
>> +#define PMSR_TX_FC BIT(4)
>> +#define PMSR_SPEED_1000 BIT(3)
>> +#define PMSR_SPEED_100 BIT(2)
>> +#define PMSR_DPX BIT(1)
>> +#define PMSR_LINK BIT(0)
>>
>> /* Register for MIB */
>> #define MT7530_PORT_MIB_COUNTER(x) (0x4000 + (x) * 0x100)
>
> Cheers,
> Daniel
Greats,
René
^ permalink raw reply
* [PATCH iproute2 1/2] devlink: fix format string warning for 32bit targets
From: Baruch Siach @ 2019-06-25 11:49 UTC (permalink / raw)
To: Stephen Hemminger, Jiri Pirko
Cc: netdev, Baruch Siach, Aya Levin, Moshe Shemesh
32bit targets define uint64_t as long long unsigned. This leads to the
following build warning:
devlink.c: In function ‘pr_out_u64’:
devlink.c:1729:11: warning: format ‘%lu’ expects argument of type ‘long unsigned int’, but argument 4 has type ‘uint64_t {aka long long unsigned int}’ [-Wformat=]
pr_out("%s %lu", name, val);
^
devlink.c:59:21: note: in definition of macro ‘pr_out’
fprintf(stdout, ##args); \
^~~~
Use 'll' length modifier in the format string to fix that.
Cc: Aya Levin <ayal@mellanox.com>
Cc: Moshe Shemesh <moshe@mellanox.com>
Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
devlink/devlink.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/devlink/devlink.c b/devlink/devlink.c
index 436935f88bda..b400fab17578 100644
--- a/devlink/devlink.c
+++ b/devlink/devlink.c
@@ -1726,9 +1726,9 @@ static void pr_out_u64(struct dl *dl, const char *name, uint64_t val)
jsonw_u64_field(dl->jw, name, val);
} else {
if (g_indent_newline)
- pr_out("%s %lu", name, val);
+ pr_out("%s %llu", name, val);
else
- pr_out(" %s %lu", name, val);
+ pr_out(" %s %llu", name, val);
}
}
@@ -1753,7 +1753,7 @@ static void pr_out_uint64_value(struct dl *dl, uint64_t value)
if (dl->json_output)
jsonw_u64(dl->jw, value);
else
- pr_out(" %lu", value);
+ pr_out(" %llu", value);
}
static void pr_out_binary_value(struct dl *dl, uint8_t *data, uint32_t len)
--
2.20.1
^ permalink raw reply related
* [PATCH iproute2 2/2] devlink: fix libc and kernel headers collision
From: Baruch Siach @ 2019-06-25 11:49 UTC (permalink / raw)
To: Stephen Hemminger, Jiri Pirko
Cc: netdev, Baruch Siach, Aya Levin, Moshe Shemesh
In-Reply-To: <016aabe2639668b4710b73157ea39e8f97f7d726.1561463345.git.baruch@tkos.co.il>
Since commit 2f1242efe9d ("devlink: Add devlink health show command") we
use the sys/sysinfo.h header for the sysinfo(2) system call. But since
iproute2 carries a local version of the kernel struct sysinfo, this
causes a collision with libc that do not rely on kernel defined sysinfo
like musl libc:
In file included from devlink.c:25:0:
.../sysroot/usr/include/sys/sysinfo.h:10:8: error: redefinition of 'struct sysinfo'
struct sysinfo {
^~~~~~~
In file included from ../include/uapi/linux/kernel.h:5:0,
from ../include/uapi/linux/netlink.h:5,
from ../include/uapi/linux/genetlink.h:6,
from devlink.c:21:
../include/uapi/linux/sysinfo.h:8:8: note: originally defined here
struct sysinfo {
^~~~~~~
Move the sys/sysinfo.h userspace header before kernel headers, and
suppress the indirect include of linux/sysinfo.h.
Cc: Aya Levin <ayal@mellanox.com>
Cc: Moshe Shemesh <moshe@mellanox.com>
Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
devlink/devlink.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/devlink/devlink.c b/devlink/devlink.c
index b400fab17578..2ea60d3556da 100644
--- a/devlink/devlink.c
+++ b/devlink/devlink.c
@@ -18,11 +18,16 @@
#include <limits.h>
#include <errno.h>
#include <inttypes.h>
+#include <sys/sysinfo.h>
+/* Suppress linux/sysinfo.h to avoid
+ * collision of struct sysinfo definition
+ * with musl libc headers
+ */
+#define _LINUX_SYSINFO_H
#include <linux/genetlink.h>
#include <linux/devlink.h>
#include <libmnl/libmnl.h>
#include <netinet/ether.h>
-#include <sys/sysinfo.h>
#include <sys/queue.h>
#include "SNAPSHOT.h"
--
2.20.1
^ permalink raw reply related
* Re: [PATCH net-next v2 08/12] xdp: tracking page_pool resources and safe removal
From: Ivan Khoronzhuk @ 2019-06-25 11:51 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: netdev, Ilias Apalodimas, Toke Høiland-Jørgensen,
Tariq Toukan, toshiaki.makita1, grygorii.strashko, mcroce
In-Reply-To: <20190625132750.06939133@carbon>
On Tue, Jun 25, 2019 at 01:27:50PM +0200, Jesper Dangaard Brouer wrote:
>On Tue, 25 Jun 2019 13:50:14 +0300
>Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
>
>> Hi Jesper,
>>
>> Could you please clarify one question.
>>
>> On Tue, Jun 18, 2019 at 03:05:47PM +0200, Jesper Dangaard Brouer wrote:
>> >This patch is needed before we can allow drivers to use page_pool for
>> >DMA-mappings. Today with page_pool and XDP return API, it is possible to
>> >remove the page_pool object (from rhashtable), while there are still
>> >in-flight packet-pages. This is safely handled via RCU and failed lookups in
>> >__xdp_return() fallback to call put_page(), when page_pool object is gone.
>> >In-case page is still DMA mapped, this will result in page note getting
>> >correctly DMA unmapped.
>> >
>> >To solve this, the page_pool is extended with tracking in-flight pages. And
>> >XDP disconnect system queries page_pool and waits, via workqueue, for all
>> >in-flight pages to be returned.
>> >
>> >To avoid killing performance when tracking in-flight pages, the implement
>> >use two (unsigned) counters, that in placed on different cache-lines, and
>> >can be used to deduct in-flight packets. This is done by mapping the
>> >unsigned "sequence" counters onto signed Two's complement arithmetic
>> >operations. This is e.g. used by kernel's time_after macros, described in
>> >kernel commit 1ba3aab3033b and 5a581b367b5, and also explained in RFC1982.
>> >
>> >The trick is these two incrementing counters only need to be read and
>> >compared, when checking if it's safe to free the page_pool structure. Which
>> >will only happen when driver have disconnected RX/alloc side. Thus, on a
>> >non-fast-path.
>> >
>> >It is chosen that page_pool tracking is also enabled for the non-DMA
>> >use-case, as this can be used for statistics later.
>> >
>> >After this patch, using page_pool requires more strict resource "release",
>> >e.g. via page_pool_release_page() that was introduced in this patchset, and
>> >previous patches implement/fix this more strict requirement.
>> >
>> >Drivers no-longer call page_pool_destroy(). Drivers already call
>> >xdp_rxq_info_unreg() which call xdp_rxq_info_unreg_mem_model(), which will
>> >attempt to disconnect the mem id, and if attempt fails schedule the
>> >disconnect for later via delayed workqueue.
>> >
>> >Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
>> >Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>> >---
>> > drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 3 -
>> > include/net/page_pool.h | 41 ++++++++++---
>> > net/core/page_pool.c | 62 +++++++++++++++-----
>> > net/core/xdp.c | 65 +++++++++++++++++++--
>> > 4 files changed, 136 insertions(+), 35 deletions(-)
>> >
>> >diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>> >index 2f647be292b6..6c9d4d7defbc 100644
>> >--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>> >+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>> >@@ -643,9 +643,6 @@ static void mlx5e_free_rq(struct mlx5e_rq *rq)
>> > }
>> >
>> > xdp_rxq_info_unreg(&rq->xdp_rxq);
>> >- if (rq->page_pool)
>> >- page_pool_destroy(rq->page_pool);
>> >-
>> > mlx5_wq_destroy(&rq->wq_ctrl);
>> > }
>> >
>> >diff --git a/include/net/page_pool.h b/include/net/page_pool.h
>> >index 754d980700df..f09b3f1994e6 100644
>> >--- a/include/net/page_pool.h
>> >+++ b/include/net/page_pool.h
>> >@@ -16,14 +16,16 @@
>> > * page_pool_alloc_pages() call. Drivers should likely use
>> > * page_pool_dev_alloc_pages() replacing dev_alloc_pages().
>> > *
>> >- * If page_pool handles DMA mapping (use page->private), then API user
>> >- * is responsible for invoking page_pool_put_page() once. In-case of
>> >- * elevated refcnt, the DMA state is released, assuming other users of
>> >- * the page will eventually call put_page().
>> >+ * API keeps track of in-flight pages, in-order to let API user know
>> >+ * when it is safe to dealloactor page_pool object. Thus, API users
>> >+ * must make sure to call page_pool_release_page() when a page is
>> >+ * "leaving" the page_pool. Or call page_pool_put_page() where
>> >+ * appropiate. For maintaining correct accounting.
>> > *
>> >- * If no DMA mapping is done, then it can act as shim-layer that
>> >- * fall-through to alloc_page. As no state is kept on the page, the
>> >- * regular put_page() call is sufficient.
>> >+ * API user must only call page_pool_put_page() once on a page, as it
>> >+ * will either recycle the page, or in case of elevated refcnt, it
>> >+ * will release the DMA mapping and in-flight state accounting. We
>> >+ * hope to lift this requirement in the future.
>> > */
>> > #ifndef _NET_PAGE_POOL_H
>> > #define _NET_PAGE_POOL_H
>> >@@ -66,9 +68,10 @@ struct page_pool_params {
>> > };
>> >
>> > struct page_pool {
>> >- struct rcu_head rcu;
>> > struct page_pool_params p;
>> >
>> >+ u32 pages_state_hold_cnt;
>> >+
>> > /*
>> > * Data structure for allocation side
>> > *
>> >@@ -96,6 +99,8 @@ struct page_pool {
>> > * TODO: Implement bulk return pages into this structure.
>> > */
>> > struct ptr_ring ring;
>> >+
>> >+ atomic_t pages_state_release_cnt;
>> > };
>> >
>> > struct page *page_pool_alloc_pages(struct page_pool *pool, gfp_t gfp);
>> >@@ -109,8 +114,6 @@ static inline struct page *page_pool_dev_alloc_pages(struct page_pool *pool)
>> >
>> > struct page_pool *page_pool_create(const struct page_pool_params *params);
>> >
>> >-void page_pool_destroy(struct page_pool *pool);
>> >-
>> > void __page_pool_free(struct page_pool *pool);
>> > static inline void page_pool_free(struct page_pool *pool)
>> > {
>> >@@ -143,6 +146,24 @@ static inline void page_pool_recycle_direct(struct page_pool *pool,
>> > __page_pool_put_page(pool, page, true);
>> > }
>> >
>> >+/* API user MUST have disconnected alloc-side (not allowed to call
>> >+ * page_pool_alloc_pages()) before calling this. The free-side can
>> >+ * still run concurrently, to handle in-flight packet-pages.
>> >+ *
>> >+ * A request to shutdown can fail (with false) if there are still
>> >+ * in-flight packet-pages.
>> >+ */
>> >+bool __page_pool_request_shutdown(struct page_pool *pool);
>> >+static inline bool page_pool_request_shutdown(struct page_pool *pool)
>> >+{
>> >+ /* When page_pool isn't compiled-in, net/core/xdp.c doesn't
>> >+ * allow registering MEM_TYPE_PAGE_POOL, but shield linker.
>> >+ */
>> >+#ifdef CONFIG_PAGE_POOL
>> >+ return __page_pool_request_shutdown(pool);
>> >+#endif
>> >+}
>>
>> The free side can ran in softirq, that means fast cache recycle is accessed.
>> And it increments not atomic pool->alloc.count.
>>
>> For instance While redirect, for remote interface, while .ndo_xdp_xmit the
>> xdp_return_frame_rx_napi(xdpf) is called everywhere in error path ....
^
|
>>
>> In the same time, simultaneously, the work queue can try one more
>> time to clear cash, calling __page_pool_request_shutdown()....
>>
>> Question, what prevents pool->alloc.count to be corrupted by race,
>> causing to wrong array num and as result wrong page to be unmapped/put ....or
>> even page leak. alloc.count usage is not protected,
>> __page_pool_request_shutdown() is called not from same rx NAPI, even not from
>> NAPI.
>>
>> Here, while alloc cache empty procedure in __page_pool_request_shutdown():
>
>You forgot to copy this comment, which explains:
>
> /* Empty alloc cache, assume caller made sure this is
> * no-longer in use, and page_pool_alloc_pages() cannot be
> * call concurrently.
> */
No I didn't. I'm talking about recycle, not allocation.
To be more specific about this:
__page_pool_recycle_direct() while remote ndev .ndo_xdp_xmit
About this:
"
/* API user MUST have disconnected alloc-side (not allowed to call
* page_pool_alloc_pages()) before calling this. The free-side can
* still run concurrently, to handle in-flight packet-pages.
"
>
>> while (pool->alloc.count) {
>> page = pool->alloc.cache[--pool->alloc.count];
>> __page_pool_return_page(pool, page);
>> }
>>
>> For me seems all works fine, but I can't find what have I missed?
>
>You have missed that, it is the drivers responsibility to "disconnect"
>the xdp_rxq_info before calling shutdown. Which means that it is not
>allowed to be used for RX, while the driver is shutting down a
>RX-queue. For drivers this is very natural, else other things will
>break.
>
>
>> ...
>>
>> Same question about how xdp frame should be returned for drivers running
>> tx napi exclusively, it can be still softirq but another CPU? What API
>> should be used to return xdp frame.
>
>You have to use the normal xdp_return_frame() which doesn't do "direct"
>return.
Oh, yes, sorry
/* mem->id is valid, checked in xdp_rxq_info_reg_mem_model() */
xa = rhashtable_lookup(mem_id_ht, &mem->id, mem_id_rht_params);
page = virt_to_head_page(data);
if (likely(xa)) {
napi_direct &= !xdp_return_frame_no_direct();
page_pool_put_page(xa->page_pool, page, napi_direct);
just masked napi_direct on 1, but forgot it's zero from scratch......
>
>--
>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 bpf-next] MAINTAINERS: add reviewer to maintainers entry
From: Daniel Borkmann @ 2019-06-25 12:00 UTC (permalink / raw)
To: Jonathan Lemon, Björn Töpel
Cc: ast, netdev, Björn Töpel, magnus.karlsson, bpf,
linux-kernel
In-Reply-To: <B5DAC105-8F88-43F7-9F6F-6C0B436C4F06@gmail.com>
On 06/24/2019 10:54 PM, Jonathan Lemon wrote:
> On 23 Jun 2019, at 22:24, Björn Töpel wrote:
>> From: Björn Töpel <bjorn.topel@intel.com>
>>
>> Jonathan Lemon has volunteered as an official AF_XDP reviewer. Thank
>> you, Jonathan!
>>
>> Signed-off-by: Björn Töpel <bjorn.topel@intel.com>
> Acked-by: Jonathan Lemon <jonathan.lemon@gmail.com>
Given it's a doc update, I've applied it to bpf, thanks!
^ permalink raw reply
* Re: [PATCH bpf-next] MAINTAINERS: add reviewer to maintainers entry
From: Björn Töpel @ 2019-06-25 12:02 UTC (permalink / raw)
To: Daniel Borkmann, Jonathan Lemon, Björn Töpel
Cc: ast, netdev, magnus.karlsson, bpf, linux-kernel
In-Reply-To: <e0654342-7110-c1e3-53bc-cb9c303252e9@iogearbox.net>
On 2019-06-25 14:00, Daniel Borkmann wrote:
> Given it's a doc update, I've applied it to bpf, thanks!
Perfect, thanks!
^ permalink raw reply
* Re: [PATCH net v2] net: mvpp2: prs: Don't override the sign bit in SRAM parser shift
From: Maxime Chevallier @ 2019-06-25 12:04 UTC (permalink / raw)
To: davem
Cc: netdev, linux-kernel, Antoine Tenart, thomas.petazzoni,
gregory.clement, nadavh, stefanc, mw, Alan Winkowski
In-Reply-To: <20190620094245.10501-1-maxime.chevallier@bootlin.com>
Hello David,
On Thu, 20 Jun 2019 11:42:45 +0200
Maxime Chevallier <maxime.chevallier@bootlin.com> wrote:
>The Header Parser allows identifying various fields in the packet
>headers, used for various kind of filtering and classification
>steps.
>
>This is a re-entrant process, where the offset in the packet header
>depends on the previous lookup results. This offset is represented in
>the SRAM results of the TCAM, as a shift to be operated.
>
>This shift can be negative in some cases, such as in IPv6 parsing.
>
>This commit prevents overriding the sign bit when setting the shift
>value, which could cause instabilities when parsing IPv6 flows.
>
>Fixes: 3f518509dedc ("ethernet: Add new driver for Marvell Armada 375 network unit")
>Suggested-by: Alan Winkowski <walan@marvell.com>
>Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
>---
>V2 : Fix a typo in the commit log, reported by Sergei.
I see that this patch was set as "Accepted" on patchwork, but hasn't
made it to -net, I was wondering if this patch slipped through the
cracks :)
https://patchwork.ozlabs.org/patch/1119311/
Thanks,
Maxime
^ permalink raw reply
* Re: [PATCH V3 07/10] net: dsa: microchip: Initial SPI regmap support
From: Marek Vasut @ 2019-06-25 12:06 UTC (permalink / raw)
To: Vladimir Oltean
Cc: netdev, Andrew Lunn, Florian Fainelli, Tristram Ha, Woojung Huh
In-Reply-To: <CA+h21hpgXxnnRS7Upc1R82ajLum4k-3O9EQDROonO6jtAD+NZw@mail.gmail.com>
On 6/25/19 1:59 AM, Vladimir Oltean wrote:
> On Tue, 25 Jun 2019 at 01:17, Marek Vasut <marex@denx.de> wrote:
>>
>> On 6/24/19 12:35 AM, Marek Vasut wrote:
>>> Add basic SPI regmap support into the driver.
>>>
>>> Previous patches unconver that ksz_spi_write() is always ever called
>>> with len = 1, 2 or 4. We can thus drop the if (len > SPI_TX_BUF_LEN)
>>> check and we can also drop the allocation of the txbuf which is part
>>> of the driver data and wastes 256 bytes for no reason. Regmap covers
>>> the whole thing now.
>>>
>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>> Cc: Andrew Lunn <andrew@lunn.ch>
>>> Cc: Florian Fainelli <f.fainelli@gmail.com>
>>> Cc: Tristram Ha <Tristram.Ha@microchip.com>
>>> Cc: Woojung Huh <Woojung.Huh@microchip.com>
>>
>> [...]
>>
>>> +#define KS_SPIOP_FLAG_MASK(opcode) \
>>> + cpu_to_be32((opcode) << (SPI_ADDR_SHIFT + SPI_TURNAROUND_SHIFT))
>>
>> So the robot is complaining about this. I believe this is correct, as
>> the mask should be in native endianness on the register and NOT the
>> native endianness of the CPU.
>>
>> I think a cast would help here, e.g.:
>> - cpu_to_be32((opcode) << (SPI_ADDR_SHIFT + SPI_TURNAROUND_SHIFT))
>> - (__force unsigned long)cpu_to_be32((opcode) << (SPI_ADDR_SHIFT +
>> SPI_TURNAROUND_SHIFT))
>>
>> Does this make sense ?
>>
>>> +#define KSZ_REGMAP_COMMON(width) \
>>> + { \
>>> + .val_bits = (width), \
>>> + .reg_stride = (width) / 8, \
>>> + .reg_bits = SPI_ADDR_SHIFT + SPI_ADDR_ALIGN, \
>>> + .pad_bits = SPI_TURNAROUND_SHIFT, \
>>> + .max_register = BIT(SPI_ADDR_SHIFT) - 1, \
>>> + .cache_type = REGCACHE_NONE, \
>>> + .read_flag_mask = KS_SPIOP_FLAG_MASK(KS_SPIOP_RD), \
>>> + .write_flag_mask = KS_SPIOP_FLAG_MASK(KS_SPIOP_WR), \
>>
>> [...]
>>
>> --
>> Best regards,
>> Marek Vasut
>
> Hi Marek,
>
> I saw SPI buffers and endianness and got triggered :)
> Would it make sense to take a look at CONFIG_PACKING for the KSZ9477 driver?
> I don't know how bad the field alignment issue is on that hardware,
> but on SJA1105 it was such a disaster that I couldn't have managed it
> any other way.
How does that help me here ? All I really need is a static constant mask
for the register/flags , 32bit for KSZ9xxx and 16bit for KSZ87xx. I
don't need any dynamic stuff, luckily.
But I'm glad to see TJA1105 driver mainline :)
--
Best regards,
Marek Vasut
^ permalink raw reply
* Re: [PATCH] bpf: Allow bpf_skb_event_output for a few prog types
From: Daniel Borkmann @ 2019-06-25 12:06 UTC (permalink / raw)
To: allanzhang, Alexei Starovoitov, Martin KaFai Lau, Song Liu,
Yonghong Song, David S. Miller, netdev, bpf, linux-kernel
In-Reply-To: <20190625001326.172280-3-allanzhang@google.com>
Hello,
On 06/25/2019 02:13 AM, allanzhang wrote:
> Software event output is only enabled by a few prog types right now (TC,
> LWT out, XDP, sockops). Many other skb based prog types need
> bpf_skb_event_output to produce software event.
>
> Added socket_filter, cg_skb, sk_skb prog types to generate sw event.
>
> Test bpf code is generated from code snippet:
>
> struct TMP {
> uint64_t tmp;
> } tt;
> tt.tmp = 5;
> bpf_perf_event_output(skb, &connection_tracking_event_map, 0,
> &tt, sizeof(tt));
> return 1;
>
> the bpf assembly from llvm is:
> 0: b7 02 00 00 05 00 00 00 r2 = 5
> 1: 7b 2a f8 ff 00 00 00 00 *(u64 *)(r10 - 8) = r2
> 2: bf a4 00 00 00 00 00 00 r4 = r10
> 3: 07 04 00 00 f8 ff ff ff r4 += -8
> 4: 18 02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 r2 = 0ll
> 6: b7 03 00 00 00 00 00 00 r3 = 0
> 7: b7 05 00 00 08 00 00 00 r5 = 8
> 8: 85 00 00 00 19 00 00 00 call 25
> 9: b7 00 00 00 01 00 00 00 r0 = 1
> 10: 95 00 00 00 00 00 00 00 exit
>
> Patch 1 is enabling code.
> Patch 2 is fullly covered selftest code.
>
> Signed-off-by: allanzhang <allanzhang@google.com>
Thanks for the contribution! I'm a bit confused given the many submissions,
some are versioned in the subject (which is the correct way), but this patch
here was sent after v3 (?) but without a version. Which is the right one to
consider for review, I presume v3?
> ---
> tools/testing/selftests/bpf/test_verifier.c | 38 +++++++-
> .../selftests/bpf/verifier/event_output.c | 94 +++++++++++++++++++
> 2 files changed, 130 insertions(+), 2 deletions(-)
> create mode 100644 tools/testing/selftests/bpf/verifier/event_output.c
>
> diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> index c5514daf8865..45ce9dd4323f 100644
> --- a/tools/testing/selftests/bpf/test_verifier.c
> +++ b/tools/testing/selftests/bpf/test_verifier.c
> @@ -1,10 +1,13 @@
> -// SPDX-License-Identifier: GPL-2.0-only
> /*
> * Testsuite for eBPF verifier
> *
> * Copyright (c) 2014 PLUMgrid, http://plumgrid.com
> * Copyright (c) 2017 Facebook
> * Copyright (c) 2018 Covalent IO, Inc. http://covalent.io
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of version 2 of the GNU General Public
> + * License as published by the Free Software Foundation.
This removal of SPDX probably slipped in as well here ..
> */
>
> #include <endian.h>
> @@ -50,7 +53,7 @@
> #define MAX_INSNS BPF_MAXINSNS
> #define MAX_TEST_INSNS 1000000
> #define MAX_FIXUPS 8
> -#define MAX_NR_MAPS 18
> +#define MAX_NR_MAPS 19
> #define MAX_TEST_RUNS 8
> #define POINTER_VALUE 0xcafe4all
> #define TEST_DATA_LEN 64
> @@ -84,6 +87,7 @@ struct bpf_test {
> int fixup_map_array_wo[MAX_FIXUPS];
> int fixup_map_array_small[MAX_FIXUPS];
> int fixup_sk_storage_map[MAX_FIXUPS];
> + int fixup_map_event_output[MAX_FIXUPS];
> const char *errstr;
> const char *errstr_unpriv;
> uint32_t retval, retval_unpriv, insn_processed;
> @@ -604,6 +608,28 @@ static int create_sk_storage_map(void)
> return fd;
> }
>
> +static int create_event_output_map(void)
> +{
> + struct bpf_create_map_attr attr = {
> + .name = "test_map",
> + .map_type = BPF_MAP_TYPE_PERF_EVENT_ARRAY,
> + .key_size = 4,
> + .value_size = 4,
> + .max_entries = 1,
> + };
> + int fd, btf_fd;
> +
> + btf_fd = load_btf();
> + if (btf_fd < 0)
> + return -1;
> + attr.btf_fd = btf_fd;
> + fd = bpf_create_map_xattr(&attr);
> + close(attr.btf_fd);
> + if (fd < 0)
> + printf("Failed to create event_output\n");
> + return fd;
> +}
> +
> static char bpf_vlog[UINT_MAX >> 8];
>
> static void do_test_fixup(struct bpf_test *test, enum bpf_prog_type prog_type,
> @@ -627,6 +653,7 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_prog_type prog_type,
> int *fixup_map_array_wo = test->fixup_map_array_wo;
> int *fixup_map_array_small = test->fixup_map_array_small;
> int *fixup_sk_storage_map = test->fixup_sk_storage_map;
> + int *fixup_map_event_output = test->fixup_map_event_output;
>
> if (test->fill_helper) {
> test->fill_insns = calloc(MAX_TEST_INSNS, sizeof(struct bpf_insn));
> @@ -788,6 +815,13 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_prog_type prog_type,
> fixup_sk_storage_map++;
> } while (*fixup_sk_storage_map);
> }
> + if (*fixup_map_event_output) {
> + map_fds[18] = create_event_output_map();
> + do {
> + prog[*fixup_map_event_output].imm = map_fds[18];
> + fixup_map_event_output++;
> + } while (*fixup_map_event_output);
> + }
> }
>
> static int set_admin(bool admin)
> diff --git a/tools/testing/selftests/bpf/verifier/event_output.c b/tools/testing/selftests/bpf/verifier/event_output.c
> new file mode 100644
> index 000000000000..b25eabcfaa56
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/verifier/event_output.c
> @@ -0,0 +1,94 @@
> +/* instructions used to output a skb based software event, produced
> + * from code snippet:
> +struct TMP {
> + uint64_t tmp;
> +} tt;
> +tt.tmp = 5;
> +bpf_perf_event_output(skb, &connection_tracking_event_map, 0,
> + &tt, sizeof(tt));
> +return 1;
> +
> +the bpf assembly from llvm is:
> + 0: b7 02 00 00 05 00 00 00 r2 = 5
> + 1: 7b 2a f8 ff 00 00 00 00 *(u64 *)(r10 - 8) = r2
> + 2: bf a4 00 00 00 00 00 00 r4 = r10
> + 3: 07 04 00 00 f8 ff ff ff r4 += -8
> + 4: 18 02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 r2 = 0ll
> + 6: b7 03 00 00 00 00 00 00 r3 = 0
> + 7: b7 05 00 00 08 00 00 00 r5 = 8
> + 8: 85 00 00 00 19 00 00 00 call 25
> + 9: b7 00 00 00 01 00 00 00 r0 = 1
> + 10: 95 00 00 00 00 00 00 00 exit
> +
> + The reason I put the code here instead of fill_helpers is that map fixup is
> + against the insns, instead of filled prog.
> +*/
> +
> +#define __PERF_EVENT_INSNS__ \
> + BPF_MOV64_IMM(BPF_REG_2, 5), \
> + BPF_STX_MEM(BPF_DW, BPF_REG_10, BPF_REG_2, -8), \
> + BPF_MOV64_REG(BPF_REG_4, BPF_REG_10), \
> + BPF_ALU64_IMM(BPF_ADD, BPF_REG_4, -8), \
> + BPF_LD_MAP_FD(BPF_REG_2, 0), \
> + BPF_MOV64_IMM(BPF_REG_3, 0), \
> + BPF_MOV64_IMM(BPF_REG_5, 8), \
> + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, \
> + BPF_FUNC_perf_event_output), \
> + BPF_MOV64_IMM(BPF_REG_0, 1), \
> + BPF_EXIT_INSN(),
> +{
> + "perfevent for sockops",
> + .insns = { __PERF_EVENT_INSNS__ },
> + .prog_type = BPF_PROG_TYPE_SOCK_OPS,
> + .fixup_map_event_output = { 4 },
> + .result = ACCEPT,
> + .retval = 1,
> +},
> +{
> + "perfevent for tc",
> + .insns = { __PERF_EVENT_INSNS__ },
> + .prog_type = BPF_PROG_TYPE_SCHED_CLS,
> + .fixup_map_event_output = { 4 },
> + .result = ACCEPT,
> + .retval = 1,
> +},
> +{
> + "perfevent for lwt out",
> + .insns = { __PERF_EVENT_INSNS__ },
> + .prog_type = BPF_PROG_TYPE_LWT_OUT,
> + .fixup_map_event_output = { 4 },
> + .result = ACCEPT,
> + .retval = 1,
> +},
> +{
> + "perfevent for xdp",
> + .insns = { __PERF_EVENT_INSNS__ },
> + .prog_type = BPF_PROG_TYPE_XDP,
> + .fixup_map_event_output = { 4 },
> + .result = ACCEPT,
> + .retval = 1,
> +},
> +{
> + "perfevent for socket filter",
> + .insns = { __PERF_EVENT_INSNS__ },
> + .prog_type = BPF_PROG_TYPE_SOCKET_FILTER,
> + .fixup_map_event_output = { 4 },
> + .result = ACCEPT,
> + .retval = 1,
> +},
> +{
> + "perfevent for sk_skb",
> + .insns = { __PERF_EVENT_INSNS__ },
> + .prog_type = BPF_PROG_TYPE_SK_SKB,
> + .fixup_map_event_output = { 4 },
> + .result = ACCEPT,
> + .retval = 1,
> +},
> +{
> + "perfevent for cgroup skb",
> + .insns = { __PERF_EVENT_INSNS__ },
> + .prog_type = BPF_PROG_TYPE_CGROUP_SKB,
> + .fixup_map_event_output = { 4 },
> + .result = ACCEPT,
> + .retval = 1,
> +},
>
^ permalink raw reply
* Re: [PATCH RFC net-next 1/5] net: dsa: mt7530: Convert to PHYLINK API
From: Russell King - ARM Linux admin @ 2019-06-25 12:10 UTC (permalink / raw)
To: René van Dorst
Cc: sean.wang, f.fainelli, davem, matthias.bgg, andrew,
vivien.didelot, frank-w, netdev, linux-mediatek, linux-mips
In-Reply-To: <20190625113158.Horde.pCaJOVUsgyhYLd5Diz5EZKI@www.vdorst.com>
On Tue, Jun 25, 2019 at 11:31:58AM +0000, René van Dorst wrote:
> > > + if (state->link || mode == MLO_AN_FIXED)
> > > + mcr |= PMCR_FORCE_LNK;
> >
> > This should be removed - state->link is not for use in mac_config.
> > Even in fixed mode, the link can be brought up/down by means of a
> > gpio, and this should be dealt with via the mac_link_* functions.
>
> Maybe I understand it wrong, but is it the intention that in
> phylink_mac_config with modes MLO_AN_FIXED and MLO_AN_PHY the MAC is always
> forces into a certain speed/mode/interface. So it never auto-negotiate because
> phylink select the best configuration for you?
You are not the only one who has recently tried to make use of
state->link in mac_config(), so I'm now preparing a set of patches
to split the current mac_config() method into two separate methods:
void (*mac_config_fixed)(struct net_device *ndev,
phy_interface_t iface, int speed, int duplex,
int pause);
void (*mac_config_inband)(struct net_device *ndev,
phy_interface_t iface, bool an_enabled,
unsigned long *advertising, int pause);
so that it's not possible to use members that should not be accessed
in various modes.
> Also the PMCR_FORCE_LNK is only set in phylink_link_up() or can I do it here
> and do nothing phylink_link_up()?
When the link comes up/down, mac_link_up() and mac_link_down() will be
called appropriately. In PHY mode, this is equivalent to phylink doing
this:
if (link_changed) {
if (phydev->link)
mac_link_up();
else
mac_link_down();
}
So the actions you would've done depending on phydev->link should be in
the mac_link_*() methods.
> Other question:
> Where does the MAC enable/disable TX and RX fits best? port_{enable,disable}?
> Or only mac_config() and port_disable?
mac_link_*().
> > > + if (state->pause || phylink_test(state->advertising, Pause))
> > > + mcr |= PMCR_TX_FC_EN | PMCR_RX_FC_EN;
> > > + if (state->pause & MLO_PAUSE_TX)
> > > + mcr |= PMCR_TX_FC_EN;
> > > + if (state->pause & MLO_PAUSE_RX)
> > > + mcr |= PMCR_RX_FC_EN;
> >
> > This is clearly wrong - if any bit in state->pause is set, then we
> > end up with both PMCR_TX_FC_EN | PMCR_RX_FC_EN set. If we have Pause
> > Pause set in the advertising mask, then both are set. This doesn't
> > seem right - are these bits setting the advertisement, or are they
> > telling the MAC to use flow control?
>
> Last one, tell the MAC to use flow control.
So the first if() statement is incorrect, and should be removed
entirely. You only want to enable the MAC to use flow control as a
result of the negotiation results.
> On the current driver both bits are set in a forced-link situation.
>
> If we always forces the MAC mode I think I always set these bits and don't
> anything with the Pause modes? Is that the right way to do it?
So what happens if your link partner (e.g. switch) does not support
flow control? What if your link partner floods such frames to all
ports? You end up transmitting flow control frames, which could be
sent to all stations on the network... seems not a good idea.
Implementing stuff properly and not taking short-cuts is always a
good idea for inter-operability.
> > > +
> > > +static void mt7530_phylink_validate(struct dsa_switch *ds, int port,
> > > + unsigned long *supported,
> > > + struct phylink_link_state *state)
> > > +{
> > > + __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
> > > +
> > > + switch (port) {
> > > + case 0: /* Internal phy */
> > > + case 1:
> > > + case 2:
> > > + case 3:
> > > + case 4:
> > > + if (state->interface != PHY_INTERFACE_MODE_NA &&
> > > + state->interface != PHY_INTERFACE_MODE_GMII)
> > > + goto unsupported;
> > > + break;
> > > + /* case 5: Port 5 not supported! */
> > > + case 6: /* 1st cpu port */
> > > + if (state->interface != PHY_INTERFACE_MODE_RGMII &&
> > > + state->interface != PHY_INTERFACE_MODE_TRGMII)
> >
> > PHY_INTERFACE_MODE_NA ?
>
> You mean PHY_INTERFACE_MODE_NA is missing?
Yes. Please see the updated documentation in the patch I sent this
morning "net: phylink: further documentation clarifications".
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply
* [PATCH net] vrf: reset rt_iif for recirculated mcast out pkts
From: Stephen Suryaputra @ 2019-06-25 10:33 UTC (permalink / raw)
To: netdev; +Cc: dsahern, Stephen Suryaputra
Multicast egress packets has skb_rtable(skb)->rt_iif set to the oif.
Depending on the socket, these packets might be recirculated back as
input and raw sockets that are opened for them are bound to the VRF. But
since skb_rtable(skb) is set and its rt_iif is non-zero, inet_iif()
function returns rt_iif instead of skb_iif (the VRF netdev). Hence, the
socket lookup fails.
Signed-off-by: Stephen Suryaputra <ssuryaextr@gmail.com>
---
include/net/route.h | 1 +
net/ipv4/ip_output.c | 25 ++++++++++++++++++++++++-
net/ipv4/route.c | 33 +++++++++++++++++++++++++++++++++
3 files changed, 58 insertions(+), 1 deletion(-)
diff --git a/include/net/route.h b/include/net/route.h
index 065b47754f05..55ff71ffb796 100644
--- a/include/net/route.h
+++ b/include/net/route.h
@@ -221,6 +221,7 @@ void ip_rt_get_source(u8 *src, struct sk_buff *skb, struct rtable *rt);
struct rtable *rt_dst_alloc(struct net_device *dev,
unsigned int flags, u16 type,
bool nopolicy, bool noxfrm, bool will_cache);
+struct rtable *rt_dst_clone(struct net_device *dev, struct rtable *rt);
struct in_ifaddr;
void fib_add_ifaddr(struct in_ifaddr *);
diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index 16f9159234a2..a5e240bad3ce 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -329,6 +329,19 @@ static int ip_mc_finish_output(struct net *net, struct sock *sk,
return dev_loopback_xmit(net, sk, skb);
}
+static void ip_mc_reset_rt_iif(struct net *net, struct rtable *rt,
+ struct sk_buff *newskb)
+{
+ struct rtable *new_rt;
+
+ new_rt = rt_dst_clone(net->loopback_dev, rt);
+ if (new_rt) {
+ new_rt->rt_iif = 0;
+ skb_dst_drop(newskb);
+ skb_dst_set(newskb, &new_rt->dst);
+ }
+}
+
int ip_mc_output(struct net *net, struct sock *sk, struct sk_buff *skb)
{
struct rtable *rt = skb_rtable(skb);
@@ -363,10 +376,20 @@ int ip_mc_output(struct net *net, struct sock *sk, struct sk_buff *skb)
#endif
) {
struct sk_buff *newskb = skb_clone(skb, GFP_ATOMIC);
- if (newskb)
+ if (newskb) {
+ /* Reset rt_iif so that inet_iif() will return
+ * skb->dev->ifIndex which is the VRF device for
+ * socket lookup. Setting this to VRF ifindex
+ * causes ipi_ifindex in in_pktinfo to be
+ * overwritten, see ipv4_pktinfo_prepare().
+ */
+ if (netif_is_l3_slave(dev))
+ ip_mc_reset_rt_iif(net, rt, newskb);
+
NF_HOOK(NFPROTO_IPV4, NF_INET_POST_ROUTING,
net, sk, newskb, NULL, newskb->dev,
ip_mc_finish_output);
+ }
}
/* Multicasts with ttl 0 must not go beyond the host */
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 6cb7cff22db9..8ea0735a6754 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -1647,6 +1647,39 @@ struct rtable *rt_dst_alloc(struct net_device *dev,
}
EXPORT_SYMBOL(rt_dst_alloc);
+struct rtable *rt_dst_clone(struct net_device *dev, struct rtable *rt)
+{
+ struct rtable *new_rt;
+
+ new_rt = dst_alloc(&ipv4_dst_ops, dev, 1, DST_OBSOLETE_FORCE_CHK,
+ rt->dst.flags);
+
+ if (new_rt) {
+ new_rt->rt_genid = rt_genid_ipv4(dev_net(dev));
+ new_rt->rt_flags = rt->rt_flags;
+ new_rt->rt_type = rt->rt_type;
+ new_rt->rt_is_input = rt->rt_is_input;
+ new_rt->rt_iif = rt->rt_iif;
+ new_rt->rt_pmtu = rt->rt_pmtu;
+ new_rt->rt_mtu_locked = rt->rt_mtu_locked;
+ new_rt->rt_gw_family = rt->rt_gw_family;
+ if (rt->rt_gw_family == AF_INET)
+ new_rt->rt_gw4 = rt->rt_gw4;
+ else if (rt->rt_gw_family == AF_INET6)
+ new_rt->rt_gw6 = rt->rt_gw6;
+ INIT_LIST_HEAD(&new_rt->rt_uncached);
+
+ new_rt->dst.flags |= DST_HOST;
+ new_rt->dst.input = rt->dst.input;
+ new_rt->dst.output = rt->dst.output;
+ new_rt->dst.error = rt->dst.error;
+ new_rt->dst.lastuse = jiffies;
+ new_rt->dst.lwtstate = lwtstate_get(rt->dst.lwtstate);
+ }
+ return new_rt;
+}
+EXPORT_SYMBOL(rt_dst_clone);
+
/* called in rcu_read_lock() section */
int ip_mc_validate_source(struct sk_buff *skb, __be32 daddr, __be32 saddr,
u8 tos, struct net_device *dev,
--
2.17.1
^ permalink raw reply related
* Re: [PATCH net-next v2 08/12] xdp: tracking page_pool resources and safe removal
From: Ivan Khoronzhuk @ 2019-06-25 12:28 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: netdev, Ilias Apalodimas, Toke Høiland-Jørgensen,
Tariq Toukan, toshiaki.makita1, grygorii.strashko, mcroce
In-Reply-To: <20190625115107.GB6485@khorivan>
On Tue, Jun 25, 2019 at 02:51:08PM +0300, Ivan Khoronzhuk wrote:
>On Tue, Jun 25, 2019 at 01:27:50PM +0200, Jesper Dangaard Brouer wrote:
>>On Tue, 25 Jun 2019 13:50:14 +0300
>>Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
>>
>>>Hi Jesper,
>>>
>>>Could you please clarify one question.
>>>
>>>On Tue, Jun 18, 2019 at 03:05:47PM +0200, Jesper Dangaard Brouer wrote:
>>>>This patch is needed before we can allow drivers to use page_pool for
>>>>DMA-mappings. Today with page_pool and XDP return API, it is possible to
>>>>remove the page_pool object (from rhashtable), while there are still
>>>>in-flight packet-pages. This is safely handled via RCU and failed lookups in
>>>>__xdp_return() fallback to call put_page(), when page_pool object is gone.
>>>>In-case page is still DMA mapped, this will result in page note getting
>>>>correctly DMA unmapped.
>>>>
>>>>To solve this, the page_pool is extended with tracking in-flight pages. And
>>>>XDP disconnect system queries page_pool and waits, via workqueue, for all
>>>>in-flight pages to be returned.
>>>>
>>>>To avoid killing performance when tracking in-flight pages, the implement
>>>>use two (unsigned) counters, that in placed on different cache-lines, and
>>>>can be used to deduct in-flight packets. This is done by mapping the
>>>>unsigned "sequence" counters onto signed Two's complement arithmetic
>>>>operations. This is e.g. used by kernel's time_after macros, described in
>>>>kernel commit 1ba3aab3033b and 5a581b367b5, and also explained in RFC1982.
>>>>
>>>>The trick is these two incrementing counters only need to be read and
>>>>compared, when checking if it's safe to free the page_pool structure. Which
>>>>will only happen when driver have disconnected RX/alloc side. Thus, on a
>>>>non-fast-path.
>>>>
>>>>It is chosen that page_pool tracking is also enabled for the non-DMA
>>>>use-case, as this can be used for statistics later.
>>>>
>>>>After this patch, using page_pool requires more strict resource "release",
>>>>e.g. via page_pool_release_page() that was introduced in this patchset, and
>>>>previous patches implement/fix this more strict requirement.
>>>>
>>>>Drivers no-longer call page_pool_destroy(). Drivers already call
>>>>xdp_rxq_info_unreg() which call xdp_rxq_info_unreg_mem_model(), which will
>>>>attempt to disconnect the mem id, and if attempt fails schedule the
>>>>disconnect for later via delayed workqueue.
>>>>
>>>>Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
>>>>Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>>>>---
>>>> drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 3 -
>>>> include/net/page_pool.h | 41 ++++++++++---
>>>> net/core/page_pool.c | 62 +++++++++++++++-----
>>>> net/core/xdp.c | 65 +++++++++++++++++++--
>>>> 4 files changed, 136 insertions(+), 35 deletions(-)
>>>>
>>>>diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>>>>index 2f647be292b6..6c9d4d7defbc 100644
>>>>--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>>>>+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>>>>@@ -643,9 +643,6 @@ static void mlx5e_free_rq(struct mlx5e_rq *rq)
>>>> }
>>>>
>>>> xdp_rxq_info_unreg(&rq->xdp_rxq);
>>>>- if (rq->page_pool)
>>>>- page_pool_destroy(rq->page_pool);
>>>>-
>>>> mlx5_wq_destroy(&rq->wq_ctrl);
>>>> }
>>>>
>>>>diff --git a/include/net/page_pool.h b/include/net/page_pool.h
>>>>index 754d980700df..f09b3f1994e6 100644
>>>>--- a/include/net/page_pool.h
>>>>+++ b/include/net/page_pool.h
>>>>@@ -16,14 +16,16 @@
>>>> * page_pool_alloc_pages() call. Drivers should likely use
>>>> * page_pool_dev_alloc_pages() replacing dev_alloc_pages().
>>>> *
>>>>- * If page_pool handles DMA mapping (use page->private), then API user
>>>>- * is responsible for invoking page_pool_put_page() once. In-case of
>>>>- * elevated refcnt, the DMA state is released, assuming other users of
>>>>- * the page will eventually call put_page().
>>>>+ * API keeps track of in-flight pages, in-order to let API user know
>>>>+ * when it is safe to dealloactor page_pool object. Thus, API users
>>>>+ * must make sure to call page_pool_release_page() when a page is
>>>>+ * "leaving" the page_pool. Or call page_pool_put_page() where
>>>>+ * appropiate. For maintaining correct accounting.
>>>> *
>>>>- * If no DMA mapping is done, then it can act as shim-layer that
>>>>- * fall-through to alloc_page. As no state is kept on the page, the
>>>>- * regular put_page() call is sufficient.
>>>>+ * API user must only call page_pool_put_page() once on a page, as it
>>>>+ * will either recycle the page, or in case of elevated refcnt, it
>>>>+ * will release the DMA mapping and in-flight state accounting. We
>>>>+ * hope to lift this requirement in the future.
>>>> */
>>>> #ifndef _NET_PAGE_POOL_H
>>>> #define _NET_PAGE_POOL_H
>>>>@@ -66,9 +68,10 @@ struct page_pool_params {
>>>> };
>>>>
>>>> struct page_pool {
>>>>- struct rcu_head rcu;
>>>> struct page_pool_params p;
>>>>
>>>>+ u32 pages_state_hold_cnt;
>>>>+
>>>> /*
>>>> * Data structure for allocation side
>>>> *
>>>>@@ -96,6 +99,8 @@ struct page_pool {
>>>> * TODO: Implement bulk return pages into this structure.
>>>> */
>>>> struct ptr_ring ring;
>>>>+
>>>>+ atomic_t pages_state_release_cnt;
>>>> };
>>>>
>>>> struct page *page_pool_alloc_pages(struct page_pool *pool, gfp_t gfp);
>>>>@@ -109,8 +114,6 @@ static inline struct page *page_pool_dev_alloc_pages(struct page_pool *pool)
>>>>
>>>> struct page_pool *page_pool_create(const struct page_pool_params *params);
>>>>
>>>>-void page_pool_destroy(struct page_pool *pool);
>>>>-
>>>> void __page_pool_free(struct page_pool *pool);
>>>> static inline void page_pool_free(struct page_pool *pool)
>>>> {
>>>>@@ -143,6 +146,24 @@ static inline void page_pool_recycle_direct(struct page_pool *pool,
>>>> __page_pool_put_page(pool, page, true);
>>>> }
>>>>
>>>>+/* API user MUST have disconnected alloc-side (not allowed to call
>>>>+ * page_pool_alloc_pages()) before calling this. The free-side can
>>>>+ * still run concurrently, to handle in-flight packet-pages.
>>>>+ *
>>>>+ * A request to shutdown can fail (with false) if there are still
>>>>+ * in-flight packet-pages.
>>>>+ */
>>>>+bool __page_pool_request_shutdown(struct page_pool *pool);
>>>>+static inline bool page_pool_request_shutdown(struct page_pool *pool)
>>>>+{
>>>>+ /* When page_pool isn't compiled-in, net/core/xdp.c doesn't
>>>>+ * allow registering MEM_TYPE_PAGE_POOL, but shield linker.
>>>>+ */
>>>>+#ifdef CONFIG_PAGE_POOL
>>>>+ return __page_pool_request_shutdown(pool);
>>>>+#endif
>>>>+}
>>>
>>>The free side can ran in softirq, that means fast cache recycle is accessed.
>>>And it increments not atomic pool->alloc.count.
>>>
>>>For instance While redirect, for remote interface, while .ndo_xdp_xmit the
>>>xdp_return_frame_rx_napi(xdpf) is called everywhere in error path ....
>^
>|
>
>>>
>>>In the same time, simultaneously, the work queue can try one more
>>>time to clear cash, calling __page_pool_request_shutdown()....
>>>
>>>Question, what prevents pool->alloc.count to be corrupted by race,
>>>causing to wrong array num and as result wrong page to be unmapped/put ....or
>>>even page leak. alloc.count usage is not protected,
>>>__page_pool_request_shutdown() is called not from same rx NAPI, even not from
>>>NAPI.
>>>
>>>Here, while alloc cache empty procedure in __page_pool_request_shutdown():
>>
>>You forgot to copy this comment, which explains:
>>
>> /* Empty alloc cache, assume caller made sure this is
>> * no-longer in use, and page_pool_alloc_pages() cannot be
>> * call concurrently.
>> */
>No I didn't. I'm talking about recycle, not allocation.
>To be more specific about this:
>__page_pool_recycle_direct() while remote ndev .ndo_xdp_xmit
>
>About this:
>"
>/* API user MUST have disconnected alloc-side (not allowed to call
>* page_pool_alloc_pages()) before calling this. The free-side can
>* still run concurrently, to handle in-flight packet-pages.
>"
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.
--
Regards,
Ivan Khoronzhuk
^ permalink raw reply
* Re: [PATCH v3 bpf-next 0/2] veth: Bulk XDP_TX
From: Daniel Borkmann @ 2019-06-25 12:29 UTC (permalink / raw)
To: Toshiaki Makita, Alexei Starovoitov, David S. Miller,
Jakub Kicinski, Jesper Dangaard Brouer, John Fastabend
Cc: netdev, xdp-newbies, bpf, Toke Høiland-Jørgensen,
Jason Wang
In-Reply-To: <20190613093959.2796-1-toshiaki.makita1@gmail.com>
On 06/13/2019 11:39 AM, Toshiaki Makita wrote:
> This introduces bulk XDP_TX in veth.
> Improves XDP_TX performance by approximately 9%. The detailed
> explanation and performance numbers are shown in patch 2.
>
> v2:
> - Use stack for bulk queue instead of a global variable.
>
> v3:
> - Add act field to xdp_bulk_tx tracepoint to be in line with other XDP
> tracepoints.
>
> Signed-off-by: Toshiaki Makita <toshiaki.makita1@gmail.com>
>
> Toshiaki Makita (2):
> xdp: Add tracepoint for bulk XDP_TX
> veth: Support bulk XDP_TX
>
> drivers/net/veth.c | 60 ++++++++++++++++++++++++++++++++++++----------
> include/trace/events/xdp.h | 29 ++++++++++++++++++++++
> kernel/bpf/core.c | 1 +
> 3 files changed, 78 insertions(+), 12 deletions(-)
>
Applied, thanks!
^ permalink raw reply
* DMA API usage in au1000_eth.c
From: Christoph Hellwig @ 2019-06-25 12:34 UTC (permalink / raw)
To: BjoernRiemerriemer, Matt Porter, Herbert Valerio Riedel
Cc: linux-mips, netdev, linux-kernel
Hi all,
you are the persons that have their names listed in the driver,
hope you all remember what you did 15 years ago :)
The au1000_eth driver uses the DMA API somewhat oddly. For one
it uses the DMA_ATTR_NON_CONSISTENT flag to allocate memory that
is not DMA coherent, accompanied by a comment say:
/* Allocate the data buffers
* Snooping works fine with eth on all au1xxx
*/
which suggests that it actually is DMA coherent. As far as I can
tell many amd mips platforms are DMA coherent, in which case
DMA_ATTR_NON_CONSISTENT is no-op and everything is fine here. But
it seems some are not, in which case DMA_ATTR_NON_CONSISTENT will
give us not coherent memory, in which case something would be
broken? Removing DMA_ATTR_NON_CONSISTENT would be no-op on
coherent platforms, but return an address in the cache segement
on those that are non-coherent. Do you know if the hardware
event cares?
The next issue is that it calls virt_to_phys on the return value
from dma_alloc_attrs. Why would the hardware care about the physical
address and not the DMA address (in general those are the same in
mips)?
Last but not least it stores the kernel address return from
dma_alloc_attrs in a u32 instead of a pointer, which is a little
odd and not type safe, but not otherwise dramatic.
I can prepare a patch to fix these up, but I'd like to confirm my
theory about coherent of the platform and device first.
^ permalink raw reply
* Re: [PATCH] net: phylink: further documentation clarifications
From: Andrew Lunn @ 2019-06-25 12:38 UTC (permalink / raw)
To: Russell King; +Cc: Florian Fainelli, Heiner Kallweit, David S. Miller, netdev
In-Reply-To: <E1hfi09-0007Zs-Vb@rmk-PC.armlinux.org.uk>
On Tue, Jun 25, 2019 at 10:44:33AM +0100, Russell King wrote:
> Clarify the validate() behaviour in a few cases which weren't mentioned
> in the documentation, but which are necessary for users to get the
> correct behaviour.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH v3 2/2] bpf: Add selftests for bpf_perf_event_output
From: Daniel Borkmann @ 2019-06-25 12:40 UTC (permalink / raw)
To: allanzhang, Alexei Starovoitov, Martin KaFai Lau, Song Liu,
Yonghong Song, David S. Miller, netdev, bpf, linux-kernel
In-Reply-To: <20190624235720.167067-3-allanzhang@google.com>
On 06/25/2019 01:57 AM, allanzhang wrote:
> Software event output is only enabled by a few prog types.
> This test is to ensure that all supported types are enbled for
Nit, typo: enbled
> bpf_perf_event_output sucessfully.
Nit, typo: sucessfully
> Signed-off-by: allanzhang <allanzhang@google.com>
For SOB, could you add proper formatted name before the email?
> ---
> tools/testing/selftests/bpf/test_verifier.c | 33 ++++++-
> .../selftests/bpf/verifier/event_output.c | 94 +++++++++++++++++++
> 2 files changed, 126 insertions(+), 1 deletion(-)
> create mode 100644 tools/testing/selftests/bpf/verifier/event_output.c
>
> diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> index c5514daf8865..901a188e1eea 100644
> --- a/tools/testing/selftests/bpf/test_verifier.c
> +++ b/tools/testing/selftests/bpf/test_verifier.c
> @@ -50,7 +50,7 @@
> #define MAX_INSNS BPF_MAXINSNS
> #define MAX_TEST_INSNS 1000000
> #define MAX_FIXUPS 8
> -#define MAX_NR_MAPS 18
> +#define MAX_NR_MAPS 19
> #define MAX_TEST_RUNS 8
> #define POINTER_VALUE 0xcafe4all
> #define TEST_DATA_LEN 64
> @@ -84,6 +84,7 @@ struct bpf_test {
> int fixup_map_array_wo[MAX_FIXUPS];
> int fixup_map_array_small[MAX_FIXUPS];
> int fixup_sk_storage_map[MAX_FIXUPS];
> + int fixup_map_event_output[MAX_FIXUPS];
> const char *errstr;
> const char *errstr_unpriv;
> uint32_t retval, retval_unpriv, insn_processed;
> @@ -604,6 +605,28 @@ static int create_sk_storage_map(void)
> return fd;
> }
>
> +static int create_event_output_map(void)
> +{
> + struct bpf_create_map_attr attr = {
> + .name = "test_map",
> + .map_type = BPF_MAP_TYPE_PERF_EVENT_ARRAY,
> + .key_size = 4,
> + .value_size = 4,
> + .max_entries = 1,
> + };
> + int fd, btf_fd;
> +
> + btf_fd = load_btf();
> + if (btf_fd < 0)
> + return -1;
> + attr.btf_fd = btf_fd;
> + fd = bpf_create_map_xattr(&attr);
This does not look correct, BTF for spinlock does not belong to perf event array.
> + close(attr.btf_fd);
> + if (fd < 0)
> + printf("Failed to create event_output\n");
> + return fd;
> +}
> +
> static char bpf_vlog[UINT_MAX >> 8];
>
> static void do_test_fixup(struct bpf_test *test, enum bpf_prog_type prog_type,
> @@ -627,6 +650,7 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_prog_type prog_type,
> int *fixup_map_array_wo = test->fixup_map_array_wo;
> int *fixup_map_array_small = test->fixup_map_array_small;
> int *fixup_sk_storage_map = test->fixup_sk_storage_map;
> + int *fixup_map_event_output = test->fixup_map_event_output;
>
> if (test->fill_helper) {
> test->fill_insns = calloc(MAX_TEST_INSNS, sizeof(struct bpf_insn));
> @@ -788,6 +812,13 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_prog_type prog_type,
> fixup_sk_storage_map++;
> } while (*fixup_sk_storage_map);
> }
> + if (*fixup_map_event_output) {
> + map_fds[18] = create_event_output_map();
> + do {
> + prog[*fixup_map_event_output].imm = map_fds[18];
> + fixup_map_event_output++;
> + } while (*fixup_map_event_output);
> + }
> }
>
> static int set_admin(bool admin)
> diff --git a/tools/testing/selftests/bpf/verifier/event_output.c b/tools/testing/selftests/bpf/verifier/event_output.c
> new file mode 100644
> index 000000000000..b25eabcfaa56
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/verifier/event_output.c
> @@ -0,0 +1,94 @@
> +/* instructions used to output a skb based software event, produced
> + * from code snippet:
> +struct TMP {
> + uint64_t tmp;
> +} tt;
> +tt.tmp = 5;
> +bpf_perf_event_output(skb, &connection_tracking_event_map, 0,
> + &tt, sizeof(tt));
> +return 1;
[...]
^ permalink raw reply
* Re: [PATCH V3 07/10] net: dsa: microchip: Initial SPI regmap support
From: Vladimir Oltean @ 2019-06-25 12:40 UTC (permalink / raw)
To: Marek Vasut
Cc: netdev, Andrew Lunn, Florian Fainelli, Tristram Ha, Woojung Huh
In-Reply-To: <f1dfe749-4bfa-17f7-ede7-f9bc38afa0d4@denx.de>
On Tue, 25 Jun 2019 at 15:06, Marek Vasut <marex@denx.de> wrote:
>
> On 6/25/19 1:59 AM, Vladimir Oltean wrote:
> > On Tue, 25 Jun 2019 at 01:17, Marek Vasut <marex@denx.de> wrote:
> >>
> >> On 6/24/19 12:35 AM, Marek Vasut wrote:
> >>> Add basic SPI regmap support into the driver.
> >>>
> >>> Previous patches unconver that ksz_spi_write() is always ever called
> >>> with len = 1, 2 or 4. We can thus drop the if (len > SPI_TX_BUF_LEN)
> >>> check and we can also drop the allocation of the txbuf which is part
> >>> of the driver data and wastes 256 bytes for no reason. Regmap covers
> >>> the whole thing now.
> >>>
> >>> Signed-off-by: Marek Vasut <marex@denx.de>
> >>> Cc: Andrew Lunn <andrew@lunn.ch>
> >>> Cc: Florian Fainelli <f.fainelli@gmail.com>
> >>> Cc: Tristram Ha <Tristram.Ha@microchip.com>
> >>> Cc: Woojung Huh <Woojung.Huh@microchip.com>
> >>
> >> [...]
> >>
> >>> +#define KS_SPIOP_FLAG_MASK(opcode) \
> >>> + cpu_to_be32((opcode) << (SPI_ADDR_SHIFT + SPI_TURNAROUND_SHIFT))
> >>
> >> So the robot is complaining about this. I believe this is correct, as
> >> the mask should be in native endianness on the register and NOT the
> >> native endianness of the CPU.
> >>
> >> I think a cast would help here, e.g.:
> >> - cpu_to_be32((opcode) << (SPI_ADDR_SHIFT + SPI_TURNAROUND_SHIFT))
> >> - (__force unsigned long)cpu_to_be32((opcode) << (SPI_ADDR_SHIFT +
> >> SPI_TURNAROUND_SHIFT))
> >>
> >> Does this make sense ?
> >>
> >>> +#define KSZ_REGMAP_COMMON(width) \
> >>> + { \
> >>> + .val_bits = (width), \
> >>> + .reg_stride = (width) / 8, \
> >>> + .reg_bits = SPI_ADDR_SHIFT + SPI_ADDR_ALIGN, \
> >>> + .pad_bits = SPI_TURNAROUND_SHIFT, \
> >>> + .max_register = BIT(SPI_ADDR_SHIFT) - 1, \
> >>> + .cache_type = REGCACHE_NONE, \
> >>> + .read_flag_mask = KS_SPIOP_FLAG_MASK(KS_SPIOP_RD), \
> >>> + .write_flag_mask = KS_SPIOP_FLAG_MASK(KS_SPIOP_WR), \
> >>
> >> [...]
> >>
> >> --
> >> Best regards,
> >> Marek Vasut
> >
> > Hi Marek,
> >
> > I saw SPI buffers and endianness and got triggered :)
> > Would it make sense to take a look at CONFIG_PACKING for the KSZ9477 driver?
> > I don't know how bad the field alignment issue is on that hardware,
> > but on SJA1105 it was such a disaster that I couldn't have managed it
> > any other way.
>
> How does that help me here ? All I really need is a static constant mask
> for the register/flags , 32bit for KSZ9xxx and 16bit for KSZ87xx. I
> don't need any dynamic stuff, luckily.
>
Ok. I was thinking *instead of* regmap.
On the SJA1105 I couldn't use a spi regmap because:
* the enum regmap_endian wasn't enough to describe the hardware's bit
layout (it is big endian, but high-order and low-order 32 bit words
are swapped)
* it doesn't really expose a well-defined register map, but rather,
you're supposed to dynamically construct a TLV-type buffer and write
it starting with a fixed address.
I just thought you were facing some of these problems as well with
regmap. If not, that's perfectly fine!
> But I'm glad to see TJA1105 driver mainline :)
>
> --
> Best regards,
> Marek Vasut
Regards,
-Vladimir
^ permalink raw reply
* Re: memory leak in sctp_get_port_local
From: Xin Long @ 2019-06-25 12:49 UTC (permalink / raw)
To: syzbot
Cc: davem, LKML, linux-sctp, Marcelo Ricardo Leitner, network dev,
Neil Horman, syzkaller-bugs, Vlad Yasevich
In-Reply-To: <00000000000069c3140589f6d3b7@google.com>
On Wed, May 29, 2019 at 2:28 AM syzbot
<syzbot+079bf326b38072f849d9@syzkaller.appspotmail.com> wrote:
>
> Hello,
>
> syzbot found the following crash on:
>
> HEAD commit: cd6c84d8 Linux 5.2-rc2
> git tree: upstream
> console output: https://syzkaller.appspot.com/x/log.txt?x=101a184aa00000
> kernel config: https://syzkaller.appspot.com/x/.config?x=64479170dcaf0e11
> dashboard link: https://syzkaller.appspot.com/bug?extid=079bf326b38072f849d9
> compiler: gcc (GCC) 9.0.0 20181231 (experimental)
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=13b5dbbca00000
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=1038444aa00000
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+079bf326b38072f849d9@syzkaller.appspotmail.com
>
> : Permanently added '10.128.0.127' (ECDSA) to the list of known hosts.
> executing program
> executing program
> BUG: memory leak
> unreferenced object 0xffff8881288ca380 (size 64):
> comm "softirq", pid 0, jiffies 4294944468 (age 13.410s)
> hex dump (first 32 bytes):
> 21 4e 00 00 00 00 00 00 00 00 00 00 00 00 00 00 !N..............
> 28 ae 85 23 81 88 ff ff 00 00 00 00 00 00 00 00 (..#............
> backtrace:
> [<0000000054ece54d>] kmemleak_alloc_recursive
> include/linux/kmemleak.h:55 [inline]
> [<0000000054ece54d>] slab_post_alloc_hook mm/slab.h:439 [inline]
> [<0000000054ece54d>] slab_alloc mm/slab.c:3326 [inline]
> [<0000000054ece54d>] kmem_cache_alloc+0x134/0x270 mm/slab.c:3488
> [<00000000d992ea84>] sctp_bucket_create net/sctp/socket.c:8395 [inline]
> [<00000000d992ea84>] sctp_get_port_local+0x189/0x5b0
> net/sctp/socket.c:8142
> [<0000000099206d90>] sctp_do_bind+0xcc/0x1e0 net/sctp/socket.c:402
> [<00000000b8795757>] sctp_bind+0x44/0x70 net/sctp/socket.c:302
> [<00000000672a44aa>] inet6_bind+0x40/0xb7 net/ipv6/af_inet6.c:445
> [<0000000001400e1c>] __sys_bind+0x11c/0x140 net/socket.c:1659
> [<00000000e69e8036>] __do_sys_bind net/socket.c:1670 [inline]
> [<00000000e69e8036>] __se_sys_bind net/socket.c:1668 [inline]
> [<00000000e69e8036>] __x64_sys_bind+0x1e/0x30 net/socket.c:1668
> [<000000001644bb1f>] do_syscall_64+0x76/0x1a0
> arch/x86/entry/common.c:301
> [<00000000199a1ea2>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
will post a fix for this:
@@ -4816,6 +4816,7 @@ static int sctp_setsockopt(struct sock *sk, int
level, int optname,
static int sctp_connect(struct sock *sk, struct sockaddr *addr,
int addr_len, int flags)
{
+ struct sctp_bind_addr *bp = &sctp_sk(sk)->ep->base.bind_addr;
struct inet_sock *inet = inet_sk(sk);
struct sctp_af *af;
int err = 0;
@@ -4826,12 +4827,13 @@ static int sctp_connect(struct sock *sk,
struct sockaddr *addr,
addr, addr_len);
/* We may need to bind the socket. */
- if (!inet->inet_num) {
+ if (!bp->port) {
if (sk->sk_prot->get_port(sk, 0)) {
release_sock(sk);
return -EAGAIN;
}
inet->inet_sport = htons(inet->inet_num);
+ bp->port = inet->inet_sport;
}
>
>
> ---
> This bug is generated by a bot. It may contain errors.
> See https://goo.gl/tpsmEJ for more information about syzbot.
> syzbot engineers can be reached at syzkaller@googlegroups.com.
>
> syzbot will keep track of this bug report. See:
> https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
> syzbot can test patches for this bug, for details see:
> https://goo.gl/tpsmEJ#testing-patches
^ permalink raw reply
* [PATCH] can: mcp251x: add error check when wq alloc failed
From: Weitao Hou @ 2019-06-25 12:50 UTC (permalink / raw)
To: wg, mkl, davem, gregkh, allison, houweitaoo, tglx, sean
Cc: linux-can, netdev, linux-kernel
add error check when workqueue alloc failed, and remove
redundant code to make it clear
Signed-off-by: Weitao Hou <houweitaoo@gmail.com>
---
drivers/net/can/spi/mcp251x.c | 49 ++++++++++++++++-------------------
1 file changed, 22 insertions(+), 27 deletions(-)
diff --git a/drivers/net/can/spi/mcp251x.c b/drivers/net/can/spi/mcp251x.c
index 44e99e3d7134..2aec934fab0c 100644
--- a/drivers/net/can/spi/mcp251x.c
+++ b/drivers/net/can/spi/mcp251x.c
@@ -664,17 +664,6 @@ static int mcp251x_power_enable(struct regulator *reg, int enable)
return regulator_disable(reg);
}
-static void mcp251x_open_clean(struct net_device *net)
-{
- struct mcp251x_priv *priv = netdev_priv(net);
- struct spi_device *spi = priv->spi;
-
- free_irq(spi->irq, priv);
- mcp251x_hw_sleep(spi);
- mcp251x_power_enable(priv->transceiver, 0);
- close_candev(net);
-}
-
static int mcp251x_stop(struct net_device *net)
{
struct mcp251x_priv *priv = netdev_priv(net);
@@ -940,37 +929,43 @@ static int mcp251x_open(struct net_device *net)
flags | IRQF_ONESHOT, DEVICE_NAME, priv);
if (ret) {
dev_err(&spi->dev, "failed to acquire irq %d\n", spi->irq);
- mcp251x_power_enable(priv->transceiver, 0);
- close_candev(net);
- goto open_unlock;
+ goto out_close;
}
priv->wq = alloc_workqueue("mcp251x_wq", WQ_FREEZABLE | WQ_MEM_RECLAIM,
0);
+ if (!priv->wq) {
+ ret = -ENOMEM;
+ goto out_clean;
+ }
INIT_WORK(&priv->tx_work, mcp251x_tx_work_handler);
INIT_WORK(&priv->restart_work, mcp251x_restart_work_handler);
ret = mcp251x_hw_reset(spi);
- if (ret) {
- mcp251x_open_clean(net);
- goto open_unlock;
- }
+ if (ret)
+ goto out_free_wq;
ret = mcp251x_setup(net, spi);
- if (ret) {
- mcp251x_open_clean(net);
- goto open_unlock;
- }
+ if (ret)
+ goto out_free_wq;
ret = mcp251x_set_normal_mode(spi);
- if (ret) {
- mcp251x_open_clean(net);
- goto open_unlock;
- }
+ if (ret)
+ goto out_free_wq;
can_led_event(net, CAN_LED_EVENT_OPEN);
netif_wake_queue(net);
+ mutex_unlock(&priv->mcp_lock);
-open_unlock:
+ return 0;
+
+out_free_wq:
+ destroy_workqueue(priv->wq);
+out_clean:
+ free_irq(spi->irq, priv);
+ mcp251x_hw_sleep(spi);
+out_close:
+ mcp251x_power_enable(priv->transceiver, 0);
+ close_candev(net);
mutex_unlock(&priv->mcp_lock);
return ret;
}
--
2.18.0
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox