* Re: [PATCH bpf-next 0/4] sys_bpf() access control via /dev/bpf
From: Alexei Starovoitov @ 2019-06-25 21:00 UTC (permalink / raw)
To: Stanislav Fomichev, Song Liu
Cc: netdev@vger.kernel.org, bpf@vger.kernel.org, ast@kernel.org,
daniel@iogearbox.net, Kernel Team
In-Reply-To: <20190625205155.GD10487@mini-arch>
On 6/25/19 1:51 PM, Stanislav Fomichev wrote:
> On 06/25, Song Liu wrote:
>> Currently, most access to sys_bpf() is limited to root. However, there are
>> use cases that would benefit from non-privileged use of sys_bpf(), e.g.
>> systemd.
>>
>> This set introduces a new model to control the access to sys_bpf(). A
>> special device, /dev/bpf, is introduced to manage access to sys_bpf().
>> Users with access to open /dev/bpf will be able to access most of
>> sys_bpf() features. The use can get access to sys_bpf() by opening /dev/bpf
>> and use ioctl to get/put permission.
>>
>> The permission to access sys_bpf() is marked by bit TASK_BPF_FLAG_PERMITTED
>> in task_struct. During fork(), child will not inherit this bit.
> 2c: if we are going to have an fd, I'd vote for a proper fd based access
> checks instead of a per-task flag, so we can do:
> ioctl(fd, BPF_MAP_CREATE, uattr, sizeof(uattr))
>
> (and pass this fd around)
>
> I do understand that it breaks current assumptions that libbpf has,
> but maybe we can extend _xattr variants to accept optinal fd (and try
> to fallback to sysctl if it's absent/not working)?
both of these ideas were discussed at lsfmm where you were present.
I'm not sure why you're bring it up again?
^ permalink raw reply
* Re: [PATCH 1/1] mlx5: Fix build when CONFIG_MLX5_EN_RXNFC is disabled
From: Saeed Mahameed @ 2019-06-25 21:01 UTC (permalink / raw)
To: jes.sorensen@gmail.com, davem@davemloft.net
Cc: kernel-team@fb.com, jsorensen@fb.com, netdev@vger.kernel.org
In-Reply-To: <20190625.133404.1626801368802216614.davem@davemloft.net>
On Tue, 2019-06-25 at 13:34 -0700, David Miller wrote:
> From: Jes Sorensen <jes.sorensen@gmail.com>
> Date: Tue, 25 Jun 2019 11:27:08 -0400
>
> > From: Jes Sorensen <jsorensen@fb.com>
> >
> > The previous patch broke the build with a static declaration for
> > a public function.
> >
> > Fixes: 8f0916c6dc5c (net/mlx5e: Fix ethtool rxfh commands when
> > CONFIG_MLX5_EN_RXNFC is disabled)
> > Signed-off-by: Jes Sorensen <jsorensen@fb.com>
>
> Saeed, I'm assuming I will get this via your next pull request once
> things
> are sorted.
>
Well, i think there is no issue in upstream, but yes basically you can
delegate this to me.
BTW is there a way to clear up "Awaiting Upstream" clutter [1] for mlx5
patches that are already pulled ?
[1]
https://patchwork.ozlabs.org/project/netdev/list/?series=&submitter=&state=8&q=mlx5&archive=&delegate=
> Thanks.
^ permalink raw reply
* Re: What to do when a bridge port gets its pvid deleted?
From: Vladimir Oltean @ 2019-06-25 21:05 UTC (permalink / raw)
To: netdev, Andrew Lunn, Florian Fainelli, Vivien Didelot, stephen
In-Reply-To: <CA+h21hrRMrLH-RjBGhEJSTZd6_QPRSd3RkVRQF-wNKkrgKcRSA@mail.gmail.com>
On Tue, 25 Jun 2019 at 23:49, Vladimir Oltean <olteanv@gmail.com> wrote:
>
> Hi,
>
> A number of DSA drivers (BCM53XX, Microchip KSZ94XX, Mediatek MT7530
> at the very least), as well as Mellanox Spectrum (I didn't look at all
> the pure switchdev drivers) try to restore the pvid to a default value
> on .port_vlan_del.
> Sure, the port stops receiving traffic when its pvid is a VLAN ID that
> is not installed in its hw filter, but as far as the bridge core is
> concerned, this is to be expected:
>
> # bridge vlan add dev swp2 vid 100 pvid untagged
> # bridge vlan
> port vlan ids
> swp5 1 PVID Egress Untagged
>
> swp2 1 Egress Untagged
> 100 PVID Egress Untagged
>
> swp3 1 PVID Egress Untagged
>
> swp4 1 PVID Egress Untagged
>
> br0 1 PVID Egress Untagged
> # ping 10.0.0.1
> PING 10.0.0.1 (10.0.0.1) 56(84) bytes of data.
> 64 bytes from 10.0.0.1: icmp_seq=1 ttl=64 time=0.682 ms
> 64 bytes from 10.0.0.1: icmp_seq=2 ttl=64 time=0.299 ms
> 64 bytes from 10.0.0.1: icmp_seq=3 ttl=64 time=0.251 ms
> 64 bytes from 10.0.0.1: icmp_seq=4 ttl=64 time=0.324 ms
> 64 bytes from 10.0.0.1: icmp_seq=5 ttl=64 time=0.257 ms
> ^C
> --- 10.0.0.1 ping statistics ---
> 5 packets transmitted, 5 received, 0% packet loss, time 4188ms
> rtt min/avg/max/mdev = 0.251/0.362/0.682/0.163 ms
> # bridge vlan del dev swp2 vid 100
> # bridge vlan
> port vlan ids
> swp5 1 PVID Egress Untagged
>
> swp2 1 Egress Untagged
>
> swp3 1 PVID Egress Untagged
>
> swp4 1 PVID Egress Untagged
>
> br0 1 PVID Egress Untagged
>
> # ping 10.0.0.1
> PING 10.0.0.1 (10.0.0.1) 56(84) bytes of data.
> ^C
> --- 10.0.0.1 ping statistics ---
> 8 packets transmitted, 0 received, 100% packet loss, time 7267ms
>
> What is the consensus here? Is there a reason why the bridge driver
> doesn't take care of this? Do switchdev drivers have to restore the
> pvid to always be operational, even if their state becomes
> inconsistent with the upper dev? Is it just 'nice to have'? What if
> VID 1 isn't in the hw filter either (perfectly legal)?
>
> Thanks!
> -Vladimir
Or rather, let me put it differently.
I am not only asking to figure out whether I should put this extra
logic or not in a switchdev driver - I can understand the "you don't
want it, don't put it" concept.
But let's say that for various reasons, MSTP is not supported in the
Linux kernel and I'm trying to create an ad-hoc MSTP setup where I
break the loop manually in the pvid. Is there any good reason why a
switchdev driver would oppose fierce resistance to me trying to do
that? Again, this is perfectly valid - the switch will no longer
receive untagged traffic but it will continue to forward frames in
other VLANs.
-Vladimir
^ permalink raw reply
* Re: [pull request][for-next V2 0/7] Generic DIM lib for netdev and RDMA
From: Saeed Mahameed @ 2019-06-25 21:07 UTC (permalink / raw)
To: Jason Gunthorpe, davem@davemloft.net, dledford@redhat.com
Cc: linux-rdma@vger.kernel.org, Or Gerlitz, sagi@grimberg.me,
Leon Romanovsky, Tal Gilboa, netdev@vger.kernel.org
In-Reply-To: <20190625205701.17849-1-saeedm@mellanox.com>
On Tue, 2019-06-25 at 20:57 +0000, Saeed Mahameed wrote:
> Hi Dave, Doug & Jason
>
> This series improves DIM - Dynamically-tuned Interrupt
> Moderation- to be generic for netdev and RDMA use-cases.
>
> From Tal and Yamin:
>
> First 7 patches provide the necessary refactoring to current net_dim
> library which affect some net drivers who are using the API.
>
> The last 3 patches provide the RDMA implementation for DIM.
> These patches are included in this pull request and they are posted
correction: The last 3 patches are *NOT* included in this pull request.
The idea here is to pull the re-factoring API patches that effect and
touch net drivers [0-7] to both trees and [8-10] will be sent later to
rdma tree only.
Thanks,
Saeed.
^ permalink raw reply
* Re: [PATCH net-next] Revert "net: ena: ethtool: add extra properties retrieval via get_priv_flags"
From: Machulsky, Zorik @ 2019-06-25 21:07 UTC (permalink / raw)
To: David Miller, jakub.kicinski@netronome.com
Cc: netdev@vger.kernel.org, oss-drivers@netronome.com, Bshara, Nafea,
dwmw2@infradead.org, Jubran, Samih, Bshara, Saeed,
Belgazal, Netanel
In-Reply-To: <20190625.134315.561835619337464509.davem@davemloft.net>
As we agreed during Netconf discussion we'll look into other alternative - the devlink tool that should potentially answer our needs.
On 6/25/19, 1:43 PM, "David Miller" <davem@davemloft.net> wrote:
From: Jakub Kicinski <jakub.kicinski@netronome.com>
Date: Tue, 25 Jun 2019 09:59:56 -0700
> This reverts commit 315c28d2b714 ("net: ena: ethtool: add extra properties retrieval via get_priv_flags").
>
> As discussed at netconf and on the mailing list we can't allow
> for the the abuse of private flags for exposing arbitrary device
> labels.
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Amazon folks I think this is entirely reasonable, please ACK.
Thanks.
^ permalink raw reply
* Re: [PATCH RFC net-next 1/5] net: dsa: mt7530: Convert to PHYLINK API
From: René van Dorst @ 2019-06-25 21:07 UTC (permalink / raw)
To: Andrew Lunn
Cc: Daniel Santos, Russell King - ARM Linux admin, sean.wang,
f.fainelli, davem, matthias.bgg, vivien.didelot, frank-w, netdev,
linux-mediatek, linux-mips
In-Reply-To: <20190625204148.GB27733@lunn.ch>
Quoting Andrew Lunn <andrew@lunn.ch>:
Hi Andrew,
> On Tue, Jun 25, 2019 at 02:27:55PM -0500, Daniel Santos wrote:
>> On 6/25/19 2:02 PM, Andrew Lunn wrote:
>> >> But will there still be a mechanism to ignore link partner's advertising
>> >> and force these parameters?
>> > >From man 1 ethtool:
>> >
>> > -a --show-pause
>> > Queries the specified Ethernet device for pause
>> parameter information.
>> >
>> > -A --pause
>> > Changes the pause parameters of the specified
>> Ethernet device.
>> >
>> > autoneg on|off
>> > Specifies whether pause autonegotiation should
>> be enabled.
>> >
>> > rx on|off
>> > Specifies whether RX pause should be enabled.
>> >
>> > tx on|off
>> > Specifies whether TX pause should be enabled.
>> >
>> > You need to check the driver to see if it actually implements this
>> > ethtool call, but that is how it should be configured.
>> >
>> > Andrew
>> >
>> Thank you Andrew,
>>
>> So in this context, my question is the difference between "enabling" and
>> "forcing". Here's that register for the mt7620 (which has an mt7530 on
>> its die): https://imgur.com/a/pTk0668 I believe this is also what René
>> is seeking clarity on?
>
> Lets start with normal operation. If the MAC supports pause or asym
> pause, it calls phy_support_sym_pause() or phy_support_asym_pause().
> phylib will then configure the PHY to advertise pause as appropriate.
> Once auto-neg has completed, the results of the negotiation are set in
> phydev. So phdev->pause and phydev->asym_pause. The MAC callback is
> then used to tell the MAC about the autoneg results. The MAC should be
> programmed using the values in phdev->pause and phydev->asym_pause.
>
> For ethtool, the MAC driver needs to implement .get_pauseparam and
> .set_pauseparam. The set_pauseparam needs to validate the settings,
> using phy_validate_pause(). If valid, phy_set_asym_pause() is used to
> tell the PHY about the new configuration. This will trigger a new
> auto-neg if auto-neg is enabled, and the results will be passed back
> in the usual way. If auto-neg is disabled, or pause auto-neg is
> disabled, the MAC should configure pause directly based on the
> settings passed.
>
> Looking at the data sheet page, you want FORCE_MODE_Pn set. You never
> want the MAC directly talking to the PHY. Bad things will happen.
> Then use FORCE_RX_FC_Pn and FORCE_TX_Pn to reflect phydev->pause and
> phydev->asym_pause.
>
> The same idea applies when using phylink.
Thanks for this information.
I updated mt7530_phylink_mac_config(), I think I done it right this time
with the pause bits.
Now mt7530_phylink_mac_config() looks like this:
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_cur, mcr_new = 0;
switch (port) {
case 0: /* Internal phy */
case 1:
case 2:
case 3:
case 4:
if (state->interface != PHY_INTERFACE_MODE_GMII)
return;
break;
case 5: /* 2nd cpu port with phy of port 0 or 4 / external phy */
if (!phy_interface_mode_is_rgmii(state->interface) &&
state->interface != PHY_INTERFACE_MODE_MII)
return;
if (priv->p5_intf_sel != P5_INTF_SEL_GMAC5) {
priv->p5_intf_sel = P5_INTF_SEL_GMAC5;
mt7530_setup_port5(ds, state->interface);
}
/* We are connected to external phy */
if (dsa_is_user_port(ds, 5))
mcr_new |= PMCR_EXT_PHY;
break;
case 6: /* 1st cpu port */
if (state->interface != PHY_INTERFACE_MODE_RGMII &&
state->interface != PHY_INTERFACE_MODE_TRGMII)
return;
if (priv->p6_interface == state->interface)
break;
/* 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);
}
priv->p6_interface = state->interface;
break;
default:
dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port);
return;
}
mcr_cur = mt7530_read(priv, MT7530_PMCR_P(port));
mcr_new |= mcr_cur;
mcr_new &= ~(PMCR_FORCE_SPEED_1000 | PMCR_FORCE_SPEED_100 |
PMCR_FORCE_FDX | PMCR_TX_FC_EN | PMCR_RX_FC_EN);
mcr_new |= PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | PMCR_BACKOFF_EN |
PMCR_BACKPR_EN | PMCR_FORCE_MODE | PMCR_FORCE_LNK;
switch (state->speed) {
case SPEED_1000:
mcr_new |= PMCR_FORCE_SPEED_1000;
break;
case SPEED_100:
mcr_new |= PMCR_FORCE_SPEED_100;
break;
}
if (state->duplex == DUPLEX_FULL) {
mcr_new |= PMCR_FORCE_FDX;
if (state->pause & MLO_PAUSE_TX)
mcr_new |= PMCR_TX_FC_EN;
if (state->pause & MLO_PAUSE_RX)
mcr_new |= PMCR_RX_FC_EN;
}
if (mcr_new != mcr_cur)
mt7530_write(priv, MT7530_PMCR_P(port), mcr_new);
}
Greats,
René
^ permalink raw reply
* Re: [PATCH 1/1] mlx5: Fix build when CONFIG_MLX5_EN_RXNFC is disabled
From: David Miller @ 2019-06-25 21:08 UTC (permalink / raw)
To: saeedm; +Cc: jes.sorensen, kernel-team, jsorensen, netdev
In-Reply-To: <91260adb2227e477647afda66fdff9d9a9f52c60.camel@mellanox.com>
From: Saeed Mahameed <saeedm@mellanox.com>
Date: Tue, 25 Jun 2019 21:01:58 +0000
> BTW is there a way to clear up "Awaiting Upstream" clutter [1] for mlx5
> patches that are already pulled ?
>
> [1]
> https://patchwork.ozlabs.org/project/netdev/list/?series=&submitter=&state=8&q=mlx5&archive=&delegate=
I don't understand what the problem is. Everything there is in the
appropriate state.
When something hits netdev that doesn't go directly to my tree, that's
the approprate state forever.
^ permalink raw reply
* Re: [PATCH net-next] Revert "net: ena: ethtool: add extra properties retrieval via get_priv_flags"
From: David Miller @ 2019-06-25 21:09 UTC (permalink / raw)
To: jakub.kicinski
Cc: netdev, oss-drivers, nafea, dwmw2, sameehj, zorik, saeedb,
netanel
In-Reply-To: <20190625165956.19278-1-jakub.kicinski@netronome.com>
From: Jakub Kicinski <jakub.kicinski@netronome.com>
Date: Tue, 25 Jun 2019 09:59:56 -0700
> This reverts commit 315c28d2b714 ("net: ena: ethtool: add extra properties retrieval via get_priv_flags").
>
> As discussed at netconf and on the mailing list we can't allow
> for the the abuse of private flags for exposing arbitrary device
> labels.
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Applied, thanks Jakub.
^ permalink raw reply
* Re: [PATCH 0/1] Fix broken build of mlx5
From: Saeed Mahameed @ 2019-06-25 21:11 UTC (permalink / raw)
To: jes.sorensen@gmail.com
Cc: kernel-team@fb.com, jsorensen@fb.com, netdev@vger.kernel.org
In-Reply-To: <74cb713f-ad8c-7f86-c611-9dd4265f1c9b@gmail.com>
On Tue, 2019-06-25 at 14:01 -0400, Jes Sorensen wrote:
> On 6/25/19 1:54 PM, Saeed Mahameed wrote:
> > On Tue, 2019-06-25 at 11:27 -0400, Jes Sorensen wrote:
> > > From: Jes Sorensen <jsorensen@fb.com>
> > >
> > > This fixes an obvious build error that could have been caught by
> > > simply building the code before pushing out the patch.
> > >
> >
> > Hi Jes,
> >
> > Just tested again, as I have tested before submitting the blamed
> > patch,
> > and as we test on every single new patch in our build automation.
> >
> > both combinations CONFIG_MLX5_EN_RXNFC=y/n work on latest net-next,
> > what am i missing ?
>
> Linus' tree:
>
> [jes@xpeas linux.git]$ grep mlx5e_get_rxnfc
> drivers/net/ethernet/mellanox/mlx5/core/*.c
> drivers/net/ethernet/mellanox/mlx5/core/en/*.h
> drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c:static int
> mlx5e_get_rxnfc(struct net_device *dev, struct ethtool_rxnfc *info,
> u32
> *rule_locs)
> drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c: .get_rxnfc
> = mlx5e_get_rxnfc,
> drivers/net/ethernet/mellanox/mlx5/core/en_fs_ethtool.c:int
> mlx5e_get_rxnfc(struct net_device *dev,
> drivers/net/ethernet/mellanox/mlx5/core/en/fs.h:int
> mlx5e_get_rxnfc(struct net_device *dev,
>
> static vs non static functions, with a prototype that is non static.
But only one prototype can be selected and it is according
CONFIG_MLX5_EN_RXNFC
when CONFIG_MLX5_EN_RXNFC=n then the static prototype will be selected
and the other one will be compiled out.
when CONFIG_MLX5_EN_RXNFC=y the non static prototype will be selected
and the static one will be compiled out.
So no issue here.
>
> Jes
^ 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 21:13 UTC (permalink / raw)
To: Andrew Lunn
Cc: Daniel Santos, René van Dorst, sean.wang, f.fainelli, davem,
matthias.bgg, vivien.didelot, frank-w, netdev, linux-mediatek,
linux-mips
In-Reply-To: <20190625190246.GA27733@lunn.ch>
On Tue, Jun 25, 2019 at 09:02:46PM +0200, Andrew Lunn wrote:
> > But will there still be a mechanism to ignore link partner's advertising
> > and force these parameters?
>
> From man 1 ethtool:
>
> -a --show-pause
> Queries the specified Ethernet device for pause parameter information.
>
> -A --pause
> Changes the pause parameters of the specified Ethernet device.
>
> autoneg on|off
> Specifies whether pause autonegotiation should be enabled.
>
> rx on|off
> Specifies whether RX pause should be enabled.
>
> tx on|off
> Specifies whether TX pause should be enabled.
>
> You need to check the driver to see if it actually implements this
> ethtool call, but that is how it should be configured.
Note that phylink provides this call, and provided mac_config() is
correctly implemented, will result in the pause mode parameters in
the MAC being correctly set.
--
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
* Re: [for-next V2 10/10] RDMA/core: Provide RDMA DIM support for ULPs
From: Sagi Grimberg @ 2019-06-25 21:14 UTC (permalink / raw)
To: Saeed Mahameed, David S. Miller, Doug Ledford, Jason Gunthorpe
Cc: Leon Romanovsky, Or Gerlitz, Tal Gilboa, netdev@vger.kernel.org,
linux-rdma@vger.kernel.org, Yamin Friedman, Max Gurtovoy
In-Reply-To: <20190625205701.17849-11-saeedm@mellanox.com>
> +static int ib_poll_dim_handler(struct irq_poll *iop, int budget)
> +{
> + struct ib_cq *cq = container_of(iop, struct ib_cq, iop);
> + struct dim *dim = cq->dim;
> + int completed;
> +
> + completed = __ib_process_cq(cq, budget, cq->wc, IB_POLL_BATCH);
> + if (completed < budget) {
> + irq_poll_complete(&cq->iop);
> + if (ib_req_notify_cq(cq, IB_POLL_FLAGS) > 0)
> + irq_poll_sched(&cq->iop);
> + }
> +
> + rdma_dim(dim, completed);
Why duplicate the entire thing for a one-liner?
> +
> + return completed;
> +}
> +
> static void ib_cq_completion_softirq(struct ib_cq *cq, void *private)
> {
> irq_poll_sched(&cq->iop);
> @@ -105,14 +157,18 @@ static void ib_cq_completion_softirq(struct ib_cq *cq, void *private)
>
> static void ib_cq_poll_work(struct work_struct *work)
> {
> - struct ib_cq *cq = container_of(work, struct ib_cq, work);
> + struct ib_cq *cq = container_of(work, struct ib_cq,
> + work);
Why was that changed?
> int completed;
>
> completed = __ib_process_cq(cq, IB_POLL_BUDGET_WORKQUEUE, cq->wc,
> IB_POLL_BATCH);
> +
newline?
> if (completed >= IB_POLL_BUDGET_WORKQUEUE ||
> ib_req_notify_cq(cq, IB_POLL_FLAGS) > 0)
> queue_work(cq->comp_wq, &cq->work);
> + else if (cq->dim)
> + rdma_dim(cq->dim, completed);
> }
>
> static void ib_cq_completion_workqueue(struct ib_cq *cq, void *private)
> @@ -166,6 +222,8 @@ struct ib_cq *__ib_alloc_cq_user(struct ib_device *dev, void *private,
> rdma_restrack_set_task(&cq->res, caller);
> rdma_restrack_kadd(&cq->res);
>
> + rdma_dim_init(cq);
> +
> switch (cq->poll_ctx) {
> case IB_POLL_DIRECT:
> cq->comp_handler = ib_cq_completion_direct;
> @@ -173,7 +231,13 @@ struct ib_cq *__ib_alloc_cq_user(struct ib_device *dev, void *private,
> case IB_POLL_SOFTIRQ:
> cq->comp_handler = ib_cq_completion_softirq;
>
> - irq_poll_init(&cq->iop, IB_POLL_BUDGET_IRQ, ib_poll_handler);
> + if (cq->dim) {
> + irq_poll_init(&cq->iop, IB_POLL_BUDGET_IRQ,
> + ib_poll_dim_handler);
> + } else
> + irq_poll_init(&cq->iop, IB_POLL_BUDGET_IRQ,
> + ib_poll_handler);
> +
> ib_req_notify_cq(cq, IB_CQ_NEXT_COMP);
> break;
> case IB_POLL_WORKQUEUE:
> @@ -226,6 +290,9 @@ void ib_free_cq_user(struct ib_cq *cq, struct ib_udata *udata)
> WARN_ON_ONCE(1);
> }
>
> + if (cq->dim)
> + cancel_work_sync(&cq->dim->work);
> + kfree(cq->dim);
> kfree(cq->wc);
> rdma_restrack_del(&cq->res);
> ret = cq->device->ops.destroy_cq(cq, udata);
> diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
> index abac70ad5c7c..b1b45dbe24a5 100644
> --- a/drivers/infiniband/hw/mlx5/main.c
> +++ b/drivers/infiniband/hw/mlx5/main.c
> @@ -6305,6 +6305,8 @@ static int mlx5_ib_stage_caps_init(struct mlx5_ib_dev *dev)
> MLX5_CAP_GEN(dev->mdev, disable_local_lb_mc)))
> mutex_init(&dev->lb.mutex);
>
> + dev->ib_dev.use_cq_dim = true;
> +
Please don't. This is a bad choice to opt it in by default.
^ permalink raw reply
* Re: [for-next V2 09/10] RDMA/nldev: Added configuration of RDMA dynamic interrupt moderation to netlink
From: Sagi Grimberg @ 2019-06-25 21:15 UTC (permalink / raw)
To: Saeed Mahameed, David S. Miller, Doug Ledford, Jason Gunthorpe
Cc: Leon Romanovsky, Or Gerlitz, Tal Gilboa, netdev@vger.kernel.org,
linux-rdma@vger.kernel.org, Yamin Friedman
In-Reply-To: <20190625205701.17849-10-saeedm@mellanox.com>
On 6/25/19 1:57 PM, Saeed Mahameed wrote:
> From: Yamin Friedman <yaminf@mellanox.com>
>
> Added parameter in ib_device for enabling dynamic interrupt moderation so
> that it can be configured in userspace using rdma tool.
>
> In order to set dim for an ib device the command is:
> rdma dev set [DEV] dim [on|off]
> Please set on/off.
Is "dim" what you want to expose to the user? maybe
"adaptive-moderation" is more friendly?
^ permalink raw reply
* Re: [PATCH bpf-next 0/4] sys_bpf() access control via /dev/bpf
From: Stanislav Fomichev @ 2019-06-25 21:19 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Song Liu, netdev@vger.kernel.org, bpf@vger.kernel.org,
ast@kernel.org, daniel@iogearbox.net, Kernel Team
In-Reply-To: <59e56064-354c-d6b9-101a-c698976e6723@fb.com>
On 06/25, Alexei Starovoitov wrote:
> On 6/25/19 1:51 PM, Stanislav Fomichev wrote:
> > On 06/25, Song Liu wrote:
> >> Currently, most access to sys_bpf() is limited to root. However, there are
> >> use cases that would benefit from non-privileged use of sys_bpf(), e.g.
> >> systemd.
> >>
> >> This set introduces a new model to control the access to sys_bpf(). A
> >> special device, /dev/bpf, is introduced to manage access to sys_bpf().
> >> Users with access to open /dev/bpf will be able to access most of
> >> sys_bpf() features. The use can get access to sys_bpf() by opening /dev/bpf
> >> and use ioctl to get/put permission.
> >>
> >> The permission to access sys_bpf() is marked by bit TASK_BPF_FLAG_PERMITTED
> >> in task_struct. During fork(), child will not inherit this bit.
> > 2c: if we are going to have an fd, I'd vote for a proper fd based access
> > checks instead of a per-task flag, so we can do:
> > ioctl(fd, BPF_MAP_CREATE, uattr, sizeof(uattr))
> >
> > (and pass this fd around)
> >
> > I do understand that it breaks current assumptions that libbpf has,
> > but maybe we can extend _xattr variants to accept optinal fd (and try
> > to fallback to sysctl if it's absent/not working)?
>
> both of these ideas were discussed at lsfmm where you were present.
> I'm not sure why you're bring it up again?
Did we actually settle on anything? In that case feel free to ignore me,
maybe I missed that. I remember there were pros/cons for both implementations.
^ 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 21:21 UTC (permalink / raw)
To: Andrew Lunn
Cc: Daniel Santos, René van Dorst, sean.wang, f.fainelli, davem,
matthias.bgg, vivien.didelot, frank-w, netdev, linux-mediatek,
linux-mips
In-Reply-To: <20190625204148.GB27733@lunn.ch>
On Tue, Jun 25, 2019 at 10:41:48PM +0200, Andrew Lunn wrote:
> On Tue, Jun 25, 2019 at 02:27:55PM -0500, Daniel Santos wrote:
> > On 6/25/19 2:02 PM, Andrew Lunn wrote:
> > >> But will there still be a mechanism to ignore link partner's advertising
> > >> and force these parameters?
> > > >From man 1 ethtool:
> > >
> > > -a --show-pause
> > > Queries the specified Ethernet device for pause parameter information.
> > >
> > > -A --pause
> > > Changes the pause parameters of the specified Ethernet device.
> > >
> > > autoneg on|off
> > > Specifies whether pause autonegotiation should be enabled.
> > >
> > > rx on|off
> > > Specifies whether RX pause should be enabled.
> > >
> > > tx on|off
> > > Specifies whether TX pause should be enabled.
> > >
> > > You need to check the driver to see if it actually implements this
> > > ethtool call, but that is how it should be configured.
> > >
> > > Andrew
> > >
> > Thank you Andrew,
> >
> > So in this context, my question is the difference between "enabling" and
> > "forcing". Here's that register for the mt7620 (which has an mt7530 on
> > its die): https://imgur.com/a/pTk0668 I believe this is also what René
> > is seeking clarity on?
>
> Lets start with normal operation. If the MAC supports pause or asym
> pause, it calls phy_support_sym_pause() or phy_support_asym_pause().
> phylib will then configure the PHY to advertise pause as appropriate.
> Once auto-neg has completed, the results of the negotiation are set in
> phydev. So phdev->pause and phydev->asym_pause. The MAC callback is
> then used to tell the MAC about the autoneg results. The MAC should be
> programmed using the values in phdev->pause and phydev->asym_pause.
>
> For ethtool, the MAC driver needs to implement .get_pauseparam and
> .set_pauseparam. The set_pauseparam needs to validate the settings,
> using phy_validate_pause(). If valid, phy_set_asym_pause() is used to
> tell the PHY about the new configuration. This will trigger a new
> auto-neg if auto-neg is enabled, and the results will be passed back
> in the usual way. If auto-neg is disabled, or pause auto-neg is
> disabled, the MAC should configure pause directly based on the
> settings passed.
>
> Looking at the data sheet page, you want FORCE_MODE_Pn set. You never
> want the MAC directly talking to the PHY. Bad things will happen.
> Then use FORCE_RX_FC_Pn and FORCE_TX_Pn to reflect phydev->pause and
> phydev->asym_pause.
>
> The same idea applies when using phylink.
Except when using phylink, use pause & MLO_PAUSE_RX to determine whether
FORCE_RX_FC_Pn should be set, and use pause & MLO_PAUSE_TX to determine
whether FORCE_TX_Pn should be set.
phylink will take care of the results of negotiation with the link
partner and tell you what should be set if pause autoneg is enabled.
If the user has decided to force it via ethtool, and the MAC driver
passes the calls on to phylink, phylink will instead set MLO_PAUSE_RX
and MLO_PAUSE_TX according to the users settings.
So, with phylink, it's quite literally "if MLO_PAUSE_RX is set in
mac_config, enable receiption of pause frames. if MLO_PAUSE_TX is set,
enable transmission of pause frames."
The above applies for phylink's PHY, FIXED, and SGMII in-band modes.
For 802.3z in-band modes, pause is negotiated between the two link
parters (which could be the PCS built into the MACs at either end)
and in some cases its possible to set the MAC to automatically adjust
to the results of the built-in PCS negotiation.
--
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
* Re: [PATCH 1/4] b43legacy: remove b43legacy_dma_set_mask
From: Larry Finger @ 2019-06-25 21:45 UTC (permalink / raw)
To: Christoph Hellwig, Kalle Valo
Cc: b43-dev, linux-wireless, netdev, linux-kernel
In-Reply-To: <20190625102932.32257-2-hch@lst.de>
On 6/25/19 5:29 AM, Christoph Hellwig wrote:
> These days drivers are not required to fallback to smaller DMA masks,
> but can just set the largest mask they support, removing the need for
> this trial and error logic.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> drivers/net/wireless/broadcom/b43legacy/dma.c | 39 +------------------
> 1 file changed, 1 insertion(+), 38 deletions(-)
The patches work for PPC32 for both b43legacy and b43.
Tested-by: Larry Finger <Larry.Finger@lwfinger.net>
Thanks,
Larry
>
> diff --git a/drivers/net/wireless/broadcom/b43legacy/dma.c b/drivers/net/wireless/broadcom/b43legacy/dma.c
> index 2ce1537d983c..0c2de20622e3 100644
> --- a/drivers/net/wireless/broadcom/b43legacy/dma.c
> +++ b/drivers/net/wireless/broadcom/b43legacy/dma.c
> @@ -797,43 +797,6 @@ void b43legacy_dma_free(struct b43legacy_wldev *dev)
> dma->tx_ring0 = NULL;
> }
>
> -static int b43legacy_dma_set_mask(struct b43legacy_wldev *dev, u64 mask)
> -{
> - u64 orig_mask = mask;
> - bool fallback = false;
> - int err;
> -
> - /* Try to set the DMA mask. If it fails, try falling back to a
> - * lower mask, as we can always also support a lower one. */
> - while (1) {
> - err = dma_set_mask_and_coherent(dev->dev->dma_dev, mask);
> - if (!err)
> - break;
> - if (mask == DMA_BIT_MASK(64)) {
> - mask = DMA_BIT_MASK(32);
> - fallback = true;
> - continue;
> - }
> - if (mask == DMA_BIT_MASK(32)) {
> - mask = DMA_BIT_MASK(30);
> - fallback = true;
> - continue;
> - }
> - b43legacyerr(dev->wl, "The machine/kernel does not support "
> - "the required %u-bit DMA mask\n",
> - (unsigned int)dma_mask_to_engine_type(orig_mask));
> - return -EOPNOTSUPP;
> - }
> - if (fallback) {
> - b43legacyinfo(dev->wl, "DMA mask fallback from %u-bit to %u-"
> - "bit\n",
> - (unsigned int)dma_mask_to_engine_type(orig_mask),
> - (unsigned int)dma_mask_to_engine_type(mask));
> - }
> -
> - return 0;
> -}
> -
> int b43legacy_dma_init(struct b43legacy_wldev *dev)
> {
> struct b43legacy_dma *dma = &dev->dma;
> @@ -844,7 +807,7 @@ int b43legacy_dma_init(struct b43legacy_wldev *dev)
>
> dmamask = supported_dma_mask(dev);
> type = dma_mask_to_engine_type(dmamask);
> - err = b43legacy_dma_set_mask(dev, dmamask);
> + err = dma_set_mask_and_coherent(dev->dev->dma_dev, dmamask);
> if (err) {
> #ifdef CONFIG_B43LEGACY_PIO
> b43legacywarn(dev->wl, "DMA for this device not supported. "
>
^ permalink raw reply
* BUG: unable to handle kernel NULL pointer dereference in corrupted (4)
From: syzbot @ 2019-06-25 21:47 UTC (permalink / raw)
To: ast, daniel, john.fastabend, linux-kernel, netdev, syzkaller-bugs
Hello,
syzbot found the following crash on:
HEAD commit: 4b972a01 Linux 5.2-rc6
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=17852b6ea00000
kernel config: https://syzkaller.appspot.com/x/.config?x=e7c31a94f66cc0aa
dashboard link: https://syzkaller.appspot.com/bug?extid=4b5d77fdf765668f9eba
compiler: gcc (GCC) 9.0.0 20181231 (experimental)
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=15252769a00000
The bug was bisected to:
commit e9db4ef6bf4ca9894bb324c76e01b8f1a16b2650
Author: John Fastabend <john.fastabend@gmail.com>
Date: Sat Jun 30 13:17:47 2018 +0000
bpf: sockhash fix omitted bucket lock in sock_close
bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=146ae35ea00000
final crash: https://syzkaller.appspot.com/x/report.txt?x=166ae35ea00000
console output: https://syzkaller.appspot.com/x/log.txt?x=126ae35ea00000
IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+4b5d77fdf765668f9eba@syzkaller.appspotmail.com
Fixes: e9db4ef6bf4c ("bpf: sockhash fix omitted bucket lock in sock_close")
BUG: kernel NULL pointer dereference, address: 00000000000000fc
---
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.
For information about bisection process see: https://goo.gl/tpsmEJ#bisection
syzbot can test patches for this bug, for details see:
https://goo.gl/tpsmEJ#testing-patches
^ permalink raw reply
* Re: [for-next V2 01/10] linux/dim: Move logic to dim.h
From: Sagi Grimberg @ 2019-06-25 21:53 UTC (permalink / raw)
To: Saeed Mahameed, David S. Miller, Doug Ledford, Jason Gunthorpe
Cc: Leon Romanovsky, Or Gerlitz, Tal Gilboa, netdev@vger.kernel.org,
linux-rdma@vger.kernel.org
In-Reply-To: <20190625205701.17849-2-saeedm@mellanox.com>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
^ 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 21:53 UTC (permalink / raw)
To: Vladimir Oltean
Cc: René van Dorst, sean.wang, f.fainelli, davem, matthias.bgg,
andrew, vivien.didelot, frank-w, netdev, linux-mediatek,
linux-mips
In-Reply-To: <6f80325d-4b42-6174-e050-48626f7a3662@gmail.com>
On Tue, Jun 25, 2019 at 11:24:01PM +0300, Vladimir Oltean wrote:
> Hi Russell,
>
> On 6/24/19 6:39 PM, Russell King - ARM Linux admin wrote:
> > 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.
> >
>
> What do you mean exactly that state->link is not for use, is that true in
> general?
Yes. mac_config() should not touch it; it is not always in a defined
state. For example, if you set modes via ethtool (the
ethtool_ksettings_set API) then state->link will probably contain
zero irrespective of the true link state.
It exists in this structure because it was convenient to just use one
structure to store all the link information in various parts of the
code, and when requesting the negotiated in-band MAC configuration.
I've come to the conclusion that that decision was a mistake, based
on patches such as the above mistakenly thinking that everything in
the state structure is fair game. I've since updated the docs to
explicitly spell it out, but I'm also looking at the feasibility of
changing the mac_config() interface entirely - splitting it into two
(mac_config_fixed() and mac_config_inband()) and passing only the
appropriate parameters to each.
However, having looked at that, I think such a change will make some
MAC drivers quite a bit more complicated - having all the config
steps in one method appears to make the configuration of MAC drivers
easier (eg, mvneta, mvpp2.)
> In drivers/net/dsa/sja1105/sja1105_main.c, if I remove the "if
> (!state->link)" guard, I see PHYLINK calls with a SPEED_UNKNOWN argument for
> ports that are BR_STATE_DISABLED. Is that normal?
This looks like another driver which has been converted to phylink
without my review; I certainly wasn't aware of it. It gets a few
things wrong, such as:
1) not checking state->interface in the validate callback - so it
is basically saying that it can support any PHY interface mode
that the kernel happens to support.
2) if phylink is configured to use in-band, then state->speed is
undefined; this driver will fail. (If folk don't want to support
that, we ought to have a way to tell phylink to reject anything
that attempts to set it to in-band mode!)
3) it doesn't implement phylink_mac_link_state DSA ops, so it doesn't
support SGMII or 802.3z phy interface modes (see 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
^ permalink raw reply
* Re: [for-next V2 02/10] linux/dim: Remove "net" prefix from internal DIM members
From: Sagi Grimberg @ 2019-06-25 21:53 UTC (permalink / raw)
To: Saeed Mahameed, David S. Miller, Doug Ledford, Jason Gunthorpe
Cc: Leon Romanovsky, Or Gerlitz, Tal Gilboa, netdev@vger.kernel.org,
linux-rdma@vger.kernel.org
In-Reply-To: <20190625205701.17849-3-saeedm@mellanox.com>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
^ permalink raw reply
* Re: [for-next V2 03/10] linux/dim: Rename externally exposed macros
From: Sagi Grimberg @ 2019-06-25 21:54 UTC (permalink / raw)
To: Saeed Mahameed, David S. Miller, Doug Ledford, Jason Gunthorpe
Cc: Leon Romanovsky, Or Gerlitz, Tal Gilboa, netdev@vger.kernel.org,
linux-rdma@vger.kernel.org
In-Reply-To: <20190625205701.17849-4-saeedm@mellanox.com>
This can be squashed to the prev one, otherwise
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
^ permalink raw reply
* Re: [for-next V2 04/10] linux/dim: Rename net_dim_sample() to net_dim_update_sample()
From: Sagi Grimberg @ 2019-06-25 21:55 UTC (permalink / raw)
To: Saeed Mahameed, David S. Miller, Doug Ledford, Jason Gunthorpe
Cc: Leon Romanovsky, Or Gerlitz, Tal Gilboa, netdev@vger.kernel.org,
linux-rdma@vger.kernel.org
In-Reply-To: <20190625205701.17849-5-saeedm@mellanox.com>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
^ permalink raw reply
* Re: [for-next V2 05/10] linux/dim: Rename externally used net_dim members
From: Sagi Grimberg @ 2019-06-25 21:57 UTC (permalink / raw)
To: Saeed Mahameed, David S. Miller, Doug Ledford, Jason Gunthorpe
Cc: Leon Romanovsky, Or Gerlitz, Tal Gilboa, netdev@vger.kernel.org,
linux-rdma@vger.kernel.org
In-Reply-To: <20190625205701.17849-6-saeedm@mellanox.com>
Question, do any other nics use or plan to use this?
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
^ permalink raw reply
* [PATCH v4 net] af_packet: Block execution of tasks waiting for transmit to complete in AF_PACKET
From: Neil Horman @ 2019-06-25 21:57 UTC (permalink / raw)
To: netdev; +Cc: Neil Horman, Matteo Croce, David S. Miller, Willem de Bruijn
In-Reply-To: <20190619202533.4856-1-nhorman@tuxdriver.com>
When an application is run that:
a) Sets its scheduler to be SCHED_FIFO
and
b) Opens a memory mapped AF_PACKET socket, and sends frames with the
MSG_DONTWAIT flag cleared, its possible for the application to hang
forever in the kernel. This occurs because when waiting, the code in
tpacket_snd calls schedule, which under normal circumstances allows
other tasks to run, including ksoftirqd, which in some cases is
responsible for freeing the transmitted skb (which in AF_PACKET calls a
destructor that flips the status bit of the transmitted frame back to
available, allowing the transmitting task to complete).
However, when the calling application is SCHED_FIFO, its priority is
such that the schedule call immediately places the task back on the cpu,
preventing ksoftirqd from freeing the skb, which in turn prevents the
transmitting task from detecting that the transmission is complete.
We can fix this by converting the schedule call to a completion
mechanism. By using a completion queue, we force the calling task, when
it detects there are no more frames to send, to schedule itself off the
cpu until such time as the last transmitted skb is freed, allowing
forward progress to be made.
Tested by myself and the reporter, with good results
Appies to the net tree
Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
Reported-by: Matteo Croce <mcroce@redhat.com>
CC: "David S. Miller" <davem@davemloft.net>
CC: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Change Notes:
V1->V2:
Enhance the sleep logic to support being interruptible and
allowing for honoring to SK_SNDTIMEO (Willem de Bruijn)
V2->V3:
Rearrage the point at which we wait for the completion queue, to
avoid needing to check for ph/skb being null at the end of the loop.
Also move the complete call to the skb destructor to avoid needing to
modify __packet_set_status. Also gate calling complete on
packet_read_pending returning zero to avoid multiple calls to complete.
(Willem de Bruijn)
Move timeo computation within loop, to re-fetch the socket
timeout since we also use the timeo variable to record the return code
from the wait_for_complete call (Neil Horman)
V3->V4:
Willem has requested that the control flow be restored to the
previous state. Doing so lets us eliminate the need for the
po->wait_on_complete flag variable, and lets us get rid of the
packet_next_frame function, but introduces another complexity.
Specifically, but using the packet pending count, we can, if an
applications calls sendmsg multiple times with MSG_DONTWAIT set, each
set of transmitted frames, when complete, will cause
tpacket_destruct_skb to issue a complete call, for which there will
never be a wait_on_completion call. This imbalance will lead to any
future call to wait_for_completion here to return early, when the frames
they sent may not have completed. To correct this, we need to re-init
the completion queue on every call to tpacket_snd before we enter the
loop so as to ensure we wait properly for the frames we send in this
iteration.
Change the timeout and interrupted gotos to out_put rather than
out_status so that we don't try to free a non-existant skb
Clean up some extra newlines (Willem de Bruijn)
---
net/packet/af_packet.c | 20 +++++++++++++++++---
net/packet/internal.h | 1 +
2 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index a29d66da7394..a7ca6a003ebe 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -2401,6 +2401,9 @@ static void tpacket_destruct_skb(struct sk_buff *skb)
ts = __packet_set_timestamp(po, ph, skb);
__packet_set_status(po, ph, TP_STATUS_AVAILABLE | ts);
+
+ if (!packet_read_pending(&po->tx_ring))
+ complete(&po->skb_completion);
}
sock_wfree(skb);
@@ -2585,7 +2588,7 @@ static int tpacket_parse_header(struct packet_sock *po, void *frame,
static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
{
- struct sk_buff *skb;
+ struct sk_buff *skb = NULL;
struct net_device *dev;
struct virtio_net_hdr *vnet_hdr = NULL;
struct sockcm_cookie sockc;
@@ -2600,6 +2603,7 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
int len_sum = 0;
int status = TP_STATUS_AVAILABLE;
int hlen, tlen, copylen = 0;
+ long timeo = 0;
mutex_lock(&po->pg_vec_lock);
@@ -2646,12 +2650,21 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
if ((size_max > dev->mtu + reserve + VLAN_HLEN) && !po->has_vnet_hdr)
size_max = dev->mtu + reserve + VLAN_HLEN;
+ reinit_completion(&po->skb_completion);
+
do {
ph = packet_current_frame(po, &po->tx_ring,
TP_STATUS_SEND_REQUEST);
if (unlikely(ph == NULL)) {
- if (need_wait && need_resched())
- schedule();
+ if (need_wait && skb) {
+ timeo = sock_sndtimeo(&po->sk, msg->msg_flags & MSG_DONTWAIT);
+ timeo = wait_for_completion_interruptible_timeout(&po->skb_completion, timeo);
+ if (timeo <= 0) {
+ err = !timeo ? -ETIMEDOUT : -ERESTARTSYS;
+ goto out_put;
+ }
+ }
+ /* check for additional frames */
continue;
}
@@ -3207,6 +3220,7 @@ static int packet_create(struct net *net, struct socket *sock, int protocol,
sock_init_data(sock, sk);
po = pkt_sk(sk);
+ init_completion(&po->skb_completion);
sk->sk_family = PF_PACKET;
po->num = proto;
po->xmit = dev_queue_xmit;
diff --git a/net/packet/internal.h b/net/packet/internal.h
index 3bb7c5fb3bff..c70a2794456f 100644
--- a/net/packet/internal.h
+++ b/net/packet/internal.h
@@ -128,6 +128,7 @@ struct packet_sock {
unsigned int tp_hdrlen;
unsigned int tp_reserve;
unsigned int tp_tstamp;
+ struct completion skb_completion;
struct net_device __rcu *cached_dev;
int (*xmit)(struct sk_buff *skb);
struct packet_type prot_hook ____cacheline_aligned_in_smp;
--
2.20.1
^ permalink raw reply related
* Re: [PATCH v3 net] af_packet: Block execution of tasks waiting for transmit to complete in AF_PACKET
From: Willem de Bruijn @ 2019-06-25 21:59 UTC (permalink / raw)
To: Neil Horman; +Cc: Network Development, Matteo Croce, David S. Miller
In-Reply-To: <20190625162028.GC29902@hmswarspite.think-freely.org>
On Tue, Jun 25, 2019 at 12:20 PM Neil Horman <nhorman@tuxdriver.com> wrote:
>
> On Tue, Jun 25, 2019 at 09:37:17AM -0400, Willem de Bruijn wrote:
> > On Tue, Jun 25, 2019 at 7:03 AM Neil Horman <nhorman@tuxdriver.com> wrote:
> > >
> > > On Mon, Jun 24, 2019 at 06:15:29PM -0400, Willem de Bruijn wrote:
> > > > > > > + if (need_wait && !packet_next_frame(po, &po->tx_ring, TP_STATUS_SEND_REQUEST)) {
> > > > > > > + po->wait_on_complete = 1;
> > > > > > > + timeo = sock_sndtimeo(&po->sk, msg->msg_flags & MSG_DONTWAIT);
> > > > > >
> > > > > > This resets timeout on every loop. should only set above the loop once.
> > > > > >
> > > > > I explained exactly why I did that in the change log. Its because I reuse the
> > > > > timeout variable to get the return value of the wait_for_complete call.
> > > > > Otherwise I need to add additional data to the stack, which I don't want to do.
> > > > > Sock_sndtimeo is an inline function and really doesn't add any overhead to this
> > > > > path, so I see no reason not to reuse the variable.
> > > >
> > > > The issue isn't the reuse. It is that timeo is reset to sk_sndtimeo
> > > > each time. Whereas wait_for_common and its variants return the
> > > > number of jiffies left in case the loop needs to sleep again later.
> > > >
> > > > Reading sock_sndtimeo once and passing it to wait_.. repeatedly is a
> > > > common pattern across the stack.
> > > >
> > > But those patterns are unique to those situations. For instance, in
> > > tcp_sendmsg_locked, we aquire the value of the socket timeout, and use that to
> > > wait for the entire message send operation to complete, which consists of
> > > potentially several blocking operations (waiting for the tcp connection to be
> > > established, waiting for socket memory, etc). In that situation we want to wait
> > > for all of those operations to complete to send a single message, and fail if
> > > they exceed the timeout in aggregate. The semantics are different with
> > > AF_PACKET. In this use case, the message is in effect empty, and just used to
> > > pass some control information. tpacket_snd, sends as many frames from the
> > > memory mapped buffer as possible, and on each iteration we want to wait for the
> > > specified timeout for those frames to complete sending. I think resetting the
> > > timeout on each wait instance is the right way to go here.
> >
> > I disagree. If a SO_SNDTIMEO of a given time is set, the thread should
> > not wait beyond that. Else what is the point of passing a specific
> > duration in the syscall?
> >
> I can appreciate that, but you said yourself that you wanted to minimize this
> change. The current behavior of AF_PACKET is to:
> a) check for their being no more packets ready to send
> b) if (a) is false we schedule() (nominally allowing other tasks to run)
> c) when (b) is complete, check for additional frames to send, and exit if there
> are not, otherwise, continue sending and waiting
>
> In that model, a single task calling sendmsg can run in the kernel indefinately,
> if userspace continues to fill the memory mapped buffer
>
> Given that model, resetting the timeout on each call to wait_for_completion
> keeps that behavior. In fact, if we allow the timeout value to get continuously
> decremented, it will be possible for a call to sendmsg in this protocol to
> _always_ return -ETIMEDOUT (if we keep the buffer full in userspace long enough,
> then the sending task will eventually decrement timeo to zero, and force an
> -ETIMEDOUT call).
>
> I'm convinced that resetting the timeout here is the right way to go.
It upholds the contract, but extends when userspace appends to the
ring. Okay, yes, that makes sense.
> > Btw, we can always drop the timeo and go back to unconditional (bar
> > signals) waiting.
> >
> We could, but it would be nice to implement the timeout feature here if
> possible.
>
> > >
> > > > > > > @@ -2728,6 +2755,11 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
> > > > > > > err = net_xmit_errno(err);
> > > > > > > if (err && __packet_get_status(po, ph) ==
> > > > > > > TP_STATUS_AVAILABLE) {
> > > > > > > + /* re-init completion queue to avoid subsequent fallthrough
> > > > > > > + * on a future thread calling wait_on_complete_interruptible_timeout
> > > > > > > + */
> > > > > > > + po->wait_on_complete = 0;
> > > > > >
> > > > > > If setting where sleeping, no need for resetting if a failure happens
> > > > > > between those blocks.
> > > > > >
> > > > > > > + init_completion(&po->skb_completion);
> > > > > >
> > > > > > no need to reinit between each use?
> > > > > >
> > > > > I explained exactly why I did this in the comment above. We have to set
> > > > > wait_for_complete prior to calling transmit, so as to ensure that we call
> > > > > wait_for_completion before we exit the loop. However, in this error case, we
> > > > > exit the loop prior to calling wait_for_complete, so we need to reset the
> > > > > completion variable and the wait_for_complete flag. Otherwise we will be in a
> > > > > case where, on the next entrace to this loop we will have a completion variable
> > > > > with completion->done > 0, meaning the next wait will be a fall through case,
> > > > > which we don't want.
> > > >
> > > > By moving back to the point where schedule() is called, hopefully this
> > > > complexity automatically goes away. Same as my comment to the line
> > > > immediately above.
> > > >
> > > Its going to change what the complexity is, actually. I was looking at this
> > > last night, and I realized that your assertion that we could remove
> > > packet_next_frame came at a cost. This is because we need to determine if we
> > > have to wait before we call po->xmit, but we need to actually do the wait after
> > > po->xmit
> >
> > Why? The existing method using schedule() doesn't.
> >
> Because the existing method using schedule doesn't have to rely on an
> asynchronous event to wake the sending task back up. Specifically, we need to
> set some state that indicates tpacket_destruct_skb should call complete, and
> since tpacket_destruct_skb is called asynchronously in a separate execution
> context, we need to ensure there is no race condition whereby we execute
> tpacket_destruct_skb before we set the state that we also use to determine if we
> should call wait_for_complete. ie:
> 1) tpacket_send_skb calls po->xmit
> 2) tpacket_send_skb loops around and checks to see if there are more packets to
> send. If not and if need_wait is set we call wait_for_complete
>
> If tpacket_destruct_skb is called after (2), we're fine. But if its called
> between (1) and (2), then tpacket_destruct_skb won't call complete (because
> wait_on_complete isn't set yet), causing a hang.
>
> Because you wanted to remove packet_next_frame, we have no way to determine if
> we're done sending frames prior to calling po->xmit, which is the point past
> which tpacket_destruct_skb might be called, so now I have to find an alternate
> state condition to key off of.
>
>
> > Can we not just loop while sending and sleep immediately when
> > __packet_get_status returns !TP_STATUS_AVAILABLE?
> >
> > I don't understand the need to probe the next packet to send instead
> > of the current.
> >
> See above, we can definately check the return of ph at the top of the loop, and
> sleep if its NULL, but in so doing we cant use po->wait_on_complete as a gating
> variable because we've already called po->xmit. Once we call that function, if
> we haven't already set po->wait_on_complete to true, its possible
> tpacket_destruct_skb will already have been called before we get to the point
> where we check wait_for_completion. That means we will wait on a completion
> variable that never gets completed, so I need to find a new way to track weather
> or not we are waiting. Its entirely doable, I'm sure, its just not as straight
> forward as your making it out to be.
>
> > This seems to be the crux of the disagreement. My guess is that it has
> > to do with setting wait_on_complete, but I don't see the problem. It
> > can be set immediately before going to sleep.
> >
> The reason has to do with the fact that
> tpacket_destruct_skb can be called from ksoftirq context (i.e. it will be called
> asynchrnously, not from the thread running in tpacket_snd). Condsider this
> flow, assuming we do as you suggest, and just set po->wait_on_complete=1
> immediately prior to calling wait_for_complete_interruptible_timeout:
>
> 1) tpacket_snd gets a frame, builds the skb for transmission
> 2) tpakcket_snd calls po->xmit(skb), sending the frame to the driver
> 3) tpacket_snd, iterates through back to the top of the loop, and determines
> that we need to wait for the previous packet to complete
> 4) The driver gets a tx completion interrupt, interrupting the cpu on which we
> are executing, and calls kfree_skb_irq on the skb we just transmitted
> 5) the ksoftirq runs on the cpu, calling kfree_skb on our skb
> 6) (5) calls skb->destruct (which is tpakcet_skb_destruct)
> 7) tpacket_skb_destruct executes, sees that po->wait_on_completion is zero, and
> skips calling complete()
> 8) the thread running tpacket_snd gets scheduled back on the cpu and now sets
> po->wait_on_complete, then calls wait_for_completion_interruptible_timeout, but
> since the skb we are waiting to complete has already been freed, we will never
> get the completion notification, and so we will wait for the maximal timeout,
> which is absolutely not what we want.
>
> Interestingly (Ironically), that control flow will never happen in the use case
> I'm trying to fix here, because its SCHED_FIFO, and will run until such time as
> we call wait_for_completion_interuptible_timeout, so in this case we're safe.
> But in the nominal case, where the sending task is acturally SCHED_OTHER, the
> above can aboslutely happen, and thats very broken.
>
> > I don't meant to draw this out, btw, or add to your workload. If you
> > prefer, I can take a stab at my (admittedly hand-wavy) suggestion.
> >
> No, I have another method in flight that I'm testing now, it removes the
> po->wait_on_complete variable entirely, checking instead to make sure that
> we've:
> a) sent at least one frame
> and
> b) that we have a positive pending count
> and
> c) that we need to wait
> and
> d) that we have no more frames to send
>
> This is why I was saying that your idea can be done, but it trades one form of
> complexity for another.
Thanks for the detailed explanation of the races.
I was thinking this through, but just see V4. Will take a look at that.
^ permalink raw reply
* Re: [for-next V2 08/10] linux/dim: Implement rdma_dim
From: Sagi Grimberg @ 2019-06-25 22:02 UTC (permalink / raw)
To: Saeed Mahameed, David S. Miller, Doug Ledford, Jason Gunthorpe
Cc: Leon Romanovsky, Or Gerlitz, Tal Gilboa, netdev@vger.kernel.org,
linux-rdma@vger.kernel.org, Yamin Friedman, Max Gurtovoy
In-Reply-To: <20190625205701.17849-9-saeedm@mellanox.com>
> +void rdma_dim(struct dim *dim, u64 completions)
> +{
> + struct dim_sample *curr_sample = &dim->measuring_sample;
> + struct dim_stats curr_stats;
> + u32 nevents;
> +
> + dim_update_sample_with_comps(curr_sample->event_ctr + 1,
> + curr_sample->pkt_ctr,
> + curr_sample->byte_ctr,
> + curr_sample->comp_ctr + completions,
> + &dim->measuring_sample);
If this is the only caller, why add pkt_ctr and byte_ctr at all?
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox