* Re: [Xen-devel] [PATCH] xen-netback: fix occasional leak of grant ref mappings under memory pressure
From: Andrew Cooper @ 2019-02-28 9:50 UTC (permalink / raw)
To: Igor Druzhinin, xen-devel, netdev, linux-kernel
Cc: paul.durrant, wei.liu2, davem
In-Reply-To: <1551319382-32595-1-git-send-email-igor.druzhinin@citrix.com>
On 28/02/2019 02:03, Igor Druzhinin wrote:
> Zero-copy callback flag is not yet set on frag list skb at the moment
> xenvif_handle_frag_list() returns -ENOMEM. This eventually results in
> leaking grant ref mappings since xenvif_zerocopy_callback() is never
> called for these fragments. Those eventually build up and cause Xen
> to kill Dom0 as the slots get reused for new mappings.
Its worth pointing out what (debug) Xen notices is dom0 performing
implicit grant unmap.
~Andrew
^ permalink raw reply
* [PATCH net-next] cxgb4vf: Enter debugging mode if FW is inaccessible
From: Arjun Vynipadath @ 2019-02-28 10:03 UTC (permalink / raw)
To: netdev, davem; +Cc: nirranjan, indranil, dt, Arjun Vynipadath, Vishal Kulkarni
If we are not able to reach firmware, enter debugging mode that will
help us to get adapter logs.
Signed-off-by: Arjun Vynipadath <arjun@chelsio.com>
Signed-off-by: Vishal Kulkarni <vishal@chelsio.com>
---
drivers/net/ethernet/chelsio/cxgb4vf/adapter.h | 1 +
.../net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.c | 45 +++++++++++++++-------
2 files changed, 33 insertions(+), 13 deletions(-)
diff --git a/drivers/net/ethernet/chelsio/cxgb4vf/adapter.h b/drivers/net/ethernet/chelsio/cxgb4vf/adapter.h
index 26f48a14d2f9..86cf68fceceb 100644
--- a/drivers/net/ethernet/chelsio/cxgb4vf/adapter.h
+++ b/drivers/net/ethernet/chelsio/cxgb4vf/adapter.h
@@ -411,6 +411,7 @@ enum { /* adapter flags */
USING_MSIX = (1UL << 2),
QUEUES_BOUND = (1UL << 3),
ROOT_NO_RELAXED_ORDERING = (1UL << 4),
+ FW_OK = (1UL << 5),
};
/*
diff --git a/drivers/net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.c b/drivers/net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.c
index d22df0f6c088..99b22670db3d 100644
--- a/drivers/net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.c
+++ b/drivers/net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.c
@@ -845,6 +845,13 @@ static int cxgb4vf_open(struct net_device *dev)
struct adapter *adapter = pi->adapter;
/*
+ * If we don't have a connection to the firmware there's nothing we
+ * can do.
+ */
+ if (!(adapter->flags & FW_OK))
+ return -ENXIO;
+
+ /*
* If this is the first interface that we're opening on the "adapter",
* bring the "adapter" up now.
*/
@@ -2720,6 +2727,7 @@ static int adap_init0(struct adapter *adapter)
*/
size_nports_qsets(adapter);
+ adapter->flags |= FW_OK;
return 0;
}
@@ -3084,7 +3092,9 @@ static int cxgb4vf_pci_probe(struct pci_dev *pdev,
err = adap_init0(adapter);
if (err)
- goto err_unmap_bar;
+ dev_err(&pdev->dev,
+ "Adapter initialization failed, error %d. Continuing in debug mode\n",
+ err);
/* Initialize hash mac addr list */
INIT_LIST_HEAD(&adapter->mac_hlist);
@@ -3109,13 +3119,6 @@ static int cxgb4vf_pci_probe(struct pci_dev *pdev,
break;
port_id = ffs(pmask) - 1;
pmask &= ~(1 << port_id);
- viid = t4vf_alloc_vi(adapter, port_id);
- if (viid < 0) {
- dev_err(&pdev->dev, "cannot allocate VI for port %d:"
- " err=%d\n", port_id, viid);
- err = viid;
- goto err_free_dev;
- }
/*
* Allocate our network device and stitch things together.
@@ -3123,7 +3126,6 @@ static int cxgb4vf_pci_probe(struct pci_dev *pdev,
netdev = alloc_etherdev_mq(sizeof(struct port_info),
MAX_PORT_QSETS);
if (netdev == NULL) {
- t4vf_free_vi(adapter, viid);
err = -ENOMEM;
goto err_free_dev;
}
@@ -3133,7 +3135,6 @@ static int cxgb4vf_pci_probe(struct pci_dev *pdev,
pi->adapter = adapter;
pi->pidx = pidx;
pi->port_id = port_id;
- pi->viid = viid;
/*
* Initialize the starting state of our "port" and register
@@ -3159,6 +3160,23 @@ static int cxgb4vf_pci_probe(struct pci_dev *pdev,
netdev->dev_port = pi->port_id;
/*
+ * If we haven't been able to contact the firmware, there's
+ * nothing else we can do for this "port" ...
+ */
+ if (!(adapter->flags & FW_OK))
+ continue;
+
+ viid = t4vf_alloc_vi(adapter, port_id);
+ if (viid < 0) {
+ dev_err(&pdev->dev,
+ "cannot allocate VI for port %d: err=%d\n",
+ port_id, viid);
+ err = viid;
+ goto err_free_dev;
+ }
+ pi->viid = viid;
+
+ /*
* Initialize the hardware/software state for the port.
*/
err = t4vf_port_init(adapter, pidx);
@@ -3302,13 +3320,13 @@ static int cxgb4vf_pci_probe(struct pci_dev *pdev,
if (netdev == NULL)
continue;
pi = netdev_priv(netdev);
- t4vf_free_vi(adapter, pi->viid);
+ if (pi->viid)
+ t4vf_free_vi(adapter, pi->viid);
if (test_bit(pidx, &adapter->registered_device_map))
unregister_netdev(netdev);
free_netdev(netdev);
}
-err_unmap_bar:
if (!is_t4(adapter->params.chip))
iounmap(adapter->bar2);
@@ -3381,7 +3399,8 @@ static void cxgb4vf_pci_remove(struct pci_dev *pdev)
continue;
pi = netdev_priv(netdev);
- t4vf_free_vi(adapter, pi->viid);
+ if (pi->viid)
+ t4vf_free_vi(adapter, pi->viid);
free_netdev(netdev);
}
iounmap(adapter->regs);
--
2.9.5
^ permalink raw reply related
* RE: [PATCH net-next] net: sched: pie: fix 64-bit division
From: David Laight @ 2019-02-28 10:10 UTC (permalink / raw)
To: 'Leslie Monis'; +Cc: netdev@vger.kernel.org
In-Reply-To: <20190227161228.GA2014@Inspiron-3521>
From: Leslie Monis
> Sent: 27 February 2019 16:12
>
> On Wed, Feb 27, 2019 at 10:11:14AM +0000, David Laight wrote:
> > From: Leslie Monis
> > > Sent: 27 February 2019 01:00
> > > Use div_u64() to resolve build failures on 32-bit platforms.
> > >
> > > Fixes: 3f7ae5f3dc52 ("net: sched: pie: add more cases to auto-tune alpha and beta")
> > > Signed-off-by: Leslie Monis <lesliemonis@gmail.com>
> > > ---
> > > net/sched/sch_pie.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/net/sched/sch_pie.c b/net/sched/sch_pie.c
> > > index 4c0670b6aec1..f93cfe034c72 100644
> > > --- a/net/sched/sch_pie.c
> > > +++ b/net/sched/sch_pie.c
> > > @@ -429,7 +429,7 @@ static void calculate_probability(struct Qdisc *sch)
> > > */
> > >
> > > if (qdelay == 0 && qdelay_old == 0 && update_prob)
> > > - q->vars.prob = (q->vars.prob * 98) / 100;
> > > + q->vars.prob = 98 * div_u64(q->vars.prob, 100);
> >
> > This has significantly different rounding after the change.
> > The result for small values is very different.
> > The alterative:
> > q->vars.prob -= div_u64(q->vars.prob, 50);
> > is much nearer to the original - but never goes to zero.
> >
> > If the 98% decay factor isn't critical then you could remove
> > 1/64th or 1/32nd + 1/16th to avoid the slow division.
> >
> > David
>
> Hi David,
>
> You're right, the change does make the result for small
> values different. I made it anyway as the probability
> value is scaled by u64. It is safe to say that q->vars.prob
> holds relatively large values (in its scaled form) in all
> cases where it isn't 0.
>
> But, I think we can avoid the slow division here. RFC 8033
> does say that using (1 - 1/64) should be sufficient. This
> will give us:
> q-vars.prob -= q->vars.prob >> 6;
> which I feel would be much better. What do you reckon?
I think I'd leave it as a division - the compiler should do the shift.
So:
/* Scale by 98.4% */
q-vars.prob -= q->vars.prob / 64u;
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply
* Re: [PATCH v2 2/2] dt-bindings: net: can: binding for CTU CAN FD open-source IP core.
From: Michal Vokáč @ 2019-02-28 10:13 UTC (permalink / raw)
To: pisa
Cc: devicetree, mkl, linux-can, wg, davem, robh+dt, mark.rutland,
netdev, linux-kernel, martin.jerabek01, ondrej.ille
In-Reply-To: <2b529d1574916b50e7647281af87df33442b7549.1551287365.git.pisa@cmp.felk.cvut.cz>
On 27. 02. 19 18:26, pisa@cmp.felk.cvut.cz wrote:
> From: Pavel Pisa <pisa@cmp.felk.cvut.cz>
Ahoj Pavle,
I can not comment on the content of the binding but it is a good
practice to add at least a short description to each commit even
if it should be the subject said differently.
Anyway, hats off to all of you, great work!
Greetings back to my alma mater,
Michal
>
> Signed-off-by: Pavel Pisa <pisa@cmp.felk.cvut.cz>
> ---
^ permalink raw reply
* Re: [PATCH net-next] net: sched: pie: fix 64-bit division
From: Leslie Monis @ 2019-02-28 10:16 UTC (permalink / raw)
To: David Laight; +Cc: netdev@vger.kernel.org
In-Reply-To: <387a2fe8539c4de7bf3f97aadbd9b924@AcuMS.aculab.com>
On Thu, Feb 28, 2019 at 10:10:33AM +0000, David Laight wrote:
> From: Leslie Monis
> > Sent: 27 February 2019 16:12
> >
> > On Wed, Feb 27, 2019 at 10:11:14AM +0000, David Laight wrote:
> > > From: Leslie Monis
> > > > Sent: 27 February 2019 01:00
> > > > Use div_u64() to resolve build failures on 32-bit platforms.
> > > >
> > > > Fixes: 3f7ae5f3dc52 ("net: sched: pie: add more cases to auto-tune alpha and beta")
> > > > Signed-off-by: Leslie Monis <lesliemonis@gmail.com>
> > > > ---
> > > > net/sched/sch_pie.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/net/sched/sch_pie.c b/net/sched/sch_pie.c
> > > > index 4c0670b6aec1..f93cfe034c72 100644
> > > > --- a/net/sched/sch_pie.c
> > > > +++ b/net/sched/sch_pie.c
> > > > @@ -429,7 +429,7 @@ static void calculate_probability(struct Qdisc *sch)
> > > > */
> > > >
> > > > if (qdelay == 0 && qdelay_old == 0 && update_prob)
> > > > - q->vars.prob = (q->vars.prob * 98) / 100;
> > > > + q->vars.prob = 98 * div_u64(q->vars.prob, 100);
> > >
> > > This has significantly different rounding after the change.
> > > The result for small values is very different.
> > > The alterative:
> > > q->vars.prob -= div_u64(q->vars.prob, 50);
> > > is much nearer to the original - but never goes to zero.
> > >
> > > If the 98% decay factor isn't critical then you could remove
> > > 1/64th or 1/32nd + 1/16th to avoid the slow division.
> > >
> > > David
> >
> > Hi David,
> >
> > You're right, the change does make the result for small
> > values different. I made it anyway as the probability
> > value is scaled by u64. It is safe to say that q->vars.prob
> > holds relatively large values (in its scaled form) in all
> > cases where it isn't 0.
> >
> > But, I think we can avoid the slow division here. RFC 8033
> > does say that using (1 - 1/64) should be sufficient. This
> > will give us:
> > q-vars.prob -= q->vars.prob >> 6;
> > which I feel would be much better. What do you reckon?
>
> I think I'd leave it as a division - the compiler should do the shift.
> So:
> /* Scale by 98.4% */
> q-vars.prob -= q->vars.prob / 64u;
>
> David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>
Alright, I'll prepare a patch for this. May I credit you
with a Suggested-by tag?
Thanks,
Leslie
^ permalink raw reply
* Re: [PATCH net-next v3 4/4] net/mlx5e: Return -EOPNOTSUPP when attempting to offload an unsupported action
From: Tonghao Zhang @ 2019-02-28 10:22 UTC (permalink / raw)
To: Roi Dayan; +Cc: Saeed Mahameed, gerlitz.or@gmail.com, netdev@vger.kernel.org
In-Reply-To: <37c79899-ebb7-4caf-d76a-f2a9bcd49af2@mellanox.com>
On Wed, Feb 27, 2019 at 10:01 PM Roi Dayan <roid@mellanox.com> wrote:
>
>
>
> On 26/02/2019 14:27, xiangxia.m.yue@gmail.com wrote:
> > From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> >
> > * Now the encapsulation is not supported for mlx5 VFs. When we try to
> > offload that action, the -EINVAL is returned, but not -EOPNOTSUPP.
> > This patch changes the returned value and ignore to confuse user.
> > The command is shown as below [1].
> >
> > * When max modify header action is zero, we return -EOPNOTSUPP
> > directly. In this way, we can ignore wrong message info (e.g.
> > "mlx5: parsed 0 pedit actions, can't do more"). This happens when
> > offloading pedit actions on mlx(cx4) VFs. The command is shown as below [2].
> >
> > For example: (p2p1_0 is VF net device)
> > [1]
> > $ tc filter add dev p2p1_0 protocol ip parent ffff: prio 1 flower skip_sw \
> > src_mac e4:11:22:33:44:01 \
> > action tunnel_key set \
> > src_ip 1.1.1.100 \
> > dst_ip 1.1.1.200 \
> > dst_port 4789 id 100 \
> > action mirred egress redirect dev vxlan0
> >
> > [2]
> > $ tc filter add dev p2p1_0 parent ffff: protocol ip prio 1 \
> > flower skip_sw dst_mac 00:10:56:fb:64:e8 \
> > dst_ip 1.1.1.100 src_ip 1.1.1.200 \
> > action pedit ex munge eth src set 00:10:56:b4:5d:20
> >
> > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> > ---
> > drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 19 ++++++++++++++++---
> > 1 file changed, 16 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> > index 56ac50d..3a02b22 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> > @@ -2035,7 +2035,7 @@ static int parse_tc_pedit_action(struct mlx5e_priv *priv,
> > struct netlink_ext_ack *extack)
> > {
> > u8 cmd = (act->id == FLOW_ACTION_MANGLE) ? 0 : 1;
> > - int err = -EOPNOTSUPP;
> > + int max_actions, err = -EOPNOTSUPP;
> > u32 mask, val, offset;
> > u8 htype;
> >
> > @@ -2047,6 +2047,17 @@ static int parse_tc_pedit_action(struct mlx5e_priv *priv,
> > goto out_err;
> > }
> >
> > + if (namespace == MLX5_FLOW_NAMESPACE_FDB) /* FDB offloading */
> > + max_actions = MLX5_CAP_ESW_FLOWTABLE_FDB(priv->mdev, max_modify_header_actions);
> > + else /* namespace is MLX5_FLOW_NAMESPACE_KERNEL - NIC offloading */
> > + max_actions = MLX5_CAP_FLOWTABLE_NIC_RX(priv->mdev, max_modify_header_actions);
> > +
>
> we have the exact same block with comments in alloc_mod_hdr_actions() which
> is called later after we parse the action and now parsing the pedit fields.
> your check for max_actions was there in prev v. why move it?
I think we should check it as soon as possible.
> if like this I suggest we create a small static function to get max actions
> for a namespace and call it from both places to get max actions.
it's ok.
> > + if (!max_actions) {
> > + NL_SET_ERR_MSG_MOD(extack,
> > + "don't support pedit actions, can't offload");
>
> please rephrase the msg to be consistent
> i.e. The pedit offload action is not supported
>
> > + goto out_err;
> > + }
> > +
> > mask = act->mangle.mask;
> > val = act->mangle.val;
> > offset = act->mangle.offset;
> > @@ -2294,7 +2305,8 @@ static int parse_tc_nic_actions(struct mlx5e_priv *priv,
> > }
> > break;
> > default:
> > - return -EINVAL;
> > + NL_SET_ERR_MSG_MOD(extack, "The offload action is not supported");
> > + return -EOPNOTSUPP;
> > }
> > }
> >
> > @@ -2616,7 +2628,8 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
> > break;
> > }
> > default:
> > - return -EINVAL;
> > + NL_SET_ERR_MSG_MOD(extack, "The offload action is not supported");
> > + return -EOPNOTSUPP;
> > }
> > }
> >
> >
^ permalink raw reply
* [PATCH net-next 0/2]cxgb4/cxgb4vf: VF link state support
From: Arjun Vynipadath @ 2019-02-28 10:46 UTC (permalink / raw)
To: netdev, davem; +Cc: nirranjan, indranil, dt, Arjun Vynipadath
This series of patches adds support for ndo_set_vf_link_state in
cxgb4 driver.
Patch 1 implements ndo_set_vf_link_state
Patch 2 reverts the existing force_link_up behaviour for cxgb4vf driver.
Arjun Vynipadath (2):
cxgb4: Add VF Link state support
cxgb4vf: Revert force link up behaviour
drivers/net/ethernet/chelsio/cxgb4/cxgb4.h | 1 +
drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c | 57 +++++++++++++++++++---
drivers/net/ethernet/chelsio/cxgb4/t4fw_api.h | 10 ++++
.../net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.c | 13 ++---
4 files changed, 65 insertions(+), 16 deletions(-)
--
2.9.5
^ permalink raw reply
* [PATCH net-next 1/2] cxgb4: Add VF Link state support
From: Arjun Vynipadath @ 2019-02-28 10:46 UTC (permalink / raw)
To: netdev, davem; +Cc: nirranjan, indranil, dt, Arjun Vynipadath, Vishal Kulkarni
In-Reply-To: <20190228104627.10830-1-arjun@chelsio.com>
Use ndo_set_vf_link_state to control the link states associated
with the virtual interfaces.
Signed-off-by: Arjun Vynipadath <arjun@chelsio.com>
Signed-off-by: Vishal Kulkarni <vishal@chelsio.com>
---
drivers/net/ethernet/chelsio/cxgb4/cxgb4.h | 1 +
drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c | 57 ++++++++++++++++++++++---
drivers/net/ethernet/chelsio/cxgb4/t4fw_api.h | 8 ++++
3 files changed, 60 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h b/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h
index b7b0eb104430..dd6379809c74 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h
@@ -885,6 +885,7 @@ struct vf_info {
unsigned int tx_rate;
bool pf_set_mac;
u16 vlan;
+ int link_state;
};
enum {
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
index bcbac247a73d..4eda5285c867 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
@@ -2740,6 +2740,7 @@ static int cxgb4_mgmt_get_vf_config(struct net_device *dev,
ivi->min_tx_rate = 0;
ether_addr_copy(ivi->mac, vfinfo->vf_mac_addr);
ivi->vlan = vfinfo->vlan;
+ ivi->linkstate = vfinfo->link_state;
return 0;
}
@@ -2879,6 +2880,49 @@ static int cxgb4_mgmt_set_vf_vlan(struct net_device *dev, int vf,
ret, (vlan ? "setting" : "clearing"), adap->pf, vf);
return ret;
}
+
+static int cxgb4_mgmt_set_vf_link_state(struct net_device *dev, int vf,
+ int link)
+{
+ struct port_info *pi = netdev_priv(dev);
+ struct adapter *adap = pi->adapter;
+ int ret = 0;
+ u32 param, val;
+
+ if (vf >= adap->num_vfs)
+ return -EINVAL;
+
+ switch (link) {
+ case IFLA_VF_LINK_STATE_AUTO:
+ val = VF_LINK_STATE_AUTO;
+ break;
+
+ case IFLA_VF_LINK_STATE_ENABLE:
+ val = VF_LINK_STATE_ENABLE;
+ break;
+
+ case IFLA_VF_LINK_STATE_DISABLE:
+ val = VF_LINK_STATE_DISABLE;
+ break;
+
+ default:
+ return -EINVAL;
+ }
+
+ param = (FW_PARAMS_MNEM_V(FW_PARAMS_MNEM_PFVF) |
+ FW_PARAMS_PARAM_X_V(FW_PARAMS_PARAM_PFVF_LINK_STATE));
+ ret = t4_set_params(adap, adap->mbox, adap->pf, vf + 1, 1,
+ ¶m, &val);
+ if (ret) {
+ dev_err(adap->pdev_dev,
+ "Error %d in setting PF %d VF %d link state\n",
+ ret, adap->pf, vf);
+ return -EINVAL;
+ }
+
+ adap->vfinfo[vf].link_state = link;
+ return ret;
+}
#endif /* CONFIG_PCI_IOV */
static int cxgb_set_mac_addr(struct net_device *dev, void *p)
@@ -3294,12 +3338,13 @@ static const struct net_device_ops cxgb4_netdev_ops = {
#ifdef CONFIG_PCI_IOV
static const struct net_device_ops cxgb4_mgmt_netdev_ops = {
- .ndo_open = cxgb4_mgmt_open,
- .ndo_set_vf_mac = cxgb4_mgmt_set_vf_mac,
- .ndo_get_vf_config = cxgb4_mgmt_get_vf_config,
- .ndo_set_vf_rate = cxgb4_mgmt_set_vf_rate,
- .ndo_get_phys_port_id = cxgb4_mgmt_get_phys_port_id,
- .ndo_set_vf_vlan = cxgb4_mgmt_set_vf_vlan,
+ .ndo_open = cxgb4_mgmt_open,
+ .ndo_set_vf_mac = cxgb4_mgmt_set_vf_mac,
+ .ndo_get_vf_config = cxgb4_mgmt_get_vf_config,
+ .ndo_set_vf_rate = cxgb4_mgmt_set_vf_rate,
+ .ndo_get_phys_port_id = cxgb4_mgmt_get_phys_port_id,
+ .ndo_set_vf_vlan = cxgb4_mgmt_set_vf_vlan,
+ .ndo_set_vf_link_state = cxgb4_mgmt_set_vf_link_state,
};
#endif
diff --git a/drivers/net/ethernet/chelsio/cxgb4/t4fw_api.h b/drivers/net/ethernet/chelsio/cxgb4/t4fw_api.h
index 631f1663f4e0..9a3bab797760 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/t4fw_api.h
+++ b/drivers/net/ethernet/chelsio/cxgb4/t4fw_api.h
@@ -1312,6 +1312,14 @@ enum fw_params_param_pfvf {
FW_PARAMS_PARAM_PFVF_RAWF_END = 0x37,
FW_PARAMS_PARAM_PFVF_NCRYPTO_LOOKASIDE = 0x39,
FW_PARAMS_PARAM_PFVF_PORT_CAPS32 = 0x3A,
+ FW_PARAMS_PARAM_PFVF_LINK_STATE = 0x40,
+};
+
+/* Virtual link state as seen by the specified VF */
+enum vf_link_states {
+ VF_LINK_STATE_AUTO = 0x00,
+ VF_LINK_STATE_ENABLE = 0x01,
+ VF_LINK_STATE_DISABLE = 0x02,
};
/*
--
2.9.5
^ permalink raw reply related
* [PATCH net-next 2/2] cxgb4vf: Revert force link up behaviour
From: Arjun Vynipadath @ 2019-02-28 10:46 UTC (permalink / raw)
To: netdev, davem; +Cc: nirranjan, indranil, dt, Arjun Vynipadath, Vishal Kulkarni
In-Reply-To: <20190228104627.10830-1-arjun@chelsio.com>
Reverting force link up changes since this behaviour can be
achieved using VF link state feature.
Reverts:
commit 0913667ab3ad ("cxgb4vf: Forcefully link up virtual interfaces")
Signed-off-by: Arjun Vynipadath <arjun@chelsio.com>
Signed-off-by: Vishal Kulkarni <vishal@chelsio.com>
---
drivers/net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.c | 13 +++----------
1 file changed, 3 insertions(+), 10 deletions(-)
diff --git a/drivers/net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.c b/drivers/net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.c
index 3300b69a42b3..ed888227d81d 100644
--- a/drivers/net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.c
+++ b/drivers/net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.c
@@ -155,6 +155,8 @@ void t4vf_os_link_changed(struct adapter *adapter, int pidx, int link_ok)
const char *fc;
const struct port_info *pi = netdev_priv(dev);
+ netif_carrier_on(dev);
+
switch (pi->link_cfg.speed) {
case 100:
s = "100Mbps";
@@ -200,6 +202,7 @@ void t4vf_os_link_changed(struct adapter *adapter, int pidx, int link_ok)
netdev_info(dev, "link up, %s, full-duplex, %s PAUSE\n", s, fc);
} else {
+ netif_carrier_off(dev);
netdev_info(dev, "link down\n");
}
}
@@ -339,16 +342,6 @@ static int link_start(struct net_device *dev)
if (ret == 0)
ret = t4vf_enable_pi(pi->adapter, pi, true, true);
- /* The Virtual Interfaces are connected to an internal switch on the
- * chip which allows VIs attached to the same port to talk to each
- * other even when the port link is down. As a result, we generally
- * want to always report a VI's link as being "up", provided there are
- * no errors in enabling vi.
- */
-
- if (ret == 0)
- netif_carrier_on(dev);
-
return ret;
}
--
2.9.5
^ permalink raw reply related
* [PATCH v2] net: netem: fix skb length BUG_ON in __skb_to_sgvec
From: Sheng Lan @ 2019-02-28 10:47 UTC (permalink / raw)
To: eric.dumazet, stephen, davem
Cc: netdev, netem, xuhanbing, zhengshaoyu, jiqin.ji, liuzhiqiang26,
yuehaibing
From: Sheng Lan <lansheng@huawei.com>
It can be reproduced by following steps:
1. virtio_net NIC is configured with gso/tso on
2. configure nginx as http server with an index file bigger than 1M bytes
3. use tc netem to produce duplicate packets and delay:
tc qdisc add dev eth0 root netem delay 100ms 10ms 30% duplicate 90%
4. continually curl the nginx http server to get index file on client
5. BUG_ON is seen quickly
[10258690.371129] kernel BUG at net/core/skbuff.c:4028!
[10258690.371748] invalid opcode: 0000 [#1] SMP PTI
[10258690.372094] CPU: 5 PID: 0 Comm: swapper/5 Tainted: G W 5.0.0-rc6 #2
[10258690.372094] RSP: 0018:ffffa05797b43da0 EFLAGS: 00010202
[10258690.372094] RBP: 00000000000005ea R08: 0000000000000000 R09: 00000000000005ea
[10258690.372094] R10: ffffa0579334d800 R11: 00000000000002c0 R12: 0000000000000002
[10258690.372094] R13: 0000000000000000 R14: ffffa05793122900 R15: ffffa0578f7cb028
[10258690.372094] FS: 0000000000000000(0000) GS:ffffa05797b40000(0000) knlGS:0000000000000000
[10258690.372094] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[10258690.372094] CR2: 00007f1a6dc00868 CR3: 000000001000e000 CR4: 00000000000006e0
[10258690.372094] Call Trace:
[10258690.372094] <IRQ>
[10258690.372094] skb_to_sgvec+0x11/0x40
[10258690.372094] start_xmit+0x38c/0x520 [virtio_net]
[10258690.372094] dev_hard_start_xmit+0x9b/0x200
[10258690.372094] sch_direct_xmit+0xff/0x260
[10258690.372094] __qdisc_run+0x15e/0x4e0
[10258690.372094] net_tx_action+0x137/0x210
[10258690.372094] __do_softirq+0xd6/0x2a9
[10258690.372094] irq_exit+0xde/0xf0
[10258690.372094] smp_apic_timer_interrupt+0x74/0x140
[10258690.372094] apic_timer_interrupt+0xf/0x20
[10258690.372094] </IRQ>
In __skb_to_sgvec(), the skb->len is not equal to the sum of the skb's
linear data size and nonlinear data size, thus BUG_ON triggered.
Because the skb is cloned and a part of nonlinear data is split off.
Duplicate packet is cloned in netem_enqueue() and may be delayed
some time in qdisc. When qdisc len reached the limit and returns
NET_XMIT_DROP, the skb will be retransmit later in write queue.
the skb will be fragmented by tso_fragment(), the limit size
that depends on cwnd and mss decrease, the skb's nonlinear
data will be split off. The length of the skb cloned by netem
will not be updated. When we use virtio_net NIC and invoke skb_to_sgvec(),
the BUG_ON trigger.
To fix it, netem returns NET_XMIT_SUCCESS to upper stack
when it clones a duplicate packet.
Fixes: 35d889d1 ("sch_netem: fix skb leak in netem_enqueue()")
Signed-off-by: Sheng Lan <lansheng@huawei.com>
Reported-by: Qin Ji <jiqin.ji@huawei.com>
Suggested-by: Eric Dumazet <eric.dumazet@gmail.com>
---
Changes v1 to v2:
- Fix it by returning a proper value rather than copy one skb
net/sched/sch_netem.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index 75046ec..cc9d813 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -447,6 +447,7 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch,
int nb = 0;
int count = 1;
int rc = NET_XMIT_SUCCESS;
+ int rc_drop = NET_XMIT_DROP;
/* Do not fool qdisc_drop_all() */
skb->prev = NULL;
@@ -486,6 +487,7 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch,
q->duplicate = 0;
rootq->enqueue(skb2, rootq, to_free);
q->duplicate = dupsave;
+ rc_drop = NET_XMIT_SUCCESS;
}
/*
@@ -498,7 +500,7 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch,
if (skb_is_gso(skb)) {
segs = netem_segment(skb, sch, to_free);
if (!segs)
- return NET_XMIT_DROP;
+ return rc_drop;
} else {
segs = skb;
}
@@ -521,8 +523,10 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch,
1<<(prandom_u32() % 8);
}
- if (unlikely(sch->q.qlen >= sch->limit))
- return qdisc_drop_all(skb, sch, to_free);
+ if (unlikely(sch->q.qlen >= sch->limit)) {
+ qdisc_drop_all(skb, sch, to_free);
+ return rc_drop;
+ }
qdisc_qstats_backlog_inc(sch, skb);
--
^ permalink raw reply related
* RE: AF_XDP design flaws
From: Maxim Mikityanskiy @ 2019-02-28 10:49 UTC (permalink / raw)
To: Jonathan Lemon
Cc: Björn Töpel, John Fastabend, netdev@vger.kernel.org,
Björn Töpel, Magnus Karlsson, David S. Miller,
Tariq Toukan, Saeed Mahameed, Eran Ben Elisha
In-Reply-To: <D7835D58-B008-415F-B653-AAA04C84DE21@gmail.com>
> -----Original Message-----
> From: Jonathan Lemon <jonathan.lemon@gmail.com>
> Sent: 27 February, 2019 23:03
> To: Maxim Mikityanskiy <maximmi@mellanox.com>
> Cc: Björn Töpel <bjorn.topel@gmail.com>; John Fastabend
> <john.fastabend@gmail.com>; netdev@vger.kernel.org; Björn Töpel
> <bjorn.topel@intel.com>; Magnus Karlsson <magnus.karlsson@intel.com>; David
> S. Miller <davem@davemloft.net>; Tariq Toukan <tariqt@mellanox.com>; Saeed
> Mahameed <saeedm@mellanox.com>; Eran Ben Elisha <eranbe@mellanox.com>
> Subject: Re: AF_XDP design flaws
>
> On 27 Feb 2019, at 11:17, Maxim Mikityanskiy wrote:
>
> >> -----Original Message-----
> >> From: Björn Töpel <bjorn.topel@gmail.com>
> >> Sent: 27 February, 2019 10:09
> >> To: John Fastabend <john.fastabend@gmail.com>
> >> Cc: Maxim Mikityanskiy <maximmi@mellanox.com>;
> >> netdev@vger.kernel.org; Björn
> >> Töpel <bjorn.topel@intel.com>; Magnus Karlsson
> >> <magnus.karlsson@intel.com>;
> >> David S. Miller <davem@davemloft.net>; Tariq Toukan
> >> <tariqt@mellanox.com>;
> >> Saeed Mahameed <saeedm@mellanox.com>; Eran Ben Elisha
> >> <eranbe@mellanox.com>
> >> Subject: Re: AF_XDP design flaws
> >>
> >> On 2019-02-26 17:41, John Fastabend wrote:
> >>> On 2/26/19 6:49 AM, Maxim Mikityanskiy wrote:
> >>>> Hi everyone,
> >>>>
> >>
> >> Hi! It's exciting to see more vendors looking into AF_XDP. ARM was
> >> involved early on, and now Mellanox. :-) It's really important that
> >> the AF_XDP model is a good fit for all drivers/vendors! Thanks for
> >> looking into this.
> >>
> >>>> I would like to discuss some design flaws of AF_XDP socket (XSK)
> >> implementation
> >>>> in kernel. At the moment I don't see a way to work around them
> >>>> without
> >> changing
> >>>> the API, so I would like to make sure that I'm not missing anything
> >>>> and
> >> to
> >>>> suggest and discuss some possible improvements that can be made.
> >>>>
> >>>> The issues I describe below are caused by the fact that the driver
> >> depends on
> >>>> the application doing some things, and if the application is
> >>>> slow/buggy/malicious, the driver is forced to busy poll because of
> >>>> the
> >> lack of a
> >>>> notification mechanism from the application side. I will refer to
> >>>> the
> >> i40e
> >>>> driver implementation a lot, as it is the first implementation of
> >>>> AF_XDP,
> >> but
> >>>> the issues are general and affect any driver. I already considered
> >>>> trying
> >> to fix
> >>>> it on driver level, but it doesn't seem possible, so it looks like
> >>>> the
> >> behavior
> >>>> and implementation of AF_XDP in the kernel has to be changed.
> >>>>
> >>>> RX side busy polling
> >>>> ====================
> >>>>
> >>>> On the RX side, the driver expects the application to put some
> >> descriptors in
> >>>> the Fill Ring. There is no way for the application to notify the
> >>>> driver
> >> that
> >>>> there are more Fill Ring descriptors to take, so the driver is
> >>>> forced to
> >> busy
> >>>> poll the Fill Ring if it gets empty. E.g., the i40e driver does it
> >>>> in
> >> NAPI poll:
> >>>>
> >>>> int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget)
> >>>> {
> >>>> ...
> >>>> failure = failure ||
> >>>>
> >> !i40e_alloc_rx_buffers_fast_zc(rx_ring,
> >>>>
> >> cleaned_count);
> >>>> ...
> >>>> return failure ? budget : (int)total_rx_packets;
> >>>> }
> >>>>
> >>>> Basically, it means that if there are no descriptors in the Fill
> >>>> Ring,
> >> NAPI will
> >>>> never stop, draining CPU.
> >>>>
> >>>> Possible cases when it happens
> >>>> ------------------------------
> >>>>
> >>>> 1. The application is slow, it received some frames in the RX Ring,
> >>>> and
> >> it is
> >>>> still handling the data, so it has no free frames to put to the
> >>>> Fill
> >> Ring.
> >>>>
> >>>> 2. The application is malicious, it opens an XSK and puts no frames
> >>>> to
> >> the Fill
> >>>> Ring. It can be used as a local DoS attack.
> >>>>
> >>>> 3. The application is buggy and stops filling the Fill Ring for
> >>>> whatever
> >> reason
> >>>> (deadlock, waiting for another blocking operation, other bugs).
> >>>>
> >>>> Although loading an XDP program requires root access, the DoS
> >>>> attack can
> >> be
> >>>> targeted to setups that already use XDP, i.e. an XDP program is
> >>>> already
> >> loaded.
> >>>> Even under root, userspace applications should not be able to
> >>>> disrupt
> >> system
> >>>> stability by just calling normal APIs without an intention to
> >>>> destroy the
> >>>> system, and here it happens in case 1.
> >>> I believe this is by design. If packets per second is your top
> >>> priority
> >>> (at the expense of power, cpu, etc.) this is the behavior you might
> >>> want. To resolve your points if you don't trust the application it
> >>> should be isolated to a queue pair and cores so the impact is known
> >>> and
> >>> managed.
> >>>
> >>
> >> Correct, and I agree with John here. AF_XDP is a raw interface, and
> >> needs to be managed.
> >
> > OK, right, if you want high pps sacrificing system stability and
> > kernel-userspace isolation, XSK is at your service. It may be a valid
> > point. However, there is still the issue of unintended use.
>
> I believe this is a double-edged sword - on one hand, it appears AF_XDP
> is
> aimed as a competitor to DPDK, and is focused only on raw packet speed,
> at
> the expense of usability. This isn't necessarily bad, but in my
> experience,
> if things aren't simple to use, then they tend not to get widely used.
You are right, but not getting widespread also leads to the lack of
usage examples, which in turn can lead to API misuse. Another point is
that in the past there were security issues found in the code that is
rarely used but turned on in binary kernels, and AF_XDP is such kind of
code - malware can make use of its flaws even if it's not used
legitimately on a given system.
> > If you use XSK, you can take measures to prevent the disruption, e.g.
> > run only trusted and properly tested applications or isolate them.
> > However, there is a case where XSKs are not really used on a given
> > machine, but are still available (CONFIG_XDP_SOCKETS=y), opening a
> > huge
> > hole. If you don't even know that XSK exists, you are still
> > vulnerable.
> >
> >>> That said having a flag to back-off seems like a good idea. But
> >>> should
> >>> be doable as a feature without breaking current API.
> >
> > A flag can be added, e.g., to the sxdp_flags field of struct
> > sockaddr_xdp, to switch to the behavior without busy polling on empty
> > Fill Ring.
> >
> > However, I don't think the vulnerability can be eliminated while
> > keeping
> > the API intact. To protect the systems that don't use XSK, we need one
> > of these things:
> >
> > - Disable XSK support by default, until explicitly enabled by root.
> >
> > - Disable the old behavior by default, until explicitly enabled by
> > root.
> >
> > Both of those things will require additional setup on machines that
> > use
> > XSK, after upgrading the kernel.
> >
> >>>> Possible way to solve the issue
> >>>> -------------------------------
> >>>>
> >>>> When the driver can't take new Fill Ring frames, it shouldn't busy
> >>>> poll.
> >>>> Instead, it signals the failure to the application (e.g., with
> >>>> POLLERR),
> >> and
> >>>> after that it's up to the application to restart polling (e.g., by
> >> calling
> >>>> sendto()) after refilling the Fill Ring. The issue with this
> >>>> approach is
> >> that it
> >>>> changes the API, so we either have to deal with it or to introduce
> >>>> some
> >> API
> >>>> version field.
> >>> See above. I like the idea here though.
> >>>
> >>
> >> The uapi doesn't mandate *how* the driver should behave. The rational
> >> for the aggressive i40e fill ring polling (when the ring is empty) is
> >> to minimize the latency. The "fill ring is empty" behavior might be
> >> different on different drivers.
> >
> > If the behavior is different with different drivers, it will be
> > difficult to create applications that are portable across NICs. The
> > point of creating APIs is to provide a single interface to different
> > implementations, thus hiding the differences from a higher abstraction
> > level. What the driver does internally is up to the driver, but the
> > result visible in the userspace should be the same, so we can't just
> > make some drivers stop and wait when the Fill Ring is empty, while the
> > others busy poll. It should be controlled by some flags if we want to
> > preserve both options.
>
> Or if the driver can't obtain new frames from the fill ring, it just
> drops the incoming packet, and recycles the old frame in order to keep
> the ring full.
Good idea, thanks! I will need to analyze it though.
> (This also assumes that the driver was able to initially
> populate the ring in the first place,
Which will not happen if there is nothing in the Fill Ring initially...
> which eliminates one DoS attack of
> never putting anything in the fill ring initially).
>
> I do agree that issues cited in this thread can be resolved with the
> use of flags to control the different behaviors - there just needs to
> be some agreement on what those behaviors are.
> --
> Jonathan
^ permalink raw reply
* Re: [PATCH] xen-netback: fix occasional leak of grant ref mappings under memory pressure
From: Wei Liu @ 2019-02-28 11:01 UTC (permalink / raw)
To: Paul Durrant
Cc: Igor Druzhinin, xen-devel@lists.xenproject.org,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org, Wei Liu,
davem@davemloft.net
In-Reply-To: <8424fba34a1f43908a6dae3a9308863b@AMSPEX02CL02.citrite.net>
On Thu, Feb 28, 2019 at 09:46:57AM +0000, Paul Durrant wrote:
> > -----Original Message-----
> > From: Igor Druzhinin [mailto:igor.druzhinin@citrix.com]
> > Sent: 28 February 2019 02:03
> > To: xen-devel@lists.xenproject.org; netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> > Cc: Wei Liu <wei.liu2@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>; davem@davemloft.net; Igor
> > Druzhinin <igor.druzhinin@citrix.com>
> > Subject: [PATCH] xen-netback: fix occasional leak of grant ref mappings under memory pressure
> >
> > Zero-copy callback flag is not yet set on frag list skb at the moment
> > xenvif_handle_frag_list() returns -ENOMEM. This eventually results in
> > leaking grant ref mappings since xenvif_zerocopy_callback() is never
> > called for these fragments. Those eventually build up and cause Xen
> > to kill Dom0 as the slots get reused for new mappings.
> >
> > That behavior is observed under certain workloads where sudden spikes
> > of page cache usage for writes coexist with active atomic skb allocations.
> >
> > Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
> > ---
> > drivers/net/xen-netback/netback.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
> > index 80aae3a..2023317 100644
> > --- a/drivers/net/xen-netback/netback.c
> > +++ b/drivers/net/xen-netback/netback.c
> > @@ -1146,9 +1146,12 @@ static int xenvif_tx_submit(struct xenvif_queue *queue)
> >
> > if (unlikely(skb_has_frag_list(skb))) {
> > if (xenvif_handle_frag_list(queue, skb)) {
> > + struct sk_buff *nskb =
> > + skb_shinfo(skb)->frag_list;
> > if (net_ratelimit())
> > netdev_err(queue->vif->dev,
> > "Not enough memory to consolidate frag_list!\n");
> > + xenvif_skb_zerocopy_prepare(queue, nskb);
> > xenvif_skb_zerocopy_prepare(queue, skb);
> > kfree_skb(skb);
> > continue;
>
> Whilst this fix will do the job, I think it would be better to get rid of the kfree_skb() from inside xenvif_handle_frag_list() and always deal with it here rather than having it happen in two different places. Something like the following...
+1 for having only one place.
>
> ---8<---
> diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
> index 80aae3a32c2a..093c7b860772 100644
> --- a/drivers/net/xen-netback/netback.c
> +++ b/drivers/net/xen-netback/netback.c
> @@ -1027,13 +1027,13 @@ static void xenvif_tx_build_gops(struct xenvif_queue *queue,
> /* Consolidate skb with a frag_list into a brand new one with local pages on
> * frags. Returns 0 or -ENOMEM if can't allocate new pages.
> */
> -static int xenvif_handle_frag_list(struct xenvif_queue *queue, struct sk_buff *skb)
> +static int xenvif_handle_frag_list(struct xenvif_queue *queue, struct sk_buff *diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
> index 80aae3a32c2a..093c7b860772 100644
> --- a/drivers/net/xen-netback/netback.c
> +++ b/drivers/net/xen-netback/netback.c
> @@ -1027,13 +1027,13 @@ static void xenvif_tx_build_gops(struct xenvif_queue *qu
> eue,
> /* Consolidate skb with a frag_list into a brand new one with local pages on
> * frags. Returns 0 or -ENOMEM if can't allocate new pages.
> */
> -static int xenvif_handle_frag_list(struct xenvif_queue *queue, struct sk_buff *
> skb)
> +static int xenvif_handle_frag_list(struct xenvif_queue *queue, struct sk_buff *
> skb,
> + struct sk_buff *nskb)
> {
> unsigned int offset = skb_headlen(skb);
> skb_frag_t frags[MAX_SKB_FRAGS];
> int i, f;
> struct ubuf_info *uarg;
> - struct sk_buff *nskb = skb_shinfo(skb)->frag_list;
>
> queue->stats.tx_zerocopy_sent += 2;
> queue->stats.tx_frag_overflow++;
> @@ -1072,11 +1072,6 @@ static int xenvif_handle_frag_list(struct xenvif_queue *q
> ueue, struct sk_buff *s
> skb_frag_size_set(&frags[i], len);
> }
>
> - /* Copied all the bits from the frag list -- free it. */
> - skb_frag_list_init(skb);
> - xenvif_skb_zerocopy_prepare(queue, nskb);
> - kfree_skb(nskb);
> -
> /* Release all the original (foreign) frags. */
> for (f = 0; f < skb_shinfo(skb)->nr_frags; f++)
> skb_frag_unref(skb, f);
> @@ -1145,7 +1140,11 @@ static int xenvif_tx_submit(struct xenvif_queue *queue)
> xenvif_fill_frags(queue, skb);
>
> if (unlikely(skb_has_frag_list(skb))) {
> - if (xenvif_handle_frag_list(queue, skb)) {
> + struct sk_buff *nskb = skb_shinfo(skb)->frag_list;
> +
> + xenvif_skb_zerocopy_prepare(queue, nskb);
> +
> + if (xenvif_handle_frag_list(queue, skb, nskb)) {
> if (net_ratelimit())
> netdev_err(queue->vif->dev,
> "Not enough memory to consolidate frag_list!\n");
> @@ -1153,6 +1152,10 @@ static int xenvif_tx_submit(struct xenvif_queue *queue)
> kfree_skb(skb);
> continue;
> }
> +
> + /* Copied all the bits from the frag list. */
> + skb_frag_list_init(skb);
> + kfree(nskb);
I think you want kfree_skb here?
Wei.
> }
>
> skb->dev = queue->vif->dev;
> ---8<---
>
> What do you think?
>
> Paul
>
> > --
> > 2.7.4
>
^ permalink raw reply
* Re: [PATCH 0/4] mwifiex PCI/wake-up interrupt fixes
From: Rafael J. Wysocki @ 2019-02-28 11:03 UTC (permalink / raw)
To: Brian Norris
Cc: Rafael J. Wysocki, Ard Biesheuvel, Marc Zyngier, Ganapathi Bhat,
Jeffy Chen, Heiko Stuebner, Devicetree List, Xinming Hu,
<netdev@vger.kernel.org>, linux-pm,
<linux-wireless@vger.kernel.org>, Linux Kernel Mailing List,
Amitkumar Karwar, open list:ARM/Rockchip SoC...,
Nishant Sarmukadam, Rob Herring, Rafael J. Wysocki,
linux-arm-kernel, Enric Balletbo i Serra, Lorenzo Pieralisi,
David S. Miller, Kalle Valo, Tony Lindgren, Mark Rutland
In-Reply-To: <CA+ASDXNT9Taqfoz7R1Z926Vi1fMNNLYg5vqcZTw2m6JvXk+75w@mail.gmail.com>
On Thu, Feb 28, 2019 at 3:29 AM Brian Norris <briannorris@chromium.org> wrote:
>
> Hi Rafael,
>
> On Wed, Feb 27, 2019 at 3:04 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > On Wed, Feb 27, 2019 at 9:58 PM Brian Norris <briannorris@chromium.org> wrote:
> > > On Wed, Feb 27, 2019 at 11:16:12AM +0100, Ard Biesheuvel wrote:
> > > > So I'd argue that we should add an optional 'wake-gpio' DT property
> > > > instead to the generic PCI device binding, and leave the interrupt
> > > > binding and discovery alone.
> > >
> > > So I think Mark Rutland already shot that one down; it's conceptually an
> > > interrupt from the device's perspective.
>
> Perhaps I shouldn't speak for Mark, but I am basically quoting him off IRC.
>
> > Which device are you talking about? The one that signals wakeup? If
> > so, then I beg to differ.
>
> Yes, the endpoint device.
>
> > On ACPI platforms WAKE# is represented as an ACPI GPE that is signaled
> > through SCI and handled at a different level (on HW-reduced ACPI it
> > actually can be a GPIO interrupt, but it still is handled with the
> > help of AML). The driver of the device signaling wakeup need not even
> > be aware that WAKE# has been asserted.
>
> Frankly, ACPI is not relevant to how we represent WAKE# in DT, IMO.
I mentioned ACPI as an example of how WAKE# can be handled.
> Also, we're talking about the *device*, not the driver. When talking
> about Device Tree, that distinction is relevant.
I'm not sure what you mean, really.
I guess we are talking about information in DT and some of it is
consumed by the driver while some of it is consumed by some other
pieces of code.
> So while the driver need not be aware (and I agree! it only needs to
> care about enabling/disabling wake),
Not even that. Actually, PME enable is handled by the PCI bus type code.
> *something* should be aware,
Right.
> and the signal that "something" should be receiving is simply "did WAKE
> happen"?
Right. But it may not be clear which device signaled it, because
WAKE# may be shared in general.
> That sounds basically like the device is signalling an interrupt to me.
Well, consider the native PME here. In there, the device sends an
in-band message over the PCIe hierarchy to a root port and the
interrupt is signaled from there. There is a PME driver in the kernel
that requests that IRQ and handles it (for all ports that can trigger
it), but for each port it binds to a separate device (or "service" if
you will) that takes care of all PME messages coming from all devices
under that port.
At an abstract level, WAKE# is expected to be similar AFAICS except
that there is a physical signal going from the device(s) (in low-power
states) that have detected external activity (wakeup) to an entity
that can trigger an interrupt.
The original idea in the PCI PM spec regarding WAKE# seems to be to
wire up WAKE# from all devices on the bus to one input that will cause
an interrupt to trigger when one of them drives WAKE# and whatever
handled that interrupt was expected to walk the bus, find the
device(s) with PME Status set and put it (or them) into D0 (clearing
PME Status in the process). That still is a valid case to consider
IMO.
However, designers of some boards decided to provide dedicated
per-device interrupts for WAKE# and you may regard them as "wakeup
IRQs" except that the handling of each of them is the same as for the
"global WAKE#" above: if the PME Status of the device is set, put it
into D0 etc. That part belongs to the PCI bus type layer IMO.
Of course, because nothing is easy and simple, there is a third case
in which one WAKE# line (and interrupt) can be shared by a subset of
devices on the bus and there are multiple (say two or three) subsets.
> Maybe this goes back to some confusion we had elsewhere: what is the
> meaning of "interrupt" in device tree?
Maybe.
> > > We just need to figure out a good way of representing it that doesn't stomp on the existing INTx
> > > definitions.
> >
> > WAKE# is a signal that is converted into an interrupt, but that
> > interrupt may arrive at some place your driver has nothing to do with.
>
> I could agree with that, perhaps. But that's also what Device Tree is
> all about, really. We describe the relation between devices. So some
> other <foo> handles events that are triggered by <bar>, so we use a
> phandle to relate <bar> to <foo>.
So you have a PCI endpoint on the one hand and the "wakeup serivce"
device on the other.
> > It generally doesn't make sense to represent it as an interrupt for
> > the target device.
>
> What would you suggest then? I'm not clearly understanding how you
> think we should (a) describe (in DT) and (b) implement this WAKE#
> handling.
I would introduce a "wakeup service" concept (along the lines of the
native PME) and make it request all interrupts associated with WAKE#.
In the case when the WAKE# interrupts are dedicated per-device, it
would be good to avoid walking the bus and use some
"wakeup-IRQ-to-device" mapping if available.
But as I said above, IMO the *handling* of all WAKE# interrupts
belongs to the PCI bus type as it is the same for all PCI devices.
^ permalink raw reply
* RE: [PATCH] xen-netback: fix occasional leak of grant ref mappings under memory pressure
From: Paul Durrant @ 2019-02-28 11:21 UTC (permalink / raw)
To: Wei Liu
Cc: Igor Druzhinin, xen-devel@lists.xenproject.org,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org, Wei Liu,
davem@davemloft.net
In-Reply-To: <20190228110136.somjads2f5ivqhju@zion.uk.xensource.com>
> -----Original Message-----
> From: Wei Liu [mailto:wei.liu2@citrix.com]
> Sent: 28 February 2019 11:02
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Igor Druzhinin <igor.druzhinin@citrix.com>; xen-devel@lists.xenproject.org;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Wei Liu <wei.liu2@citrix.com>;
> davem@davemloft.net
> Subject: Re: [PATCH] xen-netback: fix occasional leak of grant ref mappings under memory pressure
>
> On Thu, Feb 28, 2019 at 09:46:57AM +0000, Paul Durrant wrote:
> > > -----Original Message-----
> > > From: Igor Druzhinin [mailto:igor.druzhinin@citrix.com]
> > > Sent: 28 February 2019 02:03
> > > To: xen-devel@lists.xenproject.org; netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> > > Cc: Wei Liu <wei.liu2@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>; davem@davemloft.net;
> Igor
> > > Druzhinin <igor.druzhinin@citrix.com>
> > > Subject: [PATCH] xen-netback: fix occasional leak of grant ref mappings under memory pressure
> > >
> > > Zero-copy callback flag is not yet set on frag list skb at the moment
> > > xenvif_handle_frag_list() returns -ENOMEM. This eventually results in
> > > leaking grant ref mappings since xenvif_zerocopy_callback() is never
> > > called for these fragments. Those eventually build up and cause Xen
> > > to kill Dom0 as the slots get reused for new mappings.
> > >
> > > That behavior is observed under certain workloads where sudden spikes
> > > of page cache usage for writes coexist with active atomic skb allocations.
> > >
> > > Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
> > > ---
> > > drivers/net/xen-netback/netback.c | 3 +++
> > > 1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
> > > index 80aae3a..2023317 100644
> > > --- a/drivers/net/xen-netback/netback.c
> > > +++ b/drivers/net/xen-netback/netback.c
> > > @@ -1146,9 +1146,12 @@ static int xenvif_tx_submit(struct xenvif_queue *queue)
> > >
> > > if (unlikely(skb_has_frag_list(skb))) {
> > > if (xenvif_handle_frag_list(queue, skb)) {
> > > + struct sk_buff *nskb =
> > > + skb_shinfo(skb)->frag_list;
> > > if (net_ratelimit())
> > > netdev_err(queue->vif->dev,
> > > "Not enough memory to consolidate frag_list!\n");
> > > + xenvif_skb_zerocopy_prepare(queue, nskb);
> > > xenvif_skb_zerocopy_prepare(queue, skb);
> > > kfree_skb(skb);
> > > continue;
> >
> > Whilst this fix will do the job, I think it would be better to get rid of the kfree_skb() from
> inside xenvif_handle_frag_list() and always deal with it here rather than having it happen in two
> different places. Something like the following...
>
> +1 for having only one place.
>
> >
> > ---8<---
> > diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
> > index 80aae3a32c2a..093c7b860772 100644
> > --- a/drivers/net/xen-netback/netback.c
> > +++ b/drivers/net/xen-netback/netback.c
> > @@ -1027,13 +1027,13 @@ static void xenvif_tx_build_gops(struct xenvif_queue *queue,
> > /* Consolidate skb with a frag_list into a brand new one with local pages on
> > * frags. Returns 0 or -ENOMEM if can't allocate new pages.
> > */
> > -static int xenvif_handle_frag_list(struct xenvif_queue *queue, struct sk_buff *skb)
> > +static int xenvif_handle_frag_list(struct xenvif_queue *queue, struct sk_buff *diff --git
> a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
> > index 80aae3a32c2a..093c7b860772 100644
> > --- a/drivers/net/xen-netback/netback.c
> > +++ b/drivers/net/xen-netback/netback.c
> > @@ -1027,13 +1027,13 @@ static void xenvif_tx_build_gops(struct xenvif_queue *qu
> > eue,
> > /* Consolidate skb with a frag_list into a brand new one with local pages on
> > * frags. Returns 0 or -ENOMEM if can't allocate new pages.
> > */
> > -static int xenvif_handle_frag_list(struct xenvif_queue *queue, struct sk_buff *
> > skb)
> > +static int xenvif_handle_frag_list(struct xenvif_queue *queue, struct sk_buff *
> > skb,
> > + struct sk_buff *nskb)
> > {
> > unsigned int offset = skb_headlen(skb);
> > skb_frag_t frags[MAX_SKB_FRAGS];
> > int i, f;
> > struct ubuf_info *uarg;
> > - struct sk_buff *nskb = skb_shinfo(skb)->frag_list;
> >
> > queue->stats.tx_zerocopy_sent += 2;
> > queue->stats.tx_frag_overflow++;
> > @@ -1072,11 +1072,6 @@ static int xenvif_handle_frag_list(struct xenvif_queue *q
> > ueue, struct sk_buff *s
> > skb_frag_size_set(&frags[i], len);
> > }
> >
> > - /* Copied all the bits from the frag list -- free it. */
> > - skb_frag_list_init(skb);
> > - xenvif_skb_zerocopy_prepare(queue, nskb);
> > - kfree_skb(nskb);
> > -
> > /* Release all the original (foreign) frags. */
> > for (f = 0; f < skb_shinfo(skb)->nr_frags; f++)
> > skb_frag_unref(skb, f);
> > @@ -1145,7 +1140,11 @@ static int xenvif_tx_submit(struct xenvif_queue *queue)
> > xenvif_fill_frags(queue, skb);
> >
> > if (unlikely(skb_has_frag_list(skb))) {
> > - if (xenvif_handle_frag_list(queue, skb)) {
> > + struct sk_buff *nskb = skb_shinfo(skb)->frag_list;
> > +
> > + xenvif_skb_zerocopy_prepare(queue, nskb);
> > +
> > + if (xenvif_handle_frag_list(queue, skb, nskb)) {
> > if (net_ratelimit())
> > netdev_err(queue->vif->dev,
> > "Not enough memory to consolidate frag_list!\n");
> > @@ -1153,6 +1152,10 @@ static int xenvif_tx_submit(struct xenvif_queue *queue)
> > kfree_skb(skb);
> > continue;
> > }
> > +
> > + /* Copied all the bits from the frag list. */
> > + skb_frag_list_init(skb);
> > + kfree(nskb);
>
> I think you want kfree_skb here?
No. nskb is the frag list... it is unlinked from skb by the call to skb_frag_list_init() and then it can be freed on its own. The skb is what we need to retain, because that now contains all the data.
Cheers,
Paul
>
> Wei.
>
> > }
> >
> > skb->dev = queue->vif->dev;
> > ---8<---
> >
> > What do you think?
> >
> > Paul
> >
> > > --
> > > 2.7.4
> >
^ permalink raw reply
* [PATCH] sctp: chunk.c: correct format string for size_t in printk
From: Matthias Maennich @ 2019-02-28 11:36 UTC (permalink / raw)
Cc: Matthias Maennich, Vlad Yasevich, Neil Horman,
Marcelo Ricardo Leitner, David S. Miller, linux-sctp, netdev,
linux-kernel
According to Documentation/core-api/printk-formats.rst, size_t should be
printed with %zu, rather than %Zu.
In addition, using %Zu triggers a warning on clang (-Wformat-extra-args):
net/sctp/chunk.c:196:25: warning: data argument not used by format string [-Wformat-extra-args]
__func__, asoc, max_data);
~~~~~~~~~~~~~~~~^~~~~~~~~
./include/linux/printk.h:440:49: note: expanded from macro 'pr_warn_ratelimited'
printk_ratelimited(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~
./include/linux/printk.h:424:17: note: expanded from macro 'printk_ratelimited'
printk(fmt, ##__VA_ARGS__); \
~~~ ^
Fixes: 5b5e0928f742 ("lib/vsprintf.c: remove %Z support")
Link: https://github.com/ClangBuiltLinux/linux/issues/378
Signed-off-by: Matthias Maennich <maennich@google.com>
---
net/sctp/chunk.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/sctp/chunk.c b/net/sctp/chunk.c
index 64bef313d436..5cb7c1ff97e9 100644
--- a/net/sctp/chunk.c
+++ b/net/sctp/chunk.c
@@ -192,7 +192,7 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,
if (unlikely(!max_data)) {
max_data = sctp_min_frag_point(sctp_sk(asoc->base.sk),
sctp_datachk_len(&asoc->stream));
- pr_warn_ratelimited("%s: asoc:%p frag_point is zero, forcing max_data to default minimum (%Zu)",
+ pr_warn_ratelimited("%s: asoc:%p frag_point is zero, forcing max_data to default minimum (%zu)",
__func__, asoc, max_data);
}
--
2.21.0.rc2.261.ga7da99ff1b-goog
^ permalink raw reply related
* Re: [PATCH] xen-netback: fix occasional leak of grant ref mappings under memory pressure
From: Igor Druzhinin @ 2019-02-28 11:43 UTC (permalink / raw)
To: Paul Durrant, Wei Liu
Cc: xen-devel@lists.xenproject.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, davem@davemloft.net
In-Reply-To: <e5ef3c0291854b67841b6c532f7651b9@AMSPEX02CL02.citrite.net>
On 28/02/2019 11:21, Paul Durrant wrote:
>>> @@ -1153,6 +1152,10 @@ static int xenvif_tx_submit(struct xenvif_queue *queue)
>>> kfree_skb(skb);
>>> continue;
>>> }
>>> +
>>> + /* Copied all the bits from the frag list. */
>>> + skb_frag_list_init(skb);
>>> + kfree(nskb);
>>
>> I think you want kfree_skb here?
>
> No. nskb is the frag list... it is unlinked from skb by the call to skb_frag_list_init() and then it can be freed on its own. The skb is what we need to retain, because that now contains all the data.
>
Are you saying previous code in xenvif_handle_frag_list() incorrectly
called kfree_skb()?
Igor
^ permalink raw reply
* RE: [PATCH] xen-netback: fix occasional leak of grant ref mappings under memory pressure
From: Paul Durrant @ 2019-02-28 11:49 UTC (permalink / raw)
To: Igor Druzhinin, Wei Liu
Cc: xen-devel@lists.xenproject.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, davem@davemloft.net
In-Reply-To: <447cd231-3ea4-e56d-29b0-6528804c9e9a@citrix.com>
> -----Original Message-----
> From: Igor Druzhinin [mailto:igor.druzhinin@citrix.com]
> Sent: 28 February 2019 11:44
> To: Paul Durrant <Paul.Durrant@citrix.com>; Wei Liu <wei.liu2@citrix.com>
> Cc: xen-devel@lists.xenproject.org; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> davem@davemloft.net
> Subject: Re: [PATCH] xen-netback: fix occasional leak of grant ref mappings under memory pressure
>
> On 28/02/2019 11:21, Paul Durrant wrote:
> >>> @@ -1153,6 +1152,10 @@ static int xenvif_tx_submit(struct xenvif_queue *queue)
> >>> kfree_skb(skb);
> >>> continue;
> >>> }
> >>> +
> >>> + /* Copied all the bits from the frag list. */
> >>> + skb_frag_list_init(skb);
> >>> + kfree(nskb);
> >>
> >> I think you want kfree_skb here?
> >
> > No. nskb is the frag list... it is unlinked from skb by the call to skb_frag_list_init() and then it
> can be freed on its own. The skb is what we need to retain, because that now contains all the data.
> >
>
> Are you saying previous code in xenvif_handle_frag_list() incorrectly
> called kfree_skb()?
No, it correctly called kfree_skb() on nskb in the success case. What Wei and myself would prefer is that we have a single place that the frag list is freed in both the success and error cases.
Paul
>
> Igor
^ permalink raw reply
* RE: [PATCH] xen-netback: fix occasional leak of grant ref mappings under memory pressure
From: Paul Durrant @ 2019-02-28 12:07 UTC (permalink / raw)
To: Paul Durrant, Wei Liu
Cc: Igor Druzhinin, Wei Liu, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, xen-devel@lists.xenproject.org,
davem@davemloft.net
In-Reply-To: <e5ef3c0291854b67841b6c532f7651b9@AMSPEX02CL02.citrite.net>
> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On Behalf Of Paul Durrant
> Sent: 28 February 2019 11:22
> To: Wei Liu <wei.liu2@citrix.com>
> Cc: Igor Druzhinin <igor.druzhinin@citrix.com>; Wei Liu <wei.liu2@citrix.com>; netdev@vger.kernel.org;
> linux-kernel@vger.kernel.org; xen-devel@lists.xenproject.org; davem@davemloft.net
> Subject: Re: [Xen-devel] [PATCH] xen-netback: fix occasional leak of grant ref mappings under memory
> pressure
>
> > -----Original Message-----
> > From: Wei Liu [mailto:wei.liu2@citrix.com]
> > Sent: 28 February 2019 11:02
> > To: Paul Durrant <Paul.Durrant@citrix.com>
> > Cc: Igor Druzhinin <igor.druzhinin@citrix.com>; xen-devel@lists.xenproject.org;
> > netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Wei Liu <wei.liu2@citrix.com>;
> > davem@davemloft.net
> > Subject: Re: [PATCH] xen-netback: fix occasional leak of grant ref mappings under memory pressure
> >
> > On Thu, Feb 28, 2019 at 09:46:57AM +0000, Paul Durrant wrote:
> > > > -----Original Message-----
> > > > From: Igor Druzhinin [mailto:igor.druzhinin@citrix.com]
> > > > Sent: 28 February 2019 02:03
> > > > To: xen-devel@lists.xenproject.org; netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> > > > Cc: Wei Liu <wei.liu2@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>; davem@davemloft.net;
> > Igor
> > > > Druzhinin <igor.druzhinin@citrix.com>
> > > > Subject: [PATCH] xen-netback: fix occasional leak of grant ref mappings under memory pressure
> > > >
> > > > Zero-copy callback flag is not yet set on frag list skb at the moment
> > > > xenvif_handle_frag_list() returns -ENOMEM. This eventually results in
> > > > leaking grant ref mappings since xenvif_zerocopy_callback() is never
> > > > called for these fragments. Those eventually build up and cause Xen
> > > > to kill Dom0 as the slots get reused for new mappings.
> > > >
> > > > That behavior is observed under certain workloads where sudden spikes
> > > > of page cache usage for writes coexist with active atomic skb allocations.
> > > >
> > > > Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
> > > > ---
> > > > drivers/net/xen-netback/netback.c | 3 +++
> > > > 1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
> > > > index 80aae3a..2023317 100644
> > > > --- a/drivers/net/xen-netback/netback.c
> > > > +++ b/drivers/net/xen-netback/netback.c
> > > > @@ -1146,9 +1146,12 @@ static int xenvif_tx_submit(struct xenvif_queue *queue)
> > > >
> > > > if (unlikely(skb_has_frag_list(skb))) {
> > > > if (xenvif_handle_frag_list(queue, skb)) {
> > > > + struct sk_buff *nskb =
> > > > + skb_shinfo(skb)->frag_list;
> > > > if (net_ratelimit())
> > > > netdev_err(queue->vif->dev,
> > > > "Not enough memory to consolidate frag_list!\n");
> > > > + xenvif_skb_zerocopy_prepare(queue, nskb);
> > > > xenvif_skb_zerocopy_prepare(queue, skb);
> > > > kfree_skb(skb);
> > > > continue;
> > >
> > > Whilst this fix will do the job, I think it would be better to get rid of the kfree_skb() from
> > inside xenvif_handle_frag_list() and always deal with it here rather than having it happen in two
> > different places. Something like the following...
> >
> > +1 for having only one place.
> >
> > >
> > > ---8<---
> > > diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
> > > index 80aae3a32c2a..093c7b860772 100644
> > > --- a/drivers/net/xen-netback/netback.c
> > > +++ b/drivers/net/xen-netback/netback.c
> > > @@ -1027,13 +1027,13 @@ static void xenvif_tx_build_gops(struct xenvif_queue *queue,
> > > /* Consolidate skb with a frag_list into a brand new one with local pages on
> > > * frags. Returns 0 or -ENOMEM if can't allocate new pages.
> > > */
> > > -static int xenvif_handle_frag_list(struct xenvif_queue *queue, struct sk_buff *skb)
> > > +static int xenvif_handle_frag_list(struct xenvif_queue *queue, struct sk_buff *diff --git
> > a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
> > > index 80aae3a32c2a..093c7b860772 100644
> > > --- a/drivers/net/xen-netback/netback.c
> > > +++ b/drivers/net/xen-netback/netback.c
> > > @@ -1027,13 +1027,13 @@ static void xenvif_tx_build_gops(struct xenvif_queue *qu
> > > eue,
> > > /* Consolidate skb with a frag_list into a brand new one with local pages on
> > > * frags. Returns 0 or -ENOMEM if can't allocate new pages.
> > > */
> > > -static int xenvif_handle_frag_list(struct xenvif_queue *queue, struct sk_buff *
> > > skb)
> > > +static int xenvif_handle_frag_list(struct xenvif_queue *queue, struct sk_buff *
> > > skb,
> > > + struct sk_buff *nskb)
> > > {
> > > unsigned int offset = skb_headlen(skb);
> > > skb_frag_t frags[MAX_SKB_FRAGS];
> > > int i, f;
> > > struct ubuf_info *uarg;
> > > - struct sk_buff *nskb = skb_shinfo(skb)->frag_list;
> > >
> > > queue->stats.tx_zerocopy_sent += 2;
> > > queue->stats.tx_frag_overflow++;
> > > @@ -1072,11 +1072,6 @@ static int xenvif_handle_frag_list(struct xenvif_queue *q
> > > ueue, struct sk_buff *s
> > > skb_frag_size_set(&frags[i], len);
> > > }
> > >
> > > - /* Copied all the bits from the frag list -- free it. */
> > > - skb_frag_list_init(skb);
> > > - xenvif_skb_zerocopy_prepare(queue, nskb);
> > > - kfree_skb(nskb);
> > > -
> > > /* Release all the original (foreign) frags. */
> > > for (f = 0; f < skb_shinfo(skb)->nr_frags; f++)
> > > skb_frag_unref(skb, f);
> > > @@ -1145,7 +1140,11 @@ static int xenvif_tx_submit(struct xenvif_queue *queue)
> > > xenvif_fill_frags(queue, skb);
> > >
> > > if (unlikely(skb_has_frag_list(skb))) {
> > > - if (xenvif_handle_frag_list(queue, skb)) {
> > > + struct sk_buff *nskb = skb_shinfo(skb)->frag_list;
> > > +
> > > + xenvif_skb_zerocopy_prepare(queue, nskb);
> > > +
> > > + if (xenvif_handle_frag_list(queue, skb, nskb)) {
> > > if (net_ratelimit())
> > > netdev_err(queue->vif->dev,
> > > "Not enough memory to consolidate
> frag_list!\n");
> > > @@ -1153,6 +1152,10 @@ static int xenvif_tx_submit(struct xenvif_queue *queue)
> > > kfree_skb(skb);
> > > continue;
> > > }
> > > +
> > > + /* Copied all the bits from the frag list. */
> > > + skb_frag_list_init(skb);
> > > + kfree(nskb);
> >
> > I think you want kfree_skb here?
>
> No. nskb is the frag list... it is unlinked from skb by the call to skb_frag_list_init() and then it
> can be freed on its own. The skb is what we need to retain, because that now contains all the data.
Sorry I misread/understood what you were getting at. Yes, I meant kfree_skb(nskb).
Paul
>
> Cheers,
>
> Paul
>
> >
> > Wei.
> >
> > > }
> > >
> > > skb->dev = queue->vif->dev;
> > > ---8<---
> > >
> > > What do you think?
> > >
> > > Paul
> > >
> > > > --
> > > > 2.7.4
> > >
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply
* [PATCH v4 iproute2-next 02/11] devlink: Fix print of uint64_t
From: Aya Levin @ 2019-02-28 12:12 UTC (permalink / raw)
To: David Ahern; +Cc: netdev, Jiri Pirko, Moshe Shemesh, Eran Ben Elisha, Aya Levin
In-Reply-To: <1551355984-15752-1-git-send-email-ayal@mellanox.com>
This patch prints uint64_t with its corresponding format and avoid implicit
cast to uint32_t.
Signed-off-by: Aya Levin <ayal@mellanox.com>
Reviewed-by: Moshe Shemesh <moshe@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
---
devlink/devlink.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/devlink/devlink.c b/devlink/devlink.c
index a774b196b7fd..8a28b5d5c26f 100644
--- a/devlink/devlink.c
+++ b/devlink/devlink.c
@@ -1651,7 +1651,14 @@ static void pr_out_u64(struct dl *dl, const char *name, uint64_t val)
if (val == (uint64_t) -1)
return pr_out_str(dl, name, "unlimited");
- return pr_out_uint(dl, name, val);
+ if (dl->json_output) {
+ jsonw_u64_field(dl->jw, name, val);
+ } else {
+ if (g_indent_newline)
+ pr_out("%s %lu", name, val);
+ else
+ pr_out(" %s %lu", name, val);
+ }
}
static void pr_out_region_chunk_start(struct dl *dl, uint64_t addr)
--
2.14.1
^ permalink raw reply related
* [PATCH v4 iproute2-next 00/11] Add support for devlink health
From: Aya Levin @ 2019-02-28 12:12 UTC (permalink / raw)
To: David Ahern; +Cc: netdev, Jiri Pirko, Moshe Shemesh, Eran Ben Elisha, Aya Levin
This series adds support for devlink health commands:
devlink health show [ DEV reporter REPORTER_NAME ]
devlink health recover DEV reporter REPORTER_NAME
devlink health diagnose DEV reporter REPORTER_NAME
devlink health dump show DEV reporter REPORTER_NAME
devlink health dump clear DEV reporter REPORTER_NAME
devlink health set DEV reporter REPORTER_NAME { grace_period | auto_recover } { msec | boolean }
The first patch refactors the validation of input parameters, which
grow way too long. Second and third patches fix bugs that were
discovered during the devlink health development. The forth patch adds
helper functions which enable output of value and labels separately.
Patches 5-10 add the devlink health functionality by command, the last
is the man page.
Changelog:
v2:
-Add patch #4.
-Separate patch "Add support for devlink health" into patches (5-10)
by command.
-Patch #1 Changed function's name dl_args_finding_required_validate
and a small refactor.
-Modify show command's output.
v3:
-Add blank row that was wrongly added patch #4
-Rephrase commit message patch #5
-Some refactoring in patch #5
v4:
-Rebase: fix conflict with patch #5
Aya Levin (11):
devlink: Refactor validation of finding required arguments
devlink: Fix print of uint64_t
devlink: Fix boolean JSON print
devlink: Add helper functions for name and value separately
devlink: Add devlink health show command
devlink: Add devlink health recover command
devlink: Add devlink health diagnose command
devlink: Add devlink health dump show command
devlink: Add devlink health dump clear command
devlink: Add devlink health set command
devlink: Add devlink-health man page
devlink/devlink.c | 725 ++++++++++++++++++++++++++++++++++++++--------
man/man8/devlink-health.8 | 197 +++++++++++++
man/man8/devlink.8 | 7 +-
3 files changed, 815 insertions(+), 114 deletions(-)
create mode 100644 man/man8/devlink-health.8
--
2.14.1
^ permalink raw reply
* [PATCH v4 iproute2-next 09/11] devlink: Add devlink health dump clear command
From: Aya Levin @ 2019-02-28 12:13 UTC (permalink / raw)
To: David Ahern; +Cc: netdev, Jiri Pirko, Moshe Shemesh, Eran Ben Elisha, Aya Levin
In-Reply-To: <1551355984-15752-1-git-send-email-ayal@mellanox.com>
Add devlink dump clear command which deletes the last saved dump file.
Clearing the last saved dump enables a new dump file to be saved.
Example:
$ devlink health dump clear pci/0000:00:09.0 reporter tx
Signed-off-by: Aya Levin <ayal@mellanox.com>
Reviewed-by: Moshe Shemesh <moshe@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
---
devlink/devlink.c | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/devlink/devlink.c b/devlink/devlink.c
index 30076d25199b..f64058246979 100644
--- a/devlink/devlink.c
+++ b/devlink/devlink.c
@@ -5816,6 +5816,23 @@ static int cmd_region(struct dl *dl)
return -ENOENT;
}
+static int cmd_health_dump_clear(struct dl *dl)
+{
+ struct nlmsghdr *nlh;
+ int err;
+
+ nlh = mnlg_msg_prepare(dl->nlg, DEVLINK_CMD_HEALTH_REPORTER_DUMP_CLEAR,
+ NLM_F_REQUEST | NLM_F_ACK);
+
+ err = dl_argv_parse_put(nlh, dl,
+ DL_OPT_HANDLE | DL_OPT_HEALTH_REPORTER_NAME, 0);
+ if (err)
+ return err;
+
+ dl_opts_put(nlh, dl);
+ return _mnlg_socket_sndrcv(dl->nlg, nlh, NULL, NULL);
+}
+
static int fmsg_value_show(struct dl *dl, int type, struct nlattr *nl_data)
{
uint8_t *data;
@@ -6162,6 +6179,7 @@ static void cmd_health_help(void)
pr_err(" devlink health recover DEV reporter REPORTER_NAME\n");
pr_err(" devlink health diagnose DEV reporter REPORTER_NAME\n");
pr_err(" devlink health dump show DEV reporter REPORTER_NAME\n");
+ pr_err(" devlink health dump clear DEV reporter REPORTER_NAME\n");
}
static int cmd_health(struct dl *dl)
@@ -6184,6 +6202,9 @@ static int cmd_health(struct dl *dl)
if (dl_argv_match(dl, "show")) {
dl_arg_inc(dl);
return cmd_health_dump_show(dl);
+ } else if (dl_argv_match(dl, "clear")) {
+ dl_arg_inc(dl);
+ return cmd_health_dump_clear(dl);
}
}
pr_err("Command \"%s\" not found\n", dl_argv(dl));
--
2.14.1
^ permalink raw reply related
* [PATCH v4 iproute2-next 10/11] devlink: Add devlink health set command
From: Aya Levin @ 2019-02-28 12:13 UTC (permalink / raw)
To: David Ahern; +Cc: netdev, Jiri Pirko, Moshe Shemesh, Eran Ben Elisha, Aya Levin
In-Reply-To: <1551355984-15752-1-git-send-email-ayal@mellanox.com>
Add devlink set command which enables the user to configure parameters
related to the devlink health mechanism per reporter.
1) grace_period [msec] time interval between auto recoveries.
2) auto_recover [true/false] whether the devlink should execute automatic
recover on error.
Add a helper function to retrieve a boolean value as an input parameter.
Example:
$ devlink health set pci/0000:00:09.0 reporter tx grace_period 3500
$ devlink health set pci/0000:00:09.0 reporter tx auto_recover false
Signed-off-by: Aya Levin <ayal@mellanox.com>
Reviewed-by: Moshe Shemesh <moshe@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
---
devlink/devlink.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 66 insertions(+)
diff --git a/devlink/devlink.c b/devlink/devlink.c
index f64058246979..dc6e73fec20c 100644
--- a/devlink/devlink.c
+++ b/devlink/devlink.c
@@ -209,6 +209,8 @@ static void ifname_map_free(struct ifname_map *ifname_map)
#define DL_OPT_FLASH_FILE_NAME BIT(25)
#define DL_OPT_FLASH_COMPONENT BIT(26)
#define DL_OPT_HEALTH_REPORTER_NAME BIT(27)
+#define DL_OPT_HEALTH_REPORTER_GRACEFUL_PERIOD BIT(27)
+#define DL_OPT_HEALTH_REPORTER_AUTO_RECOVER BIT(28)
struct dl_opts {
uint32_t present; /* flags of present items */
@@ -243,6 +245,8 @@ struct dl_opts {
const char *flash_file_name;
const char *flash_component;
const char *reporter_name;
+ uint64_t reporter_graceful_period;
+ bool reporter_auto_recover;
};
struct dl {
@@ -841,6 +845,24 @@ static int dl_argv_uint16_t(struct dl *dl, uint16_t *p_val)
return 0;
}
+static int dl_argv_bool(struct dl *dl, bool *p_val)
+{
+ char *str = dl_argv_next(dl);
+ int err;
+
+ if (!str) {
+ pr_err("Boolean argument expected\n");
+ return -EINVAL;
+ }
+
+ err = strtobool(str, p_val);
+ if (err) {
+ pr_err("\"%s\" is not a valid boolean value\n", str);
+ return err;
+ }
+ return 0;
+}
+
static int dl_argv_str(struct dl *dl, const char **p_str)
{
const char *str = dl_argv_next(dl);
@@ -1270,6 +1292,21 @@ static int dl_argv_parse(struct dl *dl, uint32_t o_required,
if (err)
return err;
o_found |= DL_OPT_HEALTH_REPORTER_NAME;
+ } else if (dl_argv_match(dl, "grace_period") &&
+ (o_all & DL_OPT_HEALTH_REPORTER_GRACEFUL_PERIOD)) {
+ dl_arg_inc(dl);
+ err = dl_argv_uint64_t(dl,
+ &opts->reporter_graceful_period);
+ if (err)
+ return err;
+ o_found |= DL_OPT_HEALTH_REPORTER_GRACEFUL_PERIOD;
+ } else if (dl_argv_match(dl, "auto_recover") &&
+ (o_all & DL_OPT_HEALTH_REPORTER_AUTO_RECOVER)) {
+ dl_arg_inc(dl);
+ err = dl_argv_bool(dl, &opts->reporter_auto_recover);
+ if (err)
+ return err;
+ o_found |= DL_OPT_HEALTH_REPORTER_AUTO_RECOVER;
} else {
pr_err("Unknown option \"%s\"\n", dl_argv(dl));
return -EINVAL;
@@ -1376,6 +1413,14 @@ static void dl_opts_put(struct nlmsghdr *nlh, struct dl *dl)
if (opts->present & DL_OPT_HEALTH_REPORTER_NAME)
mnl_attr_put_strz(nlh, DEVLINK_ATTR_HEALTH_REPORTER_NAME,
opts->reporter_name);
+ if (opts->present & DL_OPT_HEALTH_REPORTER_GRACEFUL_PERIOD)
+ mnl_attr_put_u64(nlh,
+ DEVLINK_ATTR_HEALTH_REPORTER_GRACEFUL_PERIOD,
+ opts->reporter_graceful_period);
+ if (opts->present & DL_OPT_HEALTH_REPORTER_AUTO_RECOVER)
+ mnl_attr_put_u8(nlh, DEVLINK_ATTR_HEALTH_REPORTER_AUTO_RECOVER,
+ opts->reporter_auto_recover);
+
}
static int dl_argv_parse_put(struct nlmsghdr *nlh, struct dl *dl,
@@ -5816,6 +5861,23 @@ static int cmd_region(struct dl *dl)
return -ENOENT;
}
+static int cmd_health_set_params(struct dl *dl)
+{
+ struct nlmsghdr *nlh;
+ int err;
+
+ nlh = mnlg_msg_prepare(dl->nlg, DEVLINK_CMD_HEALTH_REPORTER_SET,
+ NLM_F_REQUEST | NLM_F_ACK);
+ err = dl_argv_parse(dl, DL_OPT_HANDLE | DL_OPT_HEALTH_REPORTER_NAME,
+ DL_OPT_HEALTH_REPORTER_GRACEFUL_PERIOD |
+ DL_OPT_HEALTH_REPORTER_AUTO_RECOVER);
+ if (err)
+ return err;
+
+ dl_opts_put(nlh, dl);
+ return _mnlg_socket_sndrcv(dl->nlg, nlh, NULL, NULL);
+}
+
static int cmd_health_dump_clear(struct dl *dl)
{
struct nlmsghdr *nlh;
@@ -6180,6 +6242,7 @@ static void cmd_health_help(void)
pr_err(" devlink health diagnose DEV reporter REPORTER_NAME\n");
pr_err(" devlink health dump show DEV reporter REPORTER_NAME\n");
pr_err(" devlink health dump clear DEV reporter REPORTER_NAME\n");
+ pr_err(" devlink health set DEV reporter REPORTER_NAME { grace_period | auto_recover } { msec | boolean }\n");
}
static int cmd_health(struct dl *dl)
@@ -6206,6 +6269,9 @@ static int cmd_health(struct dl *dl)
dl_arg_inc(dl);
return cmd_health_dump_clear(dl);
}
+ } else if (dl_argv_match(dl, "set")) {
+ dl_arg_inc(dl);
+ return cmd_health_set_params(dl);
}
pr_err("Command \"%s\" not found\n", dl_argv(dl));
return -ENOENT;
--
2.14.1
^ permalink raw reply related
* [PATCH v4 iproute2-next 06/11] devlink: Add devlink health recover command
From: Aya Levin @ 2019-02-28 12:12 UTC (permalink / raw)
To: David Ahern; +Cc: netdev, Jiri Pirko, Moshe Shemesh, Eran Ben Elisha, Aya Levin
In-Reply-To: <1551355984-15752-1-git-send-email-ayal@mellanox.com>
Add devlink health recover command which enables the user to initiate a
recovery on a reporter (if a recovery cb was supplied by the reporter).
This operation will increment the recoveries counter displayed in the
show command.
Example:
$ devlink health recover pci/0000:00:09.0 reporter tx
Signed-off-by: Aya Levin <ayal@mellanox.com>
Reviewed-by: Moshe Shemesh <moshe@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
---
devlink/devlink.c | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/devlink/devlink.c b/devlink/devlink.c
index 37eb2054ef06..c77f80c87adc 100644
--- a/devlink/devlink.c
+++ b/devlink/devlink.c
@@ -5815,6 +5815,23 @@ static int cmd_region(struct dl *dl)
return -ENOENT;
}
+static int cmd_health_recover(struct dl *dl)
+{
+ struct nlmsghdr *nlh;
+ int err;
+
+ nlh = mnlg_msg_prepare(dl->nlg, DEVLINK_CMD_HEALTH_REPORTER_RECOVER,
+ NLM_F_REQUEST | NLM_F_ACK);
+
+ err = dl_argv_parse_put(nlh, dl,
+ DL_OPT_HANDLE | DL_OPT_HEALTH_REPORTER_NAME, 0);
+ if (err)
+ return err;
+
+ dl_opts_put(nlh, dl);
+ return _mnlg_socket_sndrcv(dl->nlg, nlh, NULL, NULL);
+}
+
enum devlink_health_reporter_state {
DEVLINK_HEALTH_REPORTER_STATE_HEALTHY,
DEVLINK_HEALTH_REPORTER_STATE_ERROR,
@@ -5954,6 +5971,7 @@ static int cmd_health_show(struct dl *dl)
static void cmd_health_help(void)
{
pr_err("Usage: devlink health show [ dev DEV reporter REPORTER_NAME ]\n");
+ pr_err(" devlink health recover DEV reporter REPORTER_NAME\n");
}
static int cmd_health(struct dl *dl)
@@ -5965,6 +5983,9 @@ static int cmd_health(struct dl *dl)
dl_argv_match(dl, "list") || dl_no_arg(dl)) {
dl_arg_inc(dl);
return cmd_health_show(dl);
+ } else if (dl_argv_match(dl, "recover")) {
+ dl_arg_inc(dl);
+ return cmd_health_recover(dl);
}
pr_err("Command \"%s\" not found\n", dl_argv(dl));
return -ENOENT;
--
2.14.1
^ permalink raw reply related
* [PATCH v4 iproute2-next 11/11] devlink: Add devlink-health man page
From: Aya Levin @ 2019-02-28 12:13 UTC (permalink / raw)
To: David Ahern; +Cc: netdev, Jiri Pirko, Moshe Shemesh, Eran Ben Elisha, Aya Levin
In-Reply-To: <1551355984-15752-1-git-send-email-ayal@mellanox.com>
Add a man page describing devlink health's command set. Also add a
reference link from devlink main man page.
Signed-off-by: Aya Levin <ayal@mellanox.com>
Reviewed-by: Moshe Shemesh <moshe@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
---
man/man8/devlink-health.8 | 197 ++++++++++++++++++++++++++++++++++++++++++++++
man/man8/devlink.8 | 7 +-
2 files changed, 203 insertions(+), 1 deletion(-)
create mode 100644 man/man8/devlink-health.8
diff --git a/man/man8/devlink-health.8 b/man/man8/devlink-health.8
new file mode 100644
index 000000000000..7ed0ae4534dc
--- /dev/null
+++ b/man/man8/devlink-health.8
@@ -0,0 +1,197 @@
+.TH DEVLINK\-HEALTH 8 "20 Feb 2019" "iproute2" "Linux"
+.SH NAME
+devlink-health \- devlink health reporting and recovery
+.SH SYNOPSIS
+.sp
+.ad l
+.in +8
+.ti -8
+.B devlink
+.RI "[ " OPTIONS " ]"
+.B health
+.RI " { " COMMAND " | "
+.BR help " }"
+.sp
+
+.ti -8
+.IR OPTIONS " := { "
+\fB\-V\fR[\fIersion\fR] }
+
+.ti -8
+.BR "devlink health show"
+.RI "[ " DEV ""
+.B reporter
+.RI ""REPORTER " ] "
+
+.ti -8
+.BR "devlink health recover"
+.RI "" DEV ""
+.B reporter
+.RI "" REPORTER ""
+
+.ti -8
+.BR "devlink health diagnose"
+.RI "" DEV ""
+.B reporter
+.RI "" REPORTER ""
+
+.ti -8
+.BR "devlink health dump show"
+.RI "" DEV ""
+.B reporter
+.RI "" REPORTER ""
+
+.ti -8
+.BR "devlink health dump clear"
+.RI "" DEV ""
+.B reporter
+.RI "" REPORTER ""
+
+.ti -8
+.BR "devlink health set"
+.RI "" DEV ""
+.B reporter
+.RI "" REPORTER ""
+.RI " { "
+.B grace_period | auto_recover
+.RI " } { "
+.RI "" msec ""
+.RI "|"
+.RI "" boolean ""
+.RI " } "
+.ti -8
+.B devlink health help
+
+.SH "DESCRIPTION"
+.SS devlink health show - Show status and configuration on all supported reporters on all devlink devices.
+
+.PP
+.I "DEV"
+- specifies the devlink device.
+
+.PP
+.I "REPORTER"
+- specifies the reporter's name registered on the devlink device.
+
+.SS devlink health recover - Initiate a recovery operation on a reporter.
+This action performs a recovery and increases the recoveries counter on success.
+
+.PP
+.I "DEV"
+- specifies the devlink device.
+
+.PP
+.I "REPORTER"
+- specifies the reporter's name registered on the devlink device.
+
+.SS devlink health diagnose - Retrieve diagnostics data on a reporter.
+
+.PP
+.I "DEV"
+- specifies the devlink device.
+
+.PP
+.I "REPORTER"
+- specifies the reporter's name registered on the devlink device.
+
+.SS devlink health dump show - Display the last saved dump.
+
+.PD 0
+.P
+devlink health saves a single dump per reporter. If an dump is
+.P
+not already stored by the Devlink, this command will generate a new
+.P
+dump. The dump can be generated either automatically when a
+.P
+reporter reports on an error or manually at the user's request.
+.PD
+
+.PP
+.I "DEV"
+- specifies the devlink device.
+
+.PP
+.I "REPORTER"
+- specifies the reporter's name registered on the devlink device.
+
+.SS devlink health dump clear - Delete the saved dump.
+Deleting the saved dump enables a generation of a new dump on
+.PD 0
+.P
+the next "devlink health dump show" command.
+.PD
+
+.PP
+.I "DEV"
+- specifies the devlink device.
+
+.PP
+.I "REPORTER"
+- specifies the reporter's name registered on the devlink device.
+
+.SS devlink health set - Enable the user to configure:
+.PD 0
+1) grace_period [msec] - Time interval between consecutive auto recoveries.
+.P
+2) auto_recover [true/false] - Indicates whether the devlink should execute automatic recover on error.
+.P
+Please note that this command is not supported on a reporter which
+doesn't support a recovery method.
+.PD
+
+.PP
+.I "DEV"
+- specifies the devlink device.
+
+.PP
+.I "REPORTER"
+- specifies the reporter's name registered on the devlink device.
+
+.SH "EXAMPLES"
+.PP
+devlink health show
+.RS 4
+List status and configuration of available reporters on devices.
+.RE
+.PP
+devlink health recover pci/0000:00:09.0 reporter tx
+.RS 4
+Initiate recovery on tx reporter registered on pci/0000:00:09.0.
+.RE
+.PP
+devlink health diagnose pci/0000:00:09.0 reporter tx
+.RS 4
+List diagnostics data on the specified device and reporter.
+.RE
+.PP
+devlink health dump show pci/0000:00:09.0 reporter tx
+.RS 4
+Display the last saved dump on the specified device and reporter.
+.RE
+.PP
+devlink health dump clear pci/0000:00:09.0 reporter tx
+.RS 4
+Delete saved dump on the specified device and reporter.
+.RE
+.PP
+devlink health set pci/0000:00:09.0 reporter tx grace_period 3500
+.RS 4
+Set time interval between auto recoveries to minimum of 3500 msec on
+the specified device and reporter.
+.RE
+.PP
+devlink health set pci/0000:00:09.0 reporter tx auto_recover false
+.RS 4
+Turn off auto recovery on the specified device and reporter.
+.RE
+.SH SEE ALSO
+.BR devlink (8),
+.BR devlink-dev (8),
+.BR devlink-port (8),
+.BR devlink-param (8),
+.BR devlink-region (8),
+.br
+
+.SH AUTHOR
+Aya Levin <ayal@mellanox.com>
diff --git a/man/man8/devlink.8 b/man/man8/devlink.8
index 8d527e7e1d60..13d4dcd908b3 100644
--- a/man/man8/devlink.8
+++ b/man/man8/devlink.8
@@ -7,7 +7,7 @@ devlink \- Devlink tool
.in +8
.ti -8
.B devlink
-.RI "[ " OPTIONS " ] { " dev | port | monitor | sb | resource | region " } { " COMMAND " | "
+.RI "[ " OPTIONS " ] { " dev | port | monitor | sb | resource | region | health " } { " COMMAND " | "
.BR help " }"
.sp
@@ -78,6 +78,10 @@ Turn on verbose output.
.B region
- devlink address region access
+.TP
+.B health
+- devlink reporting and recovery
+
.SS
.I COMMAND
@@ -109,6 +113,7 @@ Exit status is 0 if command was successful or a positive integer upon failure.
.BR devlink-sb (8),
.BR devlink-resource (8),
.BR devlink-region (8),
+.BR devlink-health (8),
.br
.SH REPORTING BUGS
--
2.14.1
^ permalink raw reply related
* [PATCH v4 iproute2-next 05/11] devlink: Add devlink health show command
From: Aya Levin @ 2019-02-28 12:12 UTC (permalink / raw)
To: David Ahern; +Cc: netdev, Jiri Pirko, Moshe Shemesh, Eran Ben Elisha, Aya Levin
In-Reply-To: <1551355984-15752-1-git-send-email-ayal@mellanox.com>
Add devlink health show command which displays status and configuration
info on a specific reporter on a device or dump the info on all
reporters on all devices. Add helper functions to display status and
dump's time stamp.
Example:
$ devlink health show pci/0000:00:09.0 reporter tx
pci/0000:00:09.0:
name tx
state healthy error 0 recover 1 last_dump_date 2019-02-14 last_dump_time 10:10:10 grace_period 600 auto_recover true
$ devlink health show pci/0000:00:09.0 reporter tx -jp
{
"health":{
"pci/0000:00:0a.0":[
{
"name":"tx",
"state":"healthy",
"error":0,
"recover":1,
"last_dump_date":"2019-Feb-14",
"last_dump_time":"10:10:10",
"grace_period":600,
"auto_recover":true
}
]
}
Signed-off-by: Aya Levin <ayal@mellanox.com>
Reviewed-by: Moshe Shemesh <moshe@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
---
devlink/devlink.c | 185 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 184 insertions(+), 1 deletion(-)
diff --git a/devlink/devlink.c b/devlink/devlink.c
index c3f874564309..37eb2054ef06 100644
--- a/devlink/devlink.c
+++ b/devlink/devlink.c
@@ -22,6 +22,7 @@
#include <linux/devlink.h>
#include <libmnl/libmnl.h>
#include <netinet/ether.h>
+#include <sys/sysinfo.h>
#include "SNAPSHOT.h"
#include "list.h"
@@ -41,6 +42,10 @@
#define PARAM_CMODE_PERMANENT_STR "permanent"
#define DL_ARGS_REQUIRED_MAX_ERR_LEN 80
+#define HEALTH_REPORTER_STATE_HEALTHY_STR "healthy"
+#define HEALTH_REPORTER_STATE_ERROR_STR "error"
+#define HEALTH_REPORTER_TIMESTAMP_FMT_LEN 80
+
static int g_new_line_count;
#define pr_err(args...) fprintf(stderr, ##args)
@@ -202,6 +207,7 @@ static void ifname_map_free(struct ifname_map *ifname_map)
#define DL_OPT_REGION_LENGTH BIT(24)
#define DL_OPT_FLASH_FILE_NAME BIT(25)
#define DL_OPT_FLASH_COMPONENT BIT(26)
+#define DL_OPT_HEALTH_REPORTER_NAME BIT(27)
struct dl_opts {
uint32_t present; /* flags of present items */
@@ -235,6 +241,7 @@ struct dl_opts {
uint64_t region_length;
const char *flash_file_name;
const char *flash_component;
+ const char *reporter_name;
};
struct dl {
@@ -395,6 +402,13 @@ static const enum mnl_attr_data_type devlink_policy[DEVLINK_ATTR_MAX + 1] = {
[DEVLINK_ATTR_INFO_VERSION_STORED] = MNL_TYPE_NESTED,
[DEVLINK_ATTR_INFO_VERSION_NAME] = MNL_TYPE_STRING,
[DEVLINK_ATTR_INFO_VERSION_VALUE] = MNL_TYPE_STRING,
+ [DEVLINK_ATTR_HEALTH_REPORTER] = MNL_TYPE_NESTED,
+ [DEVLINK_ATTR_HEALTH_REPORTER_NAME] = MNL_TYPE_STRING,
+ [DEVLINK_ATTR_HEALTH_REPORTER_STATE] = MNL_TYPE_U8,
+ [DEVLINK_ATTR_HEALTH_REPORTER_ERR_COUNT] = MNL_TYPE_U64,
+ [DEVLINK_ATTR_HEALTH_REPORTER_RECOVER_COUNT] = MNL_TYPE_U64,
+ [DEVLINK_ATTR_HEALTH_REPORTER_DUMP_TS] = MNL_TYPE_U64,
+ [DEVLINK_ATTR_HEALTH_REPORTER_GRACEFUL_PERIOD] = MNL_TYPE_U64,
};
static int attr_cb(const struct nlattr *attr, void *data)
@@ -980,6 +994,7 @@ static const struct dl_args_metadata dl_args_required[] = {
{DL_OPT_REGION_SNAPSHOT_ID, "Region snapshot id expected."},
{DL_OPT_REGION_ADDRESS, "Region address value expected."},
{DL_OPT_REGION_LENGTH, "Region length value expected."},
+ {DL_OPT_HEALTH_REPORTER_NAME, "Reporter's name is expected."},
};
static int dl_args_finding_required_validate(uint32_t o_required,
@@ -1247,6 +1262,13 @@ static int dl_argv_parse(struct dl *dl, uint32_t o_required,
if (err)
return err;
o_found |= DL_OPT_FLASH_COMPONENT;
+ } else if (dl_argv_match(dl, "reporter") &&
+ (o_all & DL_OPT_HEALTH_REPORTER_NAME)) {
+ dl_arg_inc(dl);
+ err = dl_argv_str(dl, &opts->reporter_name);
+ if (err)
+ return err;
+ o_found |= DL_OPT_HEALTH_REPORTER_NAME;
} else {
pr_err("Unknown option \"%s\"\n", dl_argv(dl));
return -EINVAL;
@@ -1350,6 +1372,9 @@ static void dl_opts_put(struct nlmsghdr *nlh, struct dl *dl)
if (opts->present & DL_OPT_FLASH_COMPONENT)
mnl_attr_put_strz(nlh, DEVLINK_ATTR_FLASH_UPDATE_COMPONENT,
opts->flash_component);
+ if (opts->present & DL_OPT_HEALTH_REPORTER_NAME)
+ mnl_attr_put_strz(nlh, DEVLINK_ATTR_HEALTH_REPORTER_NAME,
+ opts->reporter_name);
}
static int dl_argv_parse_put(struct nlmsghdr *nlh, struct dl *dl,
@@ -5790,11 +5815,166 @@ static int cmd_region(struct dl *dl)
return -ENOENT;
}
+enum devlink_health_reporter_state {
+ DEVLINK_HEALTH_REPORTER_STATE_HEALTHY,
+ DEVLINK_HEALTH_REPORTER_STATE_ERROR,
+};
+
+static const char *health_state_name(uint8_t state)
+{
+ switch (state) {
+ case DEVLINK_HEALTH_REPORTER_STATE_HEALTHY:
+ return HEALTH_REPORTER_STATE_HEALTHY_STR;
+ case DEVLINK_HEALTH_REPORTER_STATE_ERROR:
+ return HEALTH_REPORTER_STATE_ERROR_STR;
+ default:
+ return "<unknown state>";
+ }
+}
+
+static void format_logtime(uint64_t time_ms, char *ts_date, char *ts_time)
+{
+ struct sysinfo s_info;
+ struct tm *info;
+ time_t now, sec;
+ int err;
+
+ time(&now);
+ info = localtime(&now);
+ err = sysinfo(&s_info);
+ if (err)
+ goto out;
+ /* Subtract uptime in sec from now yields the time of system
+ * uptime. To this, add time_ms which is the amount of
+ * milliseconds elapsed between uptime and the dump taken.
+ */
+ sec = now - s_info.uptime + time_ms / 1000;
+ info = localtime(&sec);
+out:
+ strftime(ts_date, HEALTH_REPORTER_TIMESTAMP_FMT_LEN, "%Y-%m-%d", info);
+ strftime(ts_time, HEALTH_REPORTER_TIMESTAMP_FMT_LEN, "%H:%M:%S", info);
+}
+
+static void pr_out_health(struct dl *dl, struct nlattr **tb_health)
+{
+ struct nlattr *tb[DEVLINK_ATTR_MAX + 1] = {};
+ enum devlink_health_reporter_state state;
+ const struct nlattr *attr;
+ uint64_t time_ms;
+ int err;
+
+ err = mnl_attr_parse_nested(tb_health[DEVLINK_ATTR_HEALTH_REPORTER],
+ attr_cb, tb);
+ if (err != MNL_CB_OK)
+ return;
+
+ if (!tb[DEVLINK_ATTR_HEALTH_REPORTER_NAME] ||
+ !tb[DEVLINK_ATTR_HEALTH_REPORTER_ERR_COUNT] ||
+ !tb[DEVLINK_ATTR_HEALTH_REPORTER_RECOVER_COUNT] ||
+ !tb[DEVLINK_ATTR_HEALTH_REPORTER_STATE])
+ return;
+
+ pr_out_handle_start_arr(dl, tb_health);
+
+ pr_out_str(dl, "name",
+ mnl_attr_get_str(tb[DEVLINK_ATTR_HEALTH_REPORTER_NAME]));
+ if (!dl->json_output) {
+ __pr_out_newline();
+ __pr_out_indent_inc();
+ }
+ state = mnl_attr_get_u8(tb[DEVLINK_ATTR_HEALTH_REPORTER_STATE]);
+ pr_out_str(dl, "state", health_state_name(state));
+ pr_out_u64(dl, "error",
+ mnl_attr_get_u64(tb[DEVLINK_ATTR_HEALTH_REPORTER_ERR_COUNT]));
+ pr_out_u64(dl, "recover",
+ mnl_attr_get_u64(tb[DEVLINK_ATTR_HEALTH_REPORTER_RECOVER_COUNT]));
+ if (tb[DEVLINK_ATTR_HEALTH_REPORTER_DUMP_TS]) {
+ char dump_date[HEALTH_REPORTER_TIMESTAMP_FMT_LEN];
+ char dump_time[HEALTH_REPORTER_TIMESTAMP_FMT_LEN];
+
+ attr = tb[DEVLINK_ATTR_HEALTH_REPORTER_DUMP_TS];
+ time_ms = mnl_attr_get_u64(attr);
+ format_logtime(time_ms, dump_date, dump_time);
+
+ pr_out_str(dl, "last_dump_date", dump_date);
+ pr_out_str(dl, "last_dump_time", dump_time);
+ }
+ if (tb[DEVLINK_ATTR_HEALTH_REPORTER_GRACEFUL_PERIOD])
+ pr_out_u64(dl, "grace_period",
+ mnl_attr_get_u64(tb[DEVLINK_ATTR_HEALTH_REPORTER_GRACEFUL_PERIOD]));
+ if (tb[DEVLINK_ATTR_HEALTH_REPORTER_AUTO_RECOVER])
+ pr_out_bool(dl, "auto_recover",
+ mnl_attr_get_u8(tb[DEVLINK_ATTR_HEALTH_REPORTER_AUTO_RECOVER]));
+
+ __pr_out_indent_dec();
+ pr_out_handle_end(dl);
+}
+
+static int cmd_health_show_cb(const struct nlmsghdr *nlh, void *data)
+{
+ struct genlmsghdr *genl = mnl_nlmsg_get_payload(nlh);
+ struct nlattr *tb[DEVLINK_ATTR_MAX + 1] = {};
+ struct dl *dl = data;
+
+ mnl_attr_parse(nlh, sizeof(*genl), attr_cb, tb);
+ if (!tb[DEVLINK_ATTR_BUS_NAME] || !tb[DEVLINK_ATTR_DEV_NAME] ||
+ !tb[DEVLINK_ATTR_HEALTH_REPORTER])
+ return MNL_CB_ERROR;
+
+ pr_out_health(dl, tb);
+
+ return MNL_CB_OK;
+}
+
+static int cmd_health_show(struct dl *dl)
+{
+ struct nlmsghdr *nlh;
+ uint16_t flags = NLM_F_REQUEST | NLM_F_ACK;
+ int err;
+
+ if (dl_argc(dl) == 0)
+ flags |= NLM_F_DUMP;
+ nlh = mnlg_msg_prepare(dl->nlg, DEVLINK_CMD_HEALTH_REPORTER_GET,
+ flags);
+
+ if (dl_argc(dl) > 0) {
+ err = dl_argv_parse_put(nlh, dl,
+ DL_OPT_HANDLE |
+ DL_OPT_HEALTH_REPORTER_NAME, 0);
+ if (err)
+ return err;
+ }
+ pr_out_section_start(dl, "health");
+
+ err = _mnlg_socket_sndrcv(dl->nlg, nlh, cmd_health_show_cb, dl);
+ pr_out_section_end(dl);
+ return err;
+}
+
+static void cmd_health_help(void)
+{
+ pr_err("Usage: devlink health show [ dev DEV reporter REPORTER_NAME ]\n");
+}
+
+static int cmd_health(struct dl *dl)
+{
+ if (dl_argv_match(dl, "help")) {
+ cmd_health_help();
+ return 0;
+ } else if (dl_argv_match(dl, "show") ||
+ dl_argv_match(dl, "list") || dl_no_arg(dl)) {
+ dl_arg_inc(dl);
+ return cmd_health_show(dl);
+ }
+ pr_err("Command \"%s\" not found\n", dl_argv(dl));
+ return -ENOENT;
+}
+
static void help(void)
{
pr_err("Usage: devlink [ OPTIONS ] OBJECT { COMMAND | help }\n"
" devlink [ -f[orce] ] -b[atch] filename\n"
- "where OBJECT := { dev | port | sb | monitor | dpipe | resource | region }\n"
+ "where OBJECT := { dev | port | sb | monitor | dpipe | resource | region | health }\n"
" OPTIONS := { -V[ersion] | -n[o-nice-names] | -j[son] | -p[retty] | -v[erbose] }\n");
}
@@ -5827,6 +6007,9 @@ static int dl_cmd(struct dl *dl, int argc, char **argv)
} else if (dl_argv_match(dl, "region")) {
dl_arg_inc(dl);
return cmd_region(dl);
+ } else if (dl_argv_match(dl, "health")) {
+ dl_arg_inc(dl);
+ return cmd_health(dl);
}
pr_err("Object \"%s\" not found\n", dl_argv(dl));
return -ENOENT;
--
2.14.1
^ 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