* [PATCH 0/7] net: thunderx: implement DMAC filtering support
From: Vadim Lomovtsev @ 2018-03-27 15:07 UTC (permalink / raw)
To: sgoutham, sunil.kovvuri, robert.richter, linux-arm-kernel, netdev,
linux-kernel
Cc: dnelson, Vadim Lomovtsev
From: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
By default CN88XX BGX accepts all incoming multicast and broadcast
packets and filtering is disabled. The nic driver doesn't provide
an ability to change such behaviour.
This series is to implement DMAC filtering management for CN88XX
nic driver allowing user to enable/disable filtering and configure
specific MAC addresses to filter traffic.
Vadim Lomovtsev (7):
net: thunderx: move filter register related macro into proper place
net: thunderx: add MAC address filter tracking for LMAC
net: thunderx: add multicast filter management support
net: thunderx: add new messages for handle ndo_set_rx_mode callback
net: thunderx: add XCAST messages handlers for PF
net: thunderx: add workqueue control structures for handle
ndo_set_rx_mode request
net: thunderx: add ndo_set_rx_mode callback implementation for VF
drivers/net/ethernet/cavium/thunder/nic.h | 29 ++++
drivers/net/ethernet/cavium/thunder/nic_main.c | 45 ++++-
drivers/net/ethernet/cavium/thunder/nicvf_main.c | 108 +++++++++++-
drivers/net/ethernet/cavium/thunder/thunder_bgx.c | 196 ++++++++++++++++++++--
drivers/net/ethernet/cavium/thunder/thunder_bgx.h | 17 +-
5 files changed, 366 insertions(+), 29 deletions(-)
--
2.14.3
^ permalink raw reply
* [PATCH 2/7] net: thunderx: add MAC address filter tracking for LMAC
From: Vadim Lomovtsev @ 2018-03-27 15:07 UTC (permalink / raw)
To: sgoutham, sunil.kovvuri, robert.richter, linux-arm-kernel, netdev,
linux-kernel
Cc: dnelson, Vadim Lomovtsev
In-Reply-To: <20180327150736.10718-1-Vadim.Lomovtsev@caviumnetworks.com>
From: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
The ThunderX NIC has two Ethernet Interfaces (BGX) each of them could has
up to four Logical MACs configured. Each of BGX has 32 filters to be
configured for filtering ingress packets. The number of filters available
to particular LMAC is from 8 (if we have four LMACs configured per BGX)
up to 32 (in case of only one LMAC is configured per BGX).
At the same time the NIC could present up to 128 VFs to OS as network
interfaces, each of them kernel will configure with set of MAC addresses
for filtering. So to prevent dupes in BGX filter registers from different
network interfaces it is required to cache and track all filter configuration
requests prior to applying them onto BGX filter registers.
This commit is to update LMAC structures with control fields to
allocate/releasing filters tracking list along with implementing
dmac array allocate/release per LMAC.
Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
---
drivers/net/ethernet/cavium/thunder/thunder_bgx.c | 42 +++++++++++++++--------
1 file changed, 28 insertions(+), 14 deletions(-)
diff --git a/drivers/net/ethernet/cavium/thunder/thunder_bgx.c b/drivers/net/ethernet/cavium/thunder/thunder_bgx.c
index 0dd211605eb1..cf0cc19c03c5 100644
--- a/drivers/net/ethernet/cavium/thunder/thunder_bgx.c
+++ b/drivers/net/ethernet/cavium/thunder/thunder_bgx.c
@@ -37,9 +37,18 @@ enum MCAST_MODE {
#define MCAST_MODE_MASK 0x3
#define BGX_MCAST_MODE(x) (x << 1)
+struct dmac_map {
+ u64 vf_map;
+ u64 dmac;
+};
+
struct lmac {
struct bgx *bgx;
- int dmac;
+ /* actual number of DMACs configured */
+ u8 dmacs_cfg;
+ /* overal number of possible DMACs could be configured per LMAC */
+ u8 dmacs_count;
+ struct dmac_map *dmacs; /* DMAC:VFs tracking filter array */
u8 mac[ETH_ALEN];
u8 lmac_type;
u8 lane_to_sds;
@@ -236,6 +245,17 @@ void bgx_set_lmac_mac(int node, int bgx_idx, int lmacid, const u8 *mac)
}
EXPORT_SYMBOL(bgx_set_lmac_mac);
+static void bgx_flush_dmac_cam_filter(struct bgx *bgx, int lmacid)
+{
+ struct lmac *lmac = NULL;
+ u8 idx = 0;
+
+ lmac = &bgx->lmac[lmacid];
+ /* reset CAM filters */
+ for (idx = 0; idx < lmac->dmacs_count; idx++)
+ bgx_reg_write(bgx, 0, BGX_CMR_RX_DMACX_CAM + ((lmacid * lmac->dmacs_count) + idx) * sizeof(u64), 0);
+}
+
void bgx_lmac_rx_tx_enable(int node, int bgx_idx, int lmacid, bool enable)
{
struct bgx *bgx = get_bgx(node, bgx_idx);
@@ -481,18 +501,6 @@ u64 bgx_get_tx_stats(int node, int bgx_idx, int lmac, int idx)
}
EXPORT_SYMBOL(bgx_get_tx_stats);
-static void bgx_flush_dmac_addrs(struct bgx *bgx, int lmac)
-{
- u64 offset;
-
- while (bgx->lmac[lmac].dmac > 0) {
- offset = ((bgx->lmac[lmac].dmac - 1) * sizeof(u64)) +
- (lmac * MAX_DMAC_PER_LMAC * sizeof(u64));
- bgx_reg_write(bgx, 0, BGX_CMR_RX_DMACX_CAM + offset, 0);
- bgx->lmac[lmac].dmac--;
- }
-}
-
/* Configure BGX LMAC in internal loopback mode */
void bgx_lmac_internal_loopback(int node, int bgx_idx,
int lmac_idx, bool enable)
@@ -925,6 +933,11 @@ static int bgx_lmac_enable(struct bgx *bgx, u8 lmacid)
bgx_reg_write(bgx, lmacid, BGX_SMUX_TX_MIN_PKT, 60 + 4);
}
+ /* actual number of filters available to exact LMAC */
+ lmac->dmacs_count = (RX_DMAC_COUNT / bgx->lmac_count);
+ lmac->dmacs = kcalloc(lmac->dmacs_count, sizeof(*lmac->dmacs),
+ GFP_KERNEL);
+
/* Enable lmac */
bgx_reg_modify(bgx, lmacid, BGX_CMRX_CFG, CMR_EN);
@@ -1011,7 +1024,8 @@ static void bgx_lmac_disable(struct bgx *bgx, u8 lmacid)
cfg &= ~CMR_EN;
bgx_reg_write(bgx, lmacid, BGX_CMRX_CFG, cfg);
- bgx_flush_dmac_addrs(bgx, lmacid);
+ bgx_flush_dmac_cam_filter(bgx, lmacid);
+ kfree(lmac->dmacs);
if ((lmac->lmac_type != BGX_MODE_XFI) &&
(lmac->lmac_type != BGX_MODE_XLAUI) &&
--
2.14.3
^ permalink raw reply related
* [PATCH 3/7] net: thunderx: add multicast filter management support
From: Vadim Lomovtsev @ 2018-03-27 15:07 UTC (permalink / raw)
To: sgoutham, sunil.kovvuri, robert.richter, linux-arm-kernel, netdev,
linux-kernel
Cc: dnelson, Vadim Lomovtsev
In-Reply-To: <20180327150736.10718-1-Vadim.Lomovtsev@caviumnetworks.com>
From: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
The ThunderX NIC could be partitioned to up to 128 VFs and thus
represented to system. Each VF is mapped to pair BGX:LMAC, and each of VF
is configured by kernel individually. Eventually the bunch of VFs could be
mapped onto same pair BGX:LMAC and thus could cause several multicast
filtering configuration requests to LMAC with the same MAC addresses.
This commit is to add ThunderX NIC BGX filtering manipulation routines.
Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
---
drivers/net/ethernet/cavium/thunder/thunder_bgx.c | 141 ++++++++++++++++++++++
drivers/net/ethernet/cavium/thunder/thunder_bgx.h | 8 ++
2 files changed, 149 insertions(+)
diff --git a/drivers/net/ethernet/cavium/thunder/thunder_bgx.c b/drivers/net/ethernet/cavium/thunder/thunder_bgx.c
index cf0cc19c03c5..52fef3dab0a3 100644
--- a/drivers/net/ethernet/cavium/thunder/thunder_bgx.c
+++ b/drivers/net/ethernet/cavium/thunder/thunder_bgx.c
@@ -256,6 +256,147 @@ static void bgx_flush_dmac_cam_filter(struct bgx *bgx, int lmacid)
bgx_reg_write(bgx, 0, BGX_CMR_RX_DMACX_CAM + ((lmacid * lmac->dmacs_count) + idx) * sizeof(u64), 0);
}
+static void bgx_lmac_remove_filters(struct lmac *lmac, u8 vf_id)
+{
+ int i = 0;
+
+ if (!lmac)
+ return;
+
+ /* We've got reset filters request from some of attached VF, while the
+ * others might want to keep their configuration. So in this case lets
+ * iterate over all of configured filters and decrease number of
+ * referencies. if some addresses get zero refs remove them from list
+ */
+ for (i = lmac->dmacs_cfg - 1; i >= 0; i--) {
+ lmac->dmacs[i].vf_map &= ~BIT_ULL(vf_id);
+ if (!lmac->dmacs[i].vf_map) {
+ lmac->dmacs_cfg--;
+ lmac->dmacs[i].dmac = 0;
+ lmac->dmacs[i].vf_map = 0;
+ }
+ }
+}
+
+static int bgx_lmac_save_filter(struct lmac *lmac, u64 dmac, u8 vf_id)
+{
+ u8 i = 0;
+
+ if (!lmac)
+ return;
+
+ /* At the same time we could have several VFs 'attached' to some
+ * particular LMAC, and each VF is represented as network interface
+ * for kernel. So from user perspective it should be possible to
+ * manipulate with its' (VF) receive modes. However from PF
+ * driver perspective we need to keep track of filter configurations
+ * for different VFs to prevent filter values dupes
+ */
+ for (i = 0; i < lmac->dmacs_cfg; i++) {
+ if (lmac->dmacs[i].dmac == dmac) {
+ lmac->dmacs[i].vf_map |= BIT_ULL(vf_id);
+ return -1;
+ }
+ }
+
+ if (!(lmac->dmacs_cfg < lmac->dmacs_count))
+ return -1;
+
+ /* keep it for further tracking */
+ lmac->dmacs[lmac->dmacs_cfg].dmac = dmac;
+ lmac->dmacs[lmac->dmacs_cfg].vf_map = BIT_ULL(vf_id);
+ lmac->dmacs_cfg++;
+ return 0;
+}
+
+static int bgx_set_dmac_cam_filter_mac(struct bgx *bgx, int lmacid, u64 cam_dmac, u8 idx)
+{
+ struct lmac *lmac = NULL;
+ u64 cfg = 0;
+
+ /* skip zero addresses as meaningless */
+ if (!cam_dmac || !bgx)
+ return -1;
+
+ lmac = &bgx->lmac[lmacid];
+
+ /* configure DCAM filtering for designated LMAC */
+ cfg = RX_DMACX_CAM_LMACID(lmacid & LMAC_ID_MASK) |
+ RX_DMACX_CAM_EN | cam_dmac;
+ bgx_reg_write(bgx, 0, BGX_CMR_RX_DMACX_CAM + ((lmacid * lmac->dmacs_count) + idx) * sizeof(u64), cfg);
+ return 0;
+}
+
+void bgx_set_dmac_cam_filter(int node, int bgx_idx, int lmacid, u64 cam_dmac, u8 vf_id)
+{
+ struct bgx *bgx = get_bgx(node, bgx_idx);
+ struct lmac *lmac = NULL;
+
+ if (!bgx)
+ return;
+
+ lmac = &bgx->lmac[lmacid];
+
+ if (!cam_dmac)
+ cam_dmac = ether_addr_to_u64(lmac->mac);
+
+ /* since we might have several VFs attached to particular LMAC
+ * and kernel could call mcast config for each of them with the
+ * same MAC, check if requested MAC is already in filtering list and
+ * updare/prepare list of MACs to be applied later to HW filters
+ */
+ bgx_lmac_save_filter(lmac, cam_dmac, vf_id);
+}
+EXPORT_SYMBOL(bgx_set_dmac_cam_filter);
+
+void bgx_set_xcast_mode(int node, int bgx_idx, int lmacid, u8 mode)
+{
+ struct bgx *bgx = get_bgx(node, bgx_idx);
+ struct lmac *lmac = NULL;
+ u64 cfg = 0;
+ u8 i = 0;
+
+ if (!bgx)
+ return;
+
+ lmac = &bgx->lmac[lmacid];
+
+ cfg = bgx_reg_read(bgx, lmacid, BGX_CMRX_RX_DMAC_CTL);
+ if (mode & BGX_XCAST_BCAST_ACCEPT)
+ cfg |= BCAST_ACCEPT;
+ else
+ cfg &= ~BCAST_ACCEPT;
+
+ /* disable all MCASTs and DMAC filtering */
+ cfg &= ~(CAM_ACCEPT | BGX_MCAST_MODE(MCAST_MODE_MASK));
+
+ /* check requested bits and set filtergin mode appropriately */
+ if (mode & (BGX_XCAST_MCAST_ACCEPT)) {
+ cfg |= (BGX_MCAST_MODE(MCAST_MODE_ACCEPT));
+ } else if (mode & BGX_XCAST_MCAST_FILTER) {
+ cfg |= (BGX_MCAST_MODE(MCAST_MODE_CAM_FILTER) | CAM_ACCEPT);
+ for (i = 0; i < lmac->dmacs_cfg; i++)
+ bgx_set_dmac_cam_filter_mac(bgx, lmacid,
+ lmac->dmacs[i].dmac, i);
+ }
+ bgx_reg_write(bgx, lmacid, BGX_CMRX_RX_DMAC_CTL, cfg);
+}
+EXPORT_SYMBOL(bgx_set_xcast_mode);
+
+void bgx_reset_xcast_mode(int node, int bgx_idx, int lmacid, u8 vf_id)
+{
+ struct bgx *bgx = get_bgx(node, bgx_idx);
+
+ if (!bgx)
+ return;
+
+ bgx_lmac_remove_filters(&bgx->lmac[lmacid], vf_id);
+ bgx_flush_dmac_cam_filter(bgx, lmacid);
+ bgx_set_xcast_mode(node, bgx_idx, lmacid,
+ (BGX_XCAST_BCAST_ACCEPT | BGX_XCAST_MCAST_ACCEPT));
+}
+EXPORT_SYMBOL(bgx_reset_xcast_mode);
+
void bgx_lmac_rx_tx_enable(int node, int bgx_idx, int lmacid, bool enable)
{
struct bgx *bgx = get_bgx(node, bgx_idx);
diff --git a/drivers/net/ethernet/cavium/thunder/thunder_bgx.h b/drivers/net/ethernet/cavium/thunder/thunder_bgx.h
index 52439da62c97..f5de44bc3bdb 100644
--- a/drivers/net/ethernet/cavium/thunder/thunder_bgx.h
+++ b/drivers/net/ethernet/cavium/thunder/thunder_bgx.h
@@ -30,6 +30,7 @@
#define DEFAULT_PAUSE_TIME 0xFFFF
#define BGX_ID_MASK 0x3
+#define LMAC_ID_MASK 0x3
#define MAX_DMAC_PER_LMAC_TNS_BYPASS_MODE 2
@@ -205,6 +206,13 @@
#define LMAC_INTR_LINK_UP BIT(0)
#define LMAC_INTR_LINK_DOWN BIT(1)
+#define BGX_XCAST_BCAST_ACCEPT BIT(0)
+#define BGX_XCAST_MCAST_ACCEPT BIT(1)
+#define BGX_XCAST_MCAST_FILTER BIT(2)
+
+void bgx_set_dmac_cam_filter(int node, int bgx_idx, int lmacid, u64 mac, u8 vf);
+void bgx_reset_xcast_mode(int node, int bgx_idx, int lmacid, u8 vf);
+void bgx_set_xcast_mode(int node, int bgx_idx, int lmacid, u8 mode);
void octeon_mdiobus_force_mod_depencency(void);
void bgx_lmac_rx_tx_enable(int node, int bgx_idx, int lmacid, bool enable);
void bgx_add_dmac_addr(u64 dmac, int node, int bgx_idx, int lmac);
--
2.14.3
^ permalink raw reply related
* [PATCH 4/7] net: thunderx: add new messages for handle ndo_set_rx_mode callback
From: Vadim Lomovtsev @ 2018-03-27 15:07 UTC (permalink / raw)
To: sgoutham, sunil.kovvuri, robert.richter, linux-arm-kernel, netdev,
linux-kernel
Cc: dnelson, Vadim Lomovtsev
In-Reply-To: <20180327150736.10718-1-Vadim.Lomovtsev@caviumnetworks.com>
From: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
The kernel calls ndo_set_rx_mode() callback supplying it will all necessary
info, such as device state flags, multicast mac addresses list and so on.
Since we have only 128 bits to communicate with PF we need to initiate
several requests to PF with small/short operation each based on input data.
So this commit implements following PF messages codes along with new
data structures for them:
NIC_MBOX_MSG_RESET_XCAST to flush all filters configured for this
particular network interface (VF)
NIC_MBOX_MSG_ADD_MCAST to add new MAC address to DMAC filter registers
for this particular network interface (VF)
NIC_MBOX_MSG_SET_XCAST to apply filtering configuration to filter control
register
Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
---
drivers/net/ethernet/cavium/thunder/nic.h | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/net/ethernet/cavium/thunder/nic.h b/drivers/net/ethernet/cavium/thunder/nic.h
index 4cacce5d2b16..069289b4f968 100644
--- a/drivers/net/ethernet/cavium/thunder/nic.h
+++ b/drivers/net/ethernet/cavium/thunder/nic.h
@@ -403,6 +403,9 @@ struct nicvf {
#define NIC_MBOX_MSG_PTP_CFG 0x19 /* HW packet timestamp */
#define NIC_MBOX_MSG_CFG_DONE 0xF0 /* VF configuration done */
#define NIC_MBOX_MSG_SHUTDOWN 0xF1 /* VF is being shutdown */
+#define NIC_MBOX_MSG_RESET_XCAST 0xF2 /* Reset DCAM filtering mode */
+#define NIC_MBOX_MSG_ADD_MCAST 0xF3 /* Add MAC to DCAM filters */
+#define NIC_MBOX_MSG_SET_XCAST 0xF4 /* Set MCAST/BCAST RX mode */
struct nic_cfg_msg {
u8 msg;
@@ -556,6 +559,14 @@ struct set_ptp {
bool enable;
};
+struct xcast {
+ u8 msg;
+ union {
+ u8 mode;
+ u64 mac;
+ } data;
+};
+
/* 128 bit shared memory between PF and each VF */
union nic_mbx {
struct { u8 msg; } msg;
@@ -576,6 +587,7 @@ union nic_mbx {
struct reset_stat_cfg reset_stat;
struct pfc pfc;
struct set_ptp ptp;
+ struct xcast xcast;
};
#define NIC_NODE_ID_MASK 0x03
--
2.14.3
^ permalink raw reply related
* Re: [PATCH v3 net 1/5] tcp: feed correct number of pkts acked to cc modules also in recovery
From: Yuchung Cheng @ 2018-03-27 15:06 UTC (permalink / raw)
To: Ilpo Järvinen; +Cc: netdev, Neal Cardwell, Eric Dumazet, Sergei Shtylyov
In-Reply-To: <alpine.DEB.2.20.1803271337340.29120@whs-18.cs.helsinki.fi>
On Tue, Mar 27, 2018 at 7:23 AM, Ilpo Järvinen
<ilpo.jarvinen@helsinki.fi> wrote:
> On Mon, 26 Mar 2018, Yuchung Cheng wrote:
>
>> On Tue, Mar 13, 2018 at 3:25 AM, Ilpo Järvinen
>> <ilpo.jarvinen@helsinki.fi> wrote:
>> >
>> > A miscalculation for the number of acknowledged packets occurs during
>> > RTO recovery whenever SACK is not enabled and a cumulative ACK covers
>> > any non-retransmitted skbs. The reason is that pkts_acked value
>> > calculated in tcp_clean_rtx_queue is not correct for slow start after
>> > RTO as it may include segments that were not lost and therefore did
>> > not need retransmissions in the slow start following the RTO. Then
>> > tcp_slow_start will add the excess into cwnd bloating it and
>> > triggering a burst.
>> >
>> > Instead, we want to pass only the number of retransmitted segments
>> > that were covered by the cumulative ACK (and potentially newly sent
>> > data segments too if the cumulative ACK covers that far).
>> >
>> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
>> > ---
>> > net/ipv4/tcp_input.c | 16 +++++++++++++++-
>> > 1 file changed, 15 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
>> > index 9a1b3c1..4a26c09 100644
>> > --- a/net/ipv4/tcp_input.c
>> > +++ b/net/ipv4/tcp_input.c
>> > @@ -3027,6 +3027,8 @@ static int tcp_clean_rtx_queue(struct sock *sk, u32 prior_fack,
>> > long seq_rtt_us = -1L;
>> > long ca_rtt_us = -1L;
>> > u32 pkts_acked = 0;
>> > + u32 rexmit_acked = 0;
>> > + u32 newdata_acked = 0;
>> > u32 last_in_flight = 0;
>> > bool rtt_update;
>> > int flag = 0;
>> > @@ -3056,8 +3058,10 @@ static int tcp_clean_rtx_queue(struct sock *sk, u32 prior_fack,
>> > }
>> >
>> > if (unlikely(sacked & TCPCB_RETRANS)) {
>> > - if (sacked & TCPCB_SACKED_RETRANS)
>> > + if (sacked & TCPCB_SACKED_RETRANS) {
>> > tp->retrans_out -= acked_pcount;
>> > + rexmit_acked += acked_pcount;
>> > + }
>> > flag |= FLAG_RETRANS_DATA_ACKED;
>> > } else if (!(sacked & TCPCB_SACKED_ACKED)) {
>> > last_ackt = skb->skb_mstamp;
>> > @@ -3070,6 +3074,8 @@ static int tcp_clean_rtx_queue(struct sock *sk, u32 prior_fack,
>> > reord = start_seq;
>> > if (!after(scb->end_seq, tp->high_seq))
>> > flag |= FLAG_ORIG_SACK_ACKED;
>> > + else
>> > + newdata_acked += acked_pcount;
>> > }
>> >
>> > if (sacked & TCPCB_SACKED_ACKED) {
>> > @@ -3151,6 +3157,14 @@ static int tcp_clean_rtx_queue(struct sock *sk, u32 prior_fack,
>> > }
>> >
>> > if (tcp_is_reno(tp)) {
>> > + /* Due to discontinuity on RTO in the artificial
>> > + * sacked_out calculations, TCP must restrict
>> > + * pkts_acked without SACK to rexmits and new data
>> > + * segments
>> > + */
>> > + if (icsk->icsk_ca_state == TCP_CA_Loss)
>> > + pkts_acked = rexmit_acked + newdata_acked;
>> > +
>> My understanding is there are two problems
>>
>> 1) your fix: the reordering logic in tcp-remove_reno_sacks requires
>> precise cumulatively acked count, not newly acked count?
>
> While I'm not entirely sure if you intented to say that my fix is broken
> or not, I thought this very difference alot while making the fix and I
> believe that this fix is needed because of the discontinuity at RTO
> (sacked_out is cleared as we set L-bits + lost_out). This is an artifact
> in the imitation of sacked_out for non-SACK but at RTO we can't keep that
> in sync because we set L-bits (and have no S-bits to guide us). Thus, we
> cannot anymore "use" those skbs with only L-bit for the reno_sacks logic.
>
> In tcp_remove_reno_sacks acked - sacked_out is being used to calculate
> tp->delivered, using plain cumulative acked causes congestion control
> breakage later as call to tcp_cong_control will directly use the
> difference in tp->delivered.
>
> This boils down the exact definition of tp->delivered (the one given in
> the header is not detailed enough). I guess you might have better idea
> what it exactly is since one of you has added it? There are subtle things
> in the defination that can make it entirely unsuitable for cc decisions.
> Should those segments that we (possibly) already counted into
> tp->delivered during (potentially preceeding) CA_Recovery be added to it
> for _second time_ or not? This fix avoids such double counting (the
Where is the double counting, assuming normal DUPACK behavior?
In the non-sack case:
1. upon receiving a DUPACK, we assume one packet has been delivered by
incrementing tp->delivered in tcp_add_reno_sack()
2. upon receiving a partial ACK or an ACK that acks recovery point
(high_seq), tp->delivered is incremented by (cumulatively acked -
#dupacks) in tcp_remove_reno_sacks()
therefore tp->delivered is tracking the # of packets delivered
(sacked, acked, DUPACK'd) with the most information it could have
inferred.
>From congestion control's perspective, it cares about the delivery
information (e.g. how much), not the sequences (what or how).
I am pointing out that
1) your fix may fix one corner CC packet accounting issue in the
non-SACK and CA_Loss
2) but it does not fix the other (major) CC packet accounting issue in
the SACK case
3) it breaks the dynamic dupthresh / reordering in the non-SACK case
as tcp_check_reno_reordering requires strictly cum. ack.
Therefore I prefer
a) using tp->delivered to fix (1)(2)
b) perhaps improving tp->delivered SACK emulation code in the non-SACK case
> price is that we might underestimate). For SACK+ACK losses, the similar
> question is: Should those segments that we missed counting into
> tp->delivered during preceeding CA_Recovery (due to losing enough SACKs)
> be added into tp->delivered now during RTO recovery or not (I'm not
> proposing we fix this unless we want to fix the both issues at the same
> time here as its impact with SACK is not that significant)? Is
> tp->delivered supposed to under- or overestimate (in the cases we're not
> sure what/when something happened)? ...If it's overestimating under any
> circumstances (for the current ACK), we cannot base our cc decision on it.
>
> tcp_check_reno_reordering might like to have the cumulatively acked count
> but due to the forementioned discontinuity we anyway cannot accurately
> provide in CA_Loss what tcp_limit_reno_sacked expects (and the
> other CA states are unaffected by this fix). While we could call
> tcp_check_reno_reordering directly from tcp_clean_rtx_queue, it wouldn't
> remove the fundamental discontinuity problem that we have for what
> tcp_limit_reno_sacked assumes about sacked_out. It might actually be so
> that tcp_limit_reno_sacked is just never going to work after we zero
> tp->sacked_out (this would actually be the problem #3) or at least ends
> up underestimating the reordering degree by the amount we cleared from
> sacked_out at RTO?
>
>> 2) current code: pkts_acked can substantially over-estimate the newly
>> delivered pkts in both SACK and non-SACK cases. For example, let's say
>> 99/100 packets are already sacked, and the next ACK acks 100 pkts.
>> pkts_acked == 100 but really only one packet is delivered. It's wrong
>> to inform congestion control that 100 packets have just delivered.
>> AFAICT, the CCs that have pkts_acked callbacks all treat pkts_acked as
>> the newly delivered packets.
>>
>> A better fix for both SACK and non-SACK, seems to be moving
>> ca_ops->pkts_acked into tcp_cong_control, where the "acked_sacked" is
>> calibrated? this is what BBR is currently doing to avoid these pitfalls.
>
> Unfortunately that would not fix the problematic calculation of
> tp->delivered.
>
> But you might be right that there's a problem #2 (it's hard to notice
> for real though as most of the cc modules don't seem to use it for
> anything). However, this is for pkts_acked so are you sure what the
> cc modules exactly expect: the shrinkage in # of outstanding packets or
> the number of newly delivered packets?
>
> Looking (only) quickly into the modules (that use it other than in
> CA_Open):
> - Somewhat suspicious delayed ACK detection logic in cdg
> - HTCP might want shrinkage in # of outstanding packets (but I'm not
> entirely sure) for throughput measurement
> - I guess TCP illinois is broken (assumes it's newly delivered packets)
> but it would not need to use it at all as it just proxies the value in
> a local variable into tcp_illinois_cong_avoid that has the correct acked
> readily available.
>
> There are separate cong_avoid and cong_control callbacks that are more
> obviously oriented for doing cc where as I'd think pkts_acked callback
> seems more oriented to for RTT-sample related operations. Therefore, I'm
> not entirely sure I this is what is wanted for pkts_acked, especially
> given the HTCP example.
>
>
> --
> i.
^ permalink raw reply
* [PATCH 5/7] net: thunderx: add XCAST messages handlers for PF
From: Vadim Lomovtsev @ 2018-03-27 15:07 UTC (permalink / raw)
To: sgoutham, sunil.kovvuri, robert.richter, linux-arm-kernel, netdev,
linux-kernel
Cc: dnelson, Vadim Lomovtsev
In-Reply-To: <20180327150736.10718-1-Vadim.Lomovtsev@caviumnetworks.com>
From: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
This commit is to add message handling for ndo_set_rx_mode()
callback at PF side.
Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
---
drivers/net/ethernet/cavium/thunder/nic_main.c | 45 +++++++++++++++++++++++---
1 file changed, 41 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/cavium/thunder/nic_main.c b/drivers/net/ethernet/cavium/thunder/nic_main.c
index 7ff66a8194e2..55af04fa03a7 100644
--- a/drivers/net/ethernet/cavium/thunder/nic_main.c
+++ b/drivers/net/ethernet/cavium/thunder/nic_main.c
@@ -21,6 +21,8 @@
#define DRV_NAME "nicpf"
#define DRV_VERSION "1.0"
+#define NIC_VF_PER_MBX_REG 64
+
struct hw_info {
u8 bgx_cnt;
u8 chans_per_lmac;
@@ -1072,6 +1074,40 @@ static void nic_handle_mbx_intr(struct nicpf *nic, int vf)
case NIC_MBOX_MSG_PTP_CFG:
nic_config_timestamp(nic, vf, &mbx.ptp);
break;
+ case NIC_MBOX_MSG_RESET_XCAST:
+ if (vf >= nic->num_vf_en) {
+ ret = -1; /* NACK */
+ break;
+ }
+ bgx = NIC_GET_BGX_FROM_VF_LMAC_MAP(nic->vf_lmac_map[vf]);
+ lmac = NIC_GET_LMAC_FROM_VF_LMAC_MAP(nic->vf_lmac_map[vf]);
+ bgx_reset_xcast_mode(nic->node, bgx, lmac,
+ vf < NIC_VF_PER_MBX_REG ? vf :
+ vf - NIC_VF_PER_MBX_REG);
+ break;
+
+ case NIC_MBOX_MSG_ADD_MCAST:
+ if (vf >= nic->num_vf_en) {
+ ret = -1; /* NACK */
+ break;
+ }
+ bgx = NIC_GET_BGX_FROM_VF_LMAC_MAP(nic->vf_lmac_map[vf]);
+ lmac = NIC_GET_LMAC_FROM_VF_LMAC_MAP(nic->vf_lmac_map[vf]);
+ bgx_set_dmac_cam_filter(nic->node, bgx, lmac,
+ mbx.xcast.data.mac,
+ vf < NIC_VF_PER_MBX_REG ? vf :
+ vf - NIC_VF_PER_MBX_REG);
+ break;
+
+ case NIC_MBOX_MSG_SET_XCAST:
+ if (vf >= nic->num_vf_en) {
+ ret = -1; /* NACK */
+ break;
+ }
+ bgx = NIC_GET_BGX_FROM_VF_LMAC_MAP(nic->vf_lmac_map[vf]);
+ lmac = NIC_GET_LMAC_FROM_VF_LMAC_MAP(nic->vf_lmac_map[vf]);
+ bgx_set_xcast_mode(nic->node, bgx, lmac, mbx.xcast.data.mode);
+ break;
default:
dev_err(&nic->pdev->dev,
"Invalid msg from VF%d, msg 0x%x\n", vf, mbx.msg.msg);
@@ -1094,7 +1130,7 @@ static irqreturn_t nic_mbx_intr_handler(int irq, void *nic_irq)
struct nicpf *nic = (struct nicpf *)nic_irq;
int mbx;
u64 intr;
- u8 vf, vf_per_mbx_reg = 64;
+ u8 vf;
if (irq == pci_irq_vector(nic->pdev, NIC_PF_INTR_ID_MBOX0))
mbx = 0;
@@ -1103,12 +1139,13 @@ static irqreturn_t nic_mbx_intr_handler(int irq, void *nic_irq)
intr = nic_reg_read(nic, NIC_PF_MAILBOX_INT + (mbx << 3));
dev_dbg(&nic->pdev->dev, "PF interrupt Mbox%d 0x%llx\n", mbx, intr);
- for (vf = 0; vf < vf_per_mbx_reg; vf++) {
+ for (vf = 0; vf < NIC_VF_PER_MBX_REG; vf++) {
if (intr & (1ULL << vf)) {
dev_dbg(&nic->pdev->dev, "Intr from VF %d\n",
- vf + (mbx * vf_per_mbx_reg));
+ vf + (mbx * NIC_VF_PER_MBX_REG));
- nic_handle_mbx_intr(nic, vf + (mbx * vf_per_mbx_reg));
+ nic_handle_mbx_intr(nic, vf +
+ (mbx * NIC_VF_PER_MBX_REG));
nic_clear_mbx_intr(nic, vf, mbx);
}
}
--
2.14.3
^ permalink raw reply related
* [PATCH 6/7] net: thunderx: add workqueue control structures for handle ndo_set_rx_mode request
From: Vadim Lomovtsev @ 2018-03-27 15:07 UTC (permalink / raw)
To: sgoutham, sunil.kovvuri, robert.richter, linux-arm-kernel, netdev,
linux-kernel
Cc: dnelson, Vadim Lomovtsev
In-Reply-To: <20180327150736.10718-1-Vadim.Lomovtsev@caviumnetworks.com>
From: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
The kernel calls ndo_set_rx_mode() callback from atomic context which
causes messaging timeouts between VF and PF (as they’re implemented
via MSIx). So in order to handle ndo_set_rx_mode() we need to get rid of it.
This commit implements necessary workqueue related structures to let VF
queue kernel request processing in non-atomic context later.
Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
---
drivers/net/ethernet/cavium/thunder/nic.h | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/drivers/net/ethernet/cavium/thunder/nic.h b/drivers/net/ethernet/cavium/thunder/nic.h
index 069289b4f968..5fc46c5a4f36 100644
--- a/drivers/net/ethernet/cavium/thunder/nic.h
+++ b/drivers/net/ethernet/cavium/thunder/nic.h
@@ -265,6 +265,22 @@ struct nicvf_drv_stats {
struct cavium_ptp;
+struct xcast_addr {
+ struct list_head list;
+ u64 addr;
+};
+
+struct xcast_addr_list {
+ struct list_head list;
+ int count;
+};
+
+struct nicvf_work {
+ struct delayed_work work;
+ u8 mode;
+ struct xcast_addr_list *mc;
+};
+
struct nicvf {
struct nicvf *pnicvf;
struct net_device *netdev;
@@ -313,6 +329,7 @@ struct nicvf {
struct nicvf_pfc pfc;
struct tasklet_struct qs_err_task;
struct work_struct reset_task;
+ struct nicvf_work rx_mode_work;
/* PTP timestamp */
struct cavium_ptp *ptp_clock;
--
2.14.3
^ permalink raw reply related
* [PATCH 7/7] net: thunderx: add ndo_set_rx_mode callback implementation for VF
From: Vadim Lomovtsev @ 2018-03-27 15:07 UTC (permalink / raw)
To: sgoutham, sunil.kovvuri, robert.richter, linux-arm-kernel, netdev,
linux-kernel
Cc: dnelson, Vadim Lomovtsev
In-Reply-To: <20180327150736.10718-1-Vadim.Lomovtsev@caviumnetworks.com>
From: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
The ndo_set_rx_mode() is called from atomic context which causes
messages response timeouts while VF to PF communication via MSIx.
To get rid of that we're copy passed mc list, parse flags and queue
handling of kernel request to ordered workqueue.
Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
---
drivers/net/ethernet/cavium/thunder/nicvf_main.c | 108 ++++++++++++++++++++++-
1 file changed, 107 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
index 7d9c5ffbd041..0d4bec927623 100644
--- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
+++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
@@ -21,6 +21,7 @@
#include <linux/bpf_trace.h>
#include <linux/filter.h>
#include <linux/net_tstamp.h>
+#include <linux/workqueue.h>
#include "nic_reg.h"
#include "nic.h"
@@ -67,6 +68,9 @@ module_param(cpi_alg, int, S_IRUGO);
MODULE_PARM_DESC(cpi_alg,
"PFC algorithm (0=none, 1=VLAN, 2=VLAN16, 3=IP Diffserv)");
+/* workqueue for handling kernel ndo_set_rx_mode() calls */
+static struct workqueue_struct *nicvf_rx_mode_wq;
+
static inline u8 nicvf_netdev_qidx(struct nicvf *nic, u8 qidx)
{
if (nic->sqs_mode)
@@ -1919,6 +1923,98 @@ static int nicvf_ioctl(struct net_device *netdev, struct ifreq *req, int cmd)
}
}
+static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
+{
+ struct nicvf_work *vf_work = container_of(work_arg, struct nicvf_work,
+ work.work);
+ struct nicvf *nic = container_of(vf_work, struct nicvf, rx_mode_work);
+ union nic_mbx mbx = {};
+ struct xcast_addr *xaddr, *next;
+
+ if (!vf_work)
+ return;
+
+ /* From the inside of VM code flow we have only 128 bits memory
+ * available to send message to host's PF, so send all mc addrs
+ * one by one, starting from flush command in case if kernel
+ * requests to configure specific MAC filtering
+ */
+
+ /* flush DMAC filters and reset RX mode */
+ mbx.xcast.msg = NIC_MBOX_MSG_RESET_XCAST;
+ nicvf_send_msg_to_pf(nic, &mbx);
+
+ if (vf_work->mode & BGX_XCAST_MCAST_FILTER) {
+ /* once enabling filtering, we need to signal to PF to add
+ * its' own LMAC to the filter to accept packets for it.
+ */
+ mbx.xcast.msg = NIC_MBOX_MSG_ADD_MCAST;
+ mbx.xcast.data.mac = 0;
+ nicvf_send_msg_to_pf(nic, &mbx);
+ }
+
+ /* check if we have any specific MACs to be added to PF DMAC filter */
+ if (vf_work->mc) {
+ /* now go through kernel list of MACs and add them one by one */
+ list_for_each_entry_safe(xaddr, next, &vf_work->mc->list, list) {
+ mbx.xcast.msg = NIC_MBOX_MSG_ADD_MCAST;
+ mbx.xcast.data.mac = xaddr->addr;
+ nicvf_send_msg_to_pf(nic, &mbx);
+
+ /* after receiving ACK from PF release memory */
+ list_del(&xaddr->list);
+ kfree(xaddr);
+ vf_work->mc->count--;
+ }
+ kfree(vf_work->mc);
+ }
+
+ /* and finally set rx mode for PF accordingly */
+ mbx.xcast.msg = NIC_MBOX_MSG_SET_XCAST;
+ mbx.xcast.data.mode = vf_work->mode;
+
+ nicvf_send_msg_to_pf(nic, &mbx);
+}
+
+static void nicvf_set_rx_mode(struct net_device *netdev)
+{
+ struct nicvf *nic = netdev_priv(netdev);
+ struct netdev_hw_addr *ha;
+ struct xcast_addr_list *mc_list = NULL;
+ u8 mode = 0;
+
+ if (netdev->flags & IFF_PROMISC) {
+ mode = BGX_XCAST_BCAST_ACCEPT | BGX_XCAST_MCAST_ACCEPT;
+ } else {
+ if (netdev->flags & IFF_BROADCAST)
+ mode |= BGX_XCAST_BCAST_ACCEPT;
+
+ if (netdev->flags & IFF_ALLMULTI) {
+ mode |= BGX_XCAST_MCAST_ACCEPT;
+ } else if (netdev->flags & IFF_MULTICAST) {
+ mode |= BGX_XCAST_MCAST_FILTER;
+ /* here we need to copy mc addrs */
+ if (netdev_mc_count(netdev)) {
+ struct xcast_addr *xaddr;
+
+ mc_list = kmalloc(sizeof(*mc_list), GFP_ATOMIC);
+ INIT_LIST_HEAD(&mc_list->list);
+ netdev_hw_addr_list_for_each(ha, &netdev->mc) {
+ xaddr = kmalloc(sizeof(*xaddr),
+ GFP_ATOMIC);
+ xaddr->addr = ether_addr_to_u64(ha->addr);
+ list_add_tail(&xaddr->list,
+ &mc_list->list);
+ mc_list->count++;
+ }
+ }
+ }
+ }
+ nic->rx_mode_work.mc = mc_list;
+ nic->rx_mode_work.mode = mode;
+ queue_delayed_work(nicvf_rx_mode_wq, &nic->rx_mode_work.work, 2 * HZ);
+}
+
static const struct net_device_ops nicvf_netdev_ops = {
.ndo_open = nicvf_open,
.ndo_stop = nicvf_stop,
@@ -1931,6 +2027,7 @@ static const struct net_device_ops nicvf_netdev_ops = {
.ndo_set_features = nicvf_set_features,
.ndo_bpf = nicvf_xdp,
.ndo_do_ioctl = nicvf_ioctl,
+ .ndo_set_rx_mode = nicvf_set_rx_mode,
};
static int nicvf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
@@ -2071,6 +2168,8 @@ static int nicvf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
INIT_WORK(&nic->reset_task, nicvf_reset_task);
+ INIT_DELAYED_WORK(&nic->rx_mode_work.work, nicvf_set_rx_mode_task);
+
err = register_netdev(netdev);
if (err) {
dev_err(dev, "Failed to register netdevice\n");
@@ -2109,6 +2208,8 @@ static void nicvf_remove(struct pci_dev *pdev)
nic = netdev_priv(netdev);
pnetdev = nic->pnicvf->netdev;
+ cancel_delayed_work_sync(&nic->rx_mode_work.work);
+
/* Check if this Qset is assigned to different VF.
* If yes, clean primary and all secondary Qsets.
*/
@@ -2140,12 +2241,17 @@ static struct pci_driver nicvf_driver = {
static int __init nicvf_init_module(void)
{
pr_info("%s, ver %s\n", DRV_NAME, DRV_VERSION);
-
+ nicvf_rx_mode_wq = alloc_ordered_workqueue("nicvf_generic",
+ WQ_MEM_RECLAIM);
return pci_register_driver(&nicvf_driver);
}
static void __exit nicvf_cleanup_module(void)
{
+ if (nicvf_rx_mode_wq) {
+ destroy_workqueue(nicvf_rx_mode_wq);
+ nicvf_rx_mode_wq = NULL;
+ }
pci_unregister_driver(&nicvf_driver);
}
--
2.14.3
^ permalink raw reply related
* [PATCH 1/7] net: thunderx: move filter register related macro into proper place
From: Vadim Lomovtsev @ 2018-03-27 15:07 UTC (permalink / raw)
To: sgoutham, sunil.kovvuri, robert.richter, linux-arm-kernel, netdev,
linux-kernel
Cc: dnelson, Vadim Lomovtsev
In-Reply-To: <20180327150736.10718-1-Vadim.Lomovtsev@caviumnetworks.com>
From: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
The ThunderX NIC has set of registers which allows to configure
filter policy for ingress packets. There are three possible regimes
of filtering multicasts, broadcasts and unicasts: accept all, reject all
and accept filter allowed only.
Current implementation has enum with all of them and two generic macro
for enabling filtering et all (CAM_ACCEPT) and enabling/disabling
broadcast packets, which also should be corrected in order to represent
register bits properly. All these values are private for driver and
there is no need to ‘publish’ them via header file.
This commit is to move filtering register manipulation values from
header file into source with explicit assignment of exact register
values to them to be used while register configuring.
Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
---
drivers/net/ethernet/cavium/thunder/thunder_bgx.c | 13 +++++++++++++
drivers/net/ethernet/cavium/thunder/thunder_bgx.h | 11 -----------
2 files changed, 13 insertions(+), 11 deletions(-)
diff --git a/drivers/net/ethernet/cavium/thunder/thunder_bgx.c b/drivers/net/ethernet/cavium/thunder/thunder_bgx.c
index 91d34ea40e2c..0dd211605eb1 100644
--- a/drivers/net/ethernet/cavium/thunder/thunder_bgx.c
+++ b/drivers/net/ethernet/cavium/thunder/thunder_bgx.c
@@ -24,6 +24,19 @@
#define DRV_NAME "thunder_bgx"
#define DRV_VERSION "1.0"
+/* RX_DMAC_CTL configuration */
+enum MCAST_MODE {
+ MCAST_MODE_REJECT = 0x0,
+ MCAST_MODE_ACCEPT = 0x1,
+ MCAST_MODE_CAM_FILTER = 0x2,
+ RSVD = 0x3
+};
+
+#define BCAST_ACCEPT BIT(0)
+#define CAM_ACCEPT BIT(3)
+#define MCAST_MODE_MASK 0x3
+#define BGX_MCAST_MODE(x) (x << 1)
+
struct lmac {
struct bgx *bgx;
int dmac;
diff --git a/drivers/net/ethernet/cavium/thunder/thunder_bgx.h b/drivers/net/ethernet/cavium/thunder/thunder_bgx.h
index 5a7567d31138..52439da62c97 100644
--- a/drivers/net/ethernet/cavium/thunder/thunder_bgx.h
+++ b/drivers/net/ethernet/cavium/thunder/thunder_bgx.h
@@ -205,17 +205,6 @@
#define LMAC_INTR_LINK_UP BIT(0)
#define LMAC_INTR_LINK_DOWN BIT(1)
-/* RX_DMAC_CTL configuration*/
-enum MCAST_MODE {
- MCAST_MODE_REJECT,
- MCAST_MODE_ACCEPT,
- MCAST_MODE_CAM_FILTER,
- RSVD
-};
-
-#define BCAST_ACCEPT 1
-#define CAM_ACCEPT 1
-
void octeon_mdiobus_force_mod_depencency(void);
void bgx_lmac_rx_tx_enable(int node, int bgx_idx, int lmacid, bool enable);
void bgx_add_dmac_addr(u64 dmac, int node, int bgx_idx, int lmac);
--
2.14.3
^ permalink raw reply related
* Re: RFC on writel and writel_relaxed
From: Will Deacon @ 2018-03-27 15:10 UTC (permalink / raw)
To: Sinan Kaya
Cc: Benjamin Herrenschmidt, Arnd Bergmann, Jason Gunthorpe,
David Laight, Oliver,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
linux-rdma@vger.kernel.org, Alexander Duyck, Paul E. McKenney,
netdev@vger.kernel.org, Alexander Duyck, torvalds
In-Reply-To: <1e077f6a-90b6-cce9-6f0f-a8c003fec850@codeaurora.org>
Hi Alex,
On Tue, Mar 27, 2018 at 10:46:58AM -0400, Sinan Kaya wrote:
> +netdev, +Alex
>
> On 3/26/2018 6:00 PM, Benjamin Herrenschmidt wrote:
> > On Mon, 2018-03-26 at 23:30 +0200, Arnd Bergmann wrote:
> >> Most of the drivers have a unwound loop with writeq() or something to
> >>> do it.
> >>
> >> But isn't the writeq() barrier much more expensive than anything you'd
> >> do in function calls?
> >
> > It is for us, and will break any write combining.
> >
> >>>>> The same document says that _relaxed() does not give that guarentee.
> >>>>>
> >>>>> The lwn articule on this went into some depth on the interaction with
> >>>>> spinlocks.
> >>>>>
> >>>>> As far as I can see, containment in a spinlock seems to be the only
> >>>>> different between writel and writel_relaxed..
> >>>>
> >>>> I was always puzzled by this: The intention of _relaxed() on ARM
> >>>> (where it originates) was to skip the barrier that serializes DMA
> >>>> with MMIO, not to skip the serialization between MMIO and locks.
> >>>
> >>> But that was never a requirement of writel(),
> >>> Documentation/memory-barriers.txt gives an explicit example demanding
> >>> the wmb() before writel() for ordering system memory against writel.
> >
> > This is a bug in the documentation.
> >
> >> Indeed, but it's in an example for when to use dma_wmb(), not wmb().
> >> Adding Alexander Duyck to Cc, he added that section as part of
> >> 1077fa36f23e ("arch: Add lightweight memory barriers dma_rmb() and
> >> dma_wmb()"). Also adding the other people that were involved with that.
> >
> > Linus himself made it very clear years ago. readl and writel have to
> > order vs memory accesses.
> >
> >>> I actually have no idea why ARM had that barrier, I always assumed it
> >>> was to give program ordering to the accesses and that _relaxed allowed
> >>> re-ordering (the usual meaning of relaxed)..
> >>>
> >>> But the barrier document makes it pretty clear that the only
> >>> difference between the two is spinlock containment, and WillD wrote
> >>> this text, so I belive it is accurate for ARM.
> >>>
> >>> Very confusing.
> >>
> >> It does mention serialization with both DMA and locks in the
> >> section about readX_relaxed()/writeX_relaxed(). The part
> >> about DMA is very clear here, and I must have just forgotten
> >> the exact semantics with regards to spinlocks. I'm still not
> >> sure what prevents a writel() from leaking out the end of a
> >> spinlock section that doesn't happen with writel_relaxed(), since
> >> the barrier in writel() comes before the access, and the
> >> spin_unlock() shouldn't affect the external buses.
> >
> > So...
> >
> > Historically, what happened is that we (we means whoever participated
> > in the discussion on the list with Linus calling the shots really)
> > decided that there was no sane way for drivers to understand a world
> > where readl/writel didn't fully order things vs. memory accesses (ie,
> > DMA).
> >
> > So it should always be correct to do:
> >
> > - Write to some in-memory buffer
> > - writel() to kick the DMA read of that buffer
> >
> > without any extra barrier.
> >
> > The spinlock situation however got murky. Mostly that came up because
> > on architecture (I forgot who, might have been ia64) has a hard time
> > providing that consistency without making writel insanely expensive.
> >
> > Thus they created mmiowb whose main purpose was precisely to order
> > writel with a following spin_unlock.
> >
> > I decided not to go down that path on power because getting all drivers
> > "fixed" to do the right thing was going to be a losing battle, and
> > instead added per-cpu tracking of writel in order to "escalate" to a
> > heavier barrier in spin_unlock itself when necessary.
> >
> > Now, all this happened more than a decade ago and it's possible that
> > the understanding or expectations "shifted" over time...
>
> Alex is raising concerns on the netdev list.
>
> Sinan
> "We are being told that if you use writel(), then you don't need a wmb() on
> all architectures."
>
> Alex:
> "I'm not sure who told you that but that is incorrect, at least for
> x86. If you attempt to use writel() without the wmb() we will have to
> NAK the patches. We will accept the wmb() with writel_releaxed() since
> that solves things for ARM."
>
> > Jason is seeking behavior clarification for write combined buffers.
>
> Alex:
> "Don't bother. I can tell you right now that for x86 you have to have a
> wmb() before the writel().
To clarify: are you saying that on x86 you need a wmb() prior to a writel
if you want that writel to be ordered after prior writes to memory? Is this
specific to WC memory or some other non-standard attribute?
The only reason we have wmb() inside writel() on arm, arm64 and power is for
parity with x86 because Linus (CC'd) wanted architectures to order I/O vs
memory by default so that it was easier to write portable drivers. The
performance impact of that implicit barrier is non-trivial, but we want the
driver portability and I went as far as adding generic _relaxed versions for
the cases where ordering isn't required. You seem to be suggesting that none
of this is necessary and drivers would already run into problems on x86 if
they didn't use wmb() explicitly in conjunction with writel, which I find
hard to believe and is in direct contradiction with the current Linux I/O
memory model (modulo the broken example in the dma_*mb section of
memory-barriers.txt).
Has something changed?
Will
^ permalink raw reply
* Re: [PATCH v2 iproute2-next 3/6] rdma: Add CM_ID resource tracking information
From: Leon Romanovsky @ 2018-03-27 15:15 UTC (permalink / raw)
To: Jason Gunthorpe; +Cc: Steve Wise, David Ahern, stephen, netdev, linux-rdma
In-Reply-To: <20180327144455.GG12318@ziepe.ca>
[-- Attachment #1: Type: text/plain, Size: 2945 bytes --]
On Tue, Mar 27, 2018 at 08:44:55AM -0600, Jason Gunthorpe wrote:
> On Tue, Mar 27, 2018 at 06:21:41AM +0300, Leon Romanovsky wrote:
> > On Mon, Mar 26, 2018 at 04:30:33PM -0600, Jason Gunthorpe wrote:
> > > On Mon, Mar 26, 2018 at 04:34:44PM -0500, Steve Wise wrote:
> > > >
> > > > On 3/26/2018 4:15 PM, Jason Gunthorpe wrote:
> > > > > On Mon, Mar 26, 2018 at 09:30:41AM -0500, Steve Wise wrote:
> > > > >>
> > > > >> On 3/26/2018 9:17 AM, David Ahern wrote:
> > > > >>> On 2/27/18 9:07 AM, Steve Wise wrote:
> > > > >>>> diff --git a/rdma/rdma.h b/rdma/rdma.h
> > > > >>>> index 5809f70..e55205b 100644
> > > > >>>> +++ b/rdma/rdma.h
> > > > >>>> @@ -18,10 +18,12 @@
> > > > >>>> #include <libmnl/libmnl.h>
> > > > >>>> #include <rdma/rdma_netlink.h>
> > > > >>>> #include <time.h>
> > > > >>>> +#include <net/if_arp.h>
> > > > >>>>
> > > > >>>> #include "list.h"
> > > > >>>> #include "utils.h"
> > > > >>>> #include "json_writer.h"
> > > > >>>> +#include <rdma/rdma_cma.h>
> > > > >>>>
> > > > >>> did you forget to add rdma_cma.h? I don't see that file in my repo.
> > > > >> It is provided by the rdma-core package, upon which rdma tool now
> > > > >> depends for the rdma_port_space enum.
> > > > > It is a kernel bug that enum is not in an include/uapi/rdma header
> > > > >
> > > > > Fix it there and don't try to use rdma-core headers to get kernel ABI.
> > > > >
> > > > > Jason
> > > >
> > > > I wish you'd commented on this just a little sooner. I just resent v3
> > > > of this series... with rdma_cma.h included. :)
> > > >
> > > > How about the restrack/nldev code just translates the port space from
> > > > enum rdma_port_space to a new ABI enum, say nldev_rdma_port_space, that
> > > > i add to rdma_netlink.h? I'd hate to open the can of worms of trying to
> > > > split rdma_cma.h into uabi and no uabi headers. :(
> > >
> > > If port space is already part of the ABI there isn't much reason to
> > > translate it.
> > >
> > > You just need to pick the right header to put it in, since it is a verbs
> > > define it doesn't belong in the netlink header.
> >
> > I completely understand Steve's concerns.
> >
> > I tried to do such thing (expose kernel headers) in first incarnation of
> > rdmatool with attempt to clean IB/core as well to ensure that we won't expose
> > anything that is not implemented. It didn't go well.
>
> rdma-core is now using the kernel uapi/ headers natively, seems to be
> going OK. What problem did you face?
I didn't agree to move to UAPI defines which are not implemented and not
used in the kernel, so I sent small number of patches similar to those [1, 2].
Those patches were rejected.
So please don't mix Steve's need to use 3 defines with very large and
painful task to expose proper UAPIs.
Thanks
[1] https://patchwork.kernel.org/patch/9901319/
[2] https://patchwork.kernel.org/patch/9901317/
>
> Jason
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v6 bpf-next 06/11] tracepoint: compute num_args at build time
From: Steven Rostedt @ 2018-03-27 15:15 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: davem, daniel, torvalds, peterz, netdev, kernel-team, linux-api,
Mathieu Desnoyers
In-Reply-To: <20180327024706.2064725-7-ast@fb.com>
On Mon, 26 Mar 2018 19:47:01 -0700
Alexei Starovoitov <ast@fb.com> wrote:
> From: Alexei Starovoitov <ast@kernel.org>
>
> compute number of arguments passed into tracepoint
> at compile time and store it as part of 'struct tracepoint'.
> The number is necessary to check safety of bpf program access that
> is coming in subsequent patch.
>
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
-- Steve
> ---
> include/linux/tracepoint-defs.h | 1 +
> include/linux/tracepoint.h | 12 ++++++------
> include/trace/define_trace.h | 14 +++++++-------
> 3 files changed, 14 insertions(+), 13 deletions(-)
>
> diff --git a/include/linux/tracepoint-defs.h b/include/linux/tracepoint-defs.h
> index 64ed7064f1fa..39a283c61c51 100644
> --- a/include/linux/tracepoint-defs.h
> +++ b/include/linux/tracepoint-defs.h
> @@ -33,6 +33,7 @@ struct tracepoint {
> int (*regfunc)(void);
> void (*unregfunc)(void);
> struct tracepoint_func __rcu *funcs;
> + u32 num_args;
> };
>
> #endif
> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> index c94f466d57ef..c92f4adbc0d7 100644
> --- a/include/linux/tracepoint.h
> +++ b/include/linux/tracepoint.h
> @@ -230,18 +230,18 @@ extern void syscall_unregfunc(void);
> * structures, so we create an array of pointers that will be used for iteration
> * on the tracepoints.
> */
> -#define DEFINE_TRACE_FN(name, reg, unreg) \
> +#define DEFINE_TRACE_FN(name, reg, unreg, num_args) \
> static const char __tpstrtab_##name[] \
> __attribute__((section("__tracepoints_strings"))) = #name; \
> struct tracepoint __tracepoint_##name \
> __attribute__((section("__tracepoints"))) = \
> - { __tpstrtab_##name, STATIC_KEY_INIT_FALSE, reg, unreg, NULL };\
> + { __tpstrtab_##name, STATIC_KEY_INIT_FALSE, reg, unreg, NULL, num_args };\
> static struct tracepoint * const __tracepoint_ptr_##name __used \
> __attribute__((section("__tracepoints_ptrs"))) = \
> &__tracepoint_##name;
>
> -#define DEFINE_TRACE(name) \
> - DEFINE_TRACE_FN(name, NULL, NULL);
> +#define DEFINE_TRACE(name, num_args) \
> + DEFINE_TRACE_FN(name, NULL, NULL, num_args);
>
> #define EXPORT_TRACEPOINT_SYMBOL_GPL(name) \
> EXPORT_SYMBOL_GPL(__tracepoint_##name)
> @@ -275,8 +275,8 @@ extern void syscall_unregfunc(void);
> return false; \
> }
>
> -#define DEFINE_TRACE_FN(name, reg, unreg)
> -#define DEFINE_TRACE(name)
> +#define DEFINE_TRACE_FN(name, reg, unreg, num_args)
> +#define DEFINE_TRACE(name, num_args)
> #define EXPORT_TRACEPOINT_SYMBOL_GPL(name)
> #define EXPORT_TRACEPOINT_SYMBOL(name)
>
> diff --git a/include/trace/define_trace.h b/include/trace/define_trace.h
> index d9e3d4aa3f6e..96b22ace9ae7 100644
> --- a/include/trace/define_trace.h
> +++ b/include/trace/define_trace.h
> @@ -25,7 +25,7 @@
>
> #undef TRACE_EVENT
> #define TRACE_EVENT(name, proto, args, tstruct, assign, print) \
> - DEFINE_TRACE(name)
> + DEFINE_TRACE(name, COUNT_ARGS(args))
>
> #undef TRACE_EVENT_CONDITION
> #define TRACE_EVENT_CONDITION(name, proto, args, cond, tstruct, assign, print) \
> @@ -39,24 +39,24 @@
> #undef TRACE_EVENT_FN
> #define TRACE_EVENT_FN(name, proto, args, tstruct, \
> assign, print, reg, unreg) \
> - DEFINE_TRACE_FN(name, reg, unreg)
> + DEFINE_TRACE_FN(name, reg, unreg, COUNT_ARGS(args))
>
> #undef TRACE_EVENT_FN_COND
> #define TRACE_EVENT_FN_COND(name, proto, args, cond, tstruct, \
> assign, print, reg, unreg) \
> - DEFINE_TRACE_FN(name, reg, unreg)
> + DEFINE_TRACE_FN(name, reg, unreg, COUNT_ARGS(args))
>
> #undef DEFINE_EVENT
> #define DEFINE_EVENT(template, name, proto, args) \
> - DEFINE_TRACE(name)
> + DEFINE_TRACE(name, COUNT_ARGS(args))
>
> #undef DEFINE_EVENT_FN
> #define DEFINE_EVENT_FN(template, name, proto, args, reg, unreg) \
> - DEFINE_TRACE_FN(name, reg, unreg)
> + DEFINE_TRACE_FN(name, reg, unreg, COUNT_ARGS(args))
>
> #undef DEFINE_EVENT_PRINT
> #define DEFINE_EVENT_PRINT(template, name, proto, args, print) \
> - DEFINE_TRACE(name)
> + DEFINE_TRACE(name, COUNT_ARGS(args))
>
> #undef DEFINE_EVENT_CONDITION
> #define DEFINE_EVENT_CONDITION(template, name, proto, args, cond) \
> @@ -64,7 +64,7 @@
>
> #undef DECLARE_TRACE
> #define DECLARE_TRACE(name, proto, args) \
> - DEFINE_TRACE(name)
> + DEFINE_TRACE(name, COUNT_ARGS(args))
>
> #undef TRACE_INCLUDE
> #undef __TRACE_INCLUDE
^ permalink raw reply
* Re: [PATCH net-next, 2/2] hv_netvsc: Add range checking for rx packet offset and length
From: Stephen Hemminger @ 2018-03-27 15:22 UTC (permalink / raw)
To: Haiyang Zhang
Cc: olaf, sthemmin, netdev, haiyangz, linux-kernel, devel, vkuznets,
davem
In-Reply-To: <20180322190114.25596-3-haiyangz@linuxonhyperv.com>
On Thu, 22 Mar 2018 12:01:14 -0700
Haiyang Zhang <haiyangz@linuxonhyperv.com> wrote:
> From: Haiyang Zhang <haiyangz@microsoft.com>
>
> This patch adds range checking for rx packet offset and length.
> It may only happen if there is a host side bug.
>
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> ---
> drivers/net/hyperv/hyperv_net.h | 1 +
> drivers/net/hyperv/netvsc.c | 17 +++++++++++++++--
> 2 files changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
> index 0db3bd1ea06f..49c05ac894e5 100644
> --- a/drivers/net/hyperv/hyperv_net.h
> +++ b/drivers/net/hyperv/hyperv_net.h
> @@ -793,6 +793,7 @@ struct netvsc_device {
>
> /* Receive buffer allocated by us but manages by NetVSP */
> void *recv_buf;
> + u32 recv_buf_size; /* allocated bytes */
> u32 recv_buf_gpadl_handle;
> u32 recv_section_cnt;
> u32 recv_section_size;
> diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
> index 1ddb2c39b6e4..a6700d65f206 100644
> --- a/drivers/net/hyperv/netvsc.c
> +++ b/drivers/net/hyperv/netvsc.c
> @@ -289,6 +289,8 @@ static int netvsc_init_buf(struct hv_device *device,
> goto cleanup;
> }
>
> + net_device->recv_buf_size = buf_size;
> +
> /*
> * Establish the gpadl handle for this buffer on this
> * channel. Note: This call uses the vmbus connection rather
> @@ -1095,11 +1097,22 @@ static int netvsc_receive(struct net_device *ndev,
>
> /* Each range represents 1 RNDIS pkt that contains 1 ethernet frame */
> for (i = 0; i < count; i++) {
> - void *data = recv_buf
> - + vmxferpage_packet->ranges[i].byte_offset;
> + u32 offset = vmxferpage_packet->ranges[i].byte_offset;
> u32 buflen = vmxferpage_packet->ranges[i].byte_count;
> + void *data;
> int ret;
>
> + if (unlikely(offset + buflen > net_device->recv_buf_size)) {
> + status = NVSP_STAT_FAIL;
> + netif_err(net_device_ctx, rx_err, ndev,
> + "Packet offset:%u + len:%u too big\n",
> + offset, buflen);
> +
> + continue;
> + }
> +
If one part of the RNDIS packet is wrong then the whole receive
buffer is damaged. Just return, don't continue.
It could really just be a statistic and a one shot log message.
^ permalink raw reply
* Re: [PATCH v2 iproute2-next 3/6] rdma: Add CM_ID resource tracking information
From: Jason Gunthorpe @ 2018-03-27 15:23 UTC (permalink / raw)
To: Leon Romanovsky; +Cc: Steve Wise, David Ahern, stephen, netdev, linux-rdma
In-Reply-To: <20180327151544.GZ1877@mtr-leonro.local>
On Tue, Mar 27, 2018 at 06:15:44PM +0300, Leon Romanovsky wrote:
> On Tue, Mar 27, 2018 at 08:44:55AM -0600, Jason Gunthorpe wrote:
> > On Tue, Mar 27, 2018 at 06:21:41AM +0300, Leon Romanovsky wrote:
> > > On Mon, Mar 26, 2018 at 04:30:33PM -0600, Jason Gunthorpe wrote:
> > > > On Mon, Mar 26, 2018 at 04:34:44PM -0500, Steve Wise wrote:
> > > > >
> > > > > On 3/26/2018 4:15 PM, Jason Gunthorpe wrote:
> > > > > > On Mon, Mar 26, 2018 at 09:30:41AM -0500, Steve Wise wrote:
> > > > > >>
> > > > > >> On 3/26/2018 9:17 AM, David Ahern wrote:
> > > > > >>> On 2/27/18 9:07 AM, Steve Wise wrote:
> > > > > >>>> diff --git a/rdma/rdma.h b/rdma/rdma.h
> > > > > >>>> index 5809f70..e55205b 100644
> > > > > >>>> +++ b/rdma/rdma.h
> > > > > >>>> @@ -18,10 +18,12 @@
> > > > > >>>> #include <libmnl/libmnl.h>
> > > > > >>>> #include <rdma/rdma_netlink.h>
> > > > > >>>> #include <time.h>
> > > > > >>>> +#include <net/if_arp.h>
> > > > > >>>>
> > > > > >>>> #include "list.h"
> > > > > >>>> #include "utils.h"
> > > > > >>>> #include "json_writer.h"
> > > > > >>>> +#include <rdma/rdma_cma.h>
> > > > > >>>>
> > > > > >>> did you forget to add rdma_cma.h? I don't see that file in my repo.
> > > > > >> It is provided by the rdma-core package, upon which rdma tool now
> > > > > >> depends for the rdma_port_space enum.
> > > > > > It is a kernel bug that enum is not in an include/uapi/rdma header
> > > > > >
> > > > > > Fix it there and don't try to use rdma-core headers to get kernel ABI.
> > > > > >
> > > > > > Jason
> > > > >
> > > > > I wish you'd commented on this just a little sooner. I just resent v3
> > > > > of this series... with rdma_cma.h included. :)
> > > > >
> > > > > How about the restrack/nldev code just translates the port space from
> > > > > enum rdma_port_space to a new ABI enum, say nldev_rdma_port_space, that
> > > > > i add to rdma_netlink.h? I'd hate to open the can of worms of trying to
> > > > > split rdma_cma.h into uabi and no uabi headers. :(
> > > >
> > > > If port space is already part of the ABI there isn't much reason to
> > > > translate it.
> > > >
> > > > You just need to pick the right header to put it in, since it is a verbs
> > > > define it doesn't belong in the netlink header.
> > >
> > > I completely understand Steve's concerns.
> > >
> > > I tried to do such thing (expose kernel headers) in first incarnation of
> > > rdmatool with attempt to clean IB/core as well to ensure that we won't expose
> > > anything that is not implemented. It didn't go well.
> >
> > rdma-core is now using the kernel uapi/ headers natively, seems to be
> > going OK. What problem did you face?
>
> I didn't agree to move to UAPI defines which are not implemented and not
> used in the kernel, so I sent small number of patches similar to those [1, 2].
>
> Those patches were rejected.
>
> So please don't mix Steve's need to use 3 defines with very large and
> painful task to expose proper UAPIs.
Steve can just move the 3 defines he needs to the uapi, we are doing
this incrementally..
rdma_core does not define kernel ABI and it is totally wrong to use
random constants from rdma_cma.h as kernel ABI.
Jason
^ permalink raw reply
* [PATCH v2 net] strparser: Fix sign of err codes
From: Dave Watson @ 2018-03-27 15:23 UTC (permalink / raw)
To: David S. Miller, Tom Herbert, netdev
strp_parser_err is called with a negative code everywhere, which then
calls abort_parser with a negative code. strp_msg_timeout calls
abort_parser directly with a positive code. Negate ETIMEDOUT
to match signed-ness of other calls.
The default abort_parser callback, strp_abort_strp, sets
sk->sk_err to err. Also negate the error here so sk_err always
holds a positive value, as the rest of the net code expects. Currently
a negative sk_err can result in endless loops, or user code that
thinks it actually sent/received err bytes.
Found while testing net/tls_sw recv path.
Fixes: 43a0c6751a322847 ("strparser: Stream parser for messages")
Signed-off-by: Dave Watson <davejwatson@fb.com>
---
Documentation/networking/strparser.txt | 5 +++--
net/strparser/strparser.c | 8 ++++----
2 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/Documentation/networking/strparser.txt b/Documentation/networking/strparser.txt
index 13081b3..23bf827 100644
--- a/Documentation/networking/strparser.txt
+++ b/Documentation/networking/strparser.txt
@@ -158,7 +158,8 @@ int (*read_sock_done)(struct strparser *strp, int err);
the TCP socket in receive callback mode. The stream parser may
read multiple messages in a loop and this function allows cleanup
to occur when exiting the loop. If the callback is not set (NULL
- in strp_init) a default function is used.
+ in strp_init) a default function is used. err is a negative
+ error value.
void (*abort_parser)(struct strparser *strp, int err);
@@ -166,7 +167,7 @@ void (*abort_parser)(struct strparser *strp, int err);
in parsing. The default function stops the stream parser and
sets the error in the socket if the parser is in receive callback
mode. The default function can be changed by setting the callback
- to non-NULL in strp_init.
+ to non-NULL in strp_init. err is a negative error value.
Statistics
==========
diff --git a/net/strparser/strparser.c b/net/strparser/strparser.c
index 1fdab5c..a82ca09 100644
--- a/net/strparser/strparser.c
+++ b/net/strparser/strparser.c
@@ -44,7 +44,7 @@ static inline struct _strp_msg *_strp_msg(struct sk_buff *skb)
offsetof(struct qdisc_skb_cb, data));
}
-/* Lower lock held */
+/* Lower lock held. err is a negative error value. */
static void strp_abort_strp(struct strparser *strp, int err)
{
/* Unrecoverable error in receive */
@@ -60,7 +60,7 @@ static void strp_abort_strp(struct strparser *strp, int err)
struct sock *sk = strp->sk;
/* Report an error on the lower socket */
- sk->sk_err = err;
+ sk->sk_err = -err;
sk->sk_error_report(sk);
}
}
@@ -71,7 +71,7 @@ static void strp_start_timer(struct strparser *strp, long timeo)
mod_delayed_work(strp_wq, &strp->msg_timer_work, timeo);
}
-/* Lower lock held */
+/* Lower lock held. err is a negative error value. */
static void strp_parser_err(struct strparser *strp, int err,
read_descriptor_t *desc)
{
@@ -458,7 +458,7 @@ static void strp_msg_timeout(struct work_struct *w)
/* Message assembly timed out */
STRP_STATS_INCR(strp->stats.msg_timeouts);
strp->cb.lock(strp);
- strp->cb.abort_parser(strp, ETIMEDOUT);
+ strp->cb.abort_parser(strp, -ETIMEDOUT);
strp->cb.unlock(strp);
}
--
2.9.5
^ permalink raw reply related
* Re: [PATCH net-next v2 0/2] kernel: add support to collect hardware logs in crash recovery kernel
From: Rahul Lakkireddy @ 2018-03-27 15:27 UTC (permalink / raw)
To: Eric W. Biederman
Cc: netdev@vger.kernel.org, linux-fsdevel@vger.kernel.org,
kexec@lists.infradead.org, linux-kernel@vger.kernel.org,
davem@davemloft.net, viro@zeniv.linux.org.uk,
stephen@networkplumber.org, akpm@linux-foundation.org,
torvalds@linux-foundation.org, Ganesh GR, Nirranjan Kirubaharan,
Indranil Choudhury
In-Reply-To: <87k1txfyj5.fsf@xmission.com>
On Tuesday, March 03/27/18, 2018 at 18:47:34 +0530, Eric W. Biederman wrote:
> Rahul Lakkireddy <rahul.lakkireddy@chelsio.com> writes:
>
> > On Saturday, March 03/24/18, 2018 at 20:50:52 +0530, Eric W. Biederman wrote:
> >>
> >> Rahul Lakkireddy <rahul.lakkireddy@chelsio.com> writes:
> >>
> >> > On production servers running variety of workloads over time, kernel
> >> > panic can happen sporadically after days or even months. It is
> >> > important to collect as much debug logs as possible to root cause
> >> > and fix the problem, that may not be easy to reproduce. Snapshot of
> >> > underlying hardware/firmware state (like register dump, firmware
> >> > logs, adapter memory, etc.), at the time of kernel panic will be very
> >> > helpful while debugging the culprit device driver.
> >> >
> >> > This series of patches add new generic framework that enable device
> >> > drivers to collect device specific snapshot of the hardware/firmware
> >> > state of the underlying device in the crash recovery kernel. In crash
> >> > recovery kernel, the collected logs are exposed via /sys/kernel/crashdd/
> >> > directory, which is copied by user space scripts for post-analysis.
> >> >
> >> > A kernel module crashdd is newly added. In crash recovery kernel,
> >> > crashdd exposes /sys/kernel/crashdd/ directory containing device
> >> > specific hardware/firmware logs.
> >>
> >> Have you looked at instead of adding a sysfs file adding the dumps
> >> as additional elf notes in /proc/vmcore?
> >>
> >
> > I see the crash recovery kernel's memory is not present in any of the
> > the PT_LOAD headers. So, makedumpfile is not collecting the dumps
> > that are in crash recovery kernel's memory.
> >
> > Also, are you suggesting exporting the dumps themselves as PT_NOTE
> > instead? I'll look into doing it this way.
>
> Yes. I was suggesting exporting the dumps themselves as PT_NOTE
> in /proc/vmcore. I think that will allow makedumpfile to collect
> your new information without modification.
>
If I export the dumps themselves as PT_NOTE in /proc/vmcore, can the
crash tool work without modification; i.e can crash tool extract these
notes?
Thanks,
Rahul
^ permalink raw reply
* Re: [patch iproute2 rfc 1/2] devlink: introduce support for showing port flavours
From: Stephen Hemminger @ 2018-03-27 15:28 UTC (permalink / raw)
To: Jiri Pirko
Cc: netdev, davem, idosch, jakub.kicinski, mlxsw, andrew,
vivien.didelot, f.fainelli, michael.chan, ganeshgr, saeedm,
simon.horman, pieter.jansenvanvuuren, john.hurley,
dirk.vandermerwe, alexander.h.duyck, ogerlitz, dsahern,
vijaya.guvva, satananda.burla, raghu.vatsavayi, felix.manlunas,
gospo, sathya.perla, vasundhara-v.volam, tariqt, eranbe,
jeffrey.t.kirsher
In-Reply-To: <20180323063558.11718-1-jiri@resnulli.us>
On Fri, 23 Mar 2018 07:35:57 +0100
Jiri Pirko <jiri@resnulli.us> wrote:
> +static const char *port_flavour_name(uint16_t flavour)
> +{
> + switch (flavour) {
> + case DEVLINK_PORT_FLAVOUR_PHYSICAL: return "physical";
> + case DEVLINK_PORT_FLAVOUR_PF_REP: return "pf_rep";
> + case DEVLINK_PORT_FLAVOUR_VF_REP: return "vf_rep";
> + default: return "<unknown flavour>";
> + }
> +}
> +
need to break lines even in switch statement.
^ permalink raw reply
* Re: [PATCH 1/6] rhashtable: improve documentation for rhashtable_walk_peek()
From: Herbert Xu @ 2018-03-27 15:30 UTC (permalink / raw)
To: NeilBrown; +Cc: Thomas Graf, netdev, linux-kernel, Tom Herbert
In-Reply-To: <152210718418.11435.11573013181393548255.stgit@noble>
On Tue, Mar 27, 2018 at 10:33:04AM +1100, NeilBrown wrote:
> The documentation for rhashtable_walk_peek() wrong. It claims to
> return the *next* entry, whereas it in fact returns the *previous*
> entry.
> However if no entries have yet been returned - or if the iterator
> was reset due to a resize event, then rhashtable_walk_peek()
> *does* return the next entry, but also advances the iterator.
>
> I suspect that this interface should be discarded and the one user
> should be changed to not require it. Possibly this patch should be
> seen as a first step in that conversation.
>
> This patch mostly corrects the documentation, but does make a
> small code change so that the documentation can be correct without
> listing too many special cases. I don't think the one user will
> be affected by the code change.
>
> Signed-off-by: NeilBrown <neilb@suse.com>
We should cc Tom Herbert too since he wrote this code.
> ---
> lib/rhashtable.c | 17 +++++++++++++----
> 1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/lib/rhashtable.c b/lib/rhashtable.c
> index 3825c30aaa36..24a57ca494cb 100644
> --- a/lib/rhashtable.c
> +++ b/lib/rhashtable.c
> @@ -853,13 +853,17 @@ void *rhashtable_walk_next(struct rhashtable_iter *iter)
> EXPORT_SYMBOL_GPL(rhashtable_walk_next);
>
> /**
> - * rhashtable_walk_peek - Return the next object but don't advance the iterator
> + * rhashtable_walk_peek - Return the previously returned object without advancing the iterator
> * @iter: Hash table iterator
> *
> - * Returns the next object or NULL when the end of the table is reached.
> + * Returns the last object returned, or NULL if no object has yet been returned.
> + * If the previously returned object has since been removed, then some other arbitrary
> + * object maybe returned, or possibly NULL will be returned. In that case, the
> + * iterator might be advanced.
> *
> * Returns -EAGAIN if resize event occurred. Note that the iterator
> - * will rewind back to the beginning and you may continue to use it.
> + * will rewind back to the beginning and rhashtable_walk_next() should be
> + * used to get the next object.
> */
> void *rhashtable_walk_peek(struct rhashtable_iter *iter)
> {
> @@ -880,7 +884,12 @@ void *rhashtable_walk_peek(struct rhashtable_iter *iter)
> * the table hasn't changed.
> */
> iter->skip--;
> - }
> + } else
> + /* ->skip is only zero after rhashtable_walk_start()
> + * or when the iterator is reset. In this case there
> + * is no previous object to return.
> + */
> + return NULL;
>
> return __rhashtable_walk_find_next(iter);
> }
>
>
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* RE: [PATCH net-next, 2/2] hv_netvsc: Add range checking for rx packet offset and length
From: Haiyang Zhang @ 2018-03-27 15:35 UTC (permalink / raw)
To: Stephen Hemminger
Cc: davem@davemloft.net, netdev@vger.kernel.org, olaf@aepfle.de,
Stephen Hemminger, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, vkuznets@redhat.com
In-Reply-To: <20180327082245.591612dc@xeon-e3>
> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Tuesday, March 27, 2018 11:23 AM
> To: Haiyang Zhang <haiyangz@linuxonhyperv.com>
> Cc: Haiyang Zhang <haiyangz@microsoft.com>; davem@davemloft.net;
> netdev@vger.kernel.org; olaf@aepfle.de; Stephen Hemminger
> <sthemmin@microsoft.com>; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; vkuznets@redhat.com
> Subject: Re: [PATCH net-next, 2/2] hv_netvsc: Add range checking for rx packet
> offset and length
>
> On Thu, 22 Mar 2018 12:01:14 -0700
> Haiyang Zhang <haiyangz@linuxonhyperv.com> wrote:
>
> > From: Haiyang Zhang <haiyangz@microsoft.com>
> >
> > This patch adds range checking for rx packet offset and length.
> > It may only happen if there is a host side bug.
> >
> > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> > ---
> > drivers/net/hyperv/hyperv_net.h | 1 +
> > drivers/net/hyperv/netvsc.c | 17 +++++++++++++++--
> > 2 files changed, 16 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/hyperv/hyperv_net.h
> > b/drivers/net/hyperv/hyperv_net.h index 0db3bd1ea06f..49c05ac894e5
> > 100644
> > --- a/drivers/net/hyperv/hyperv_net.h
> > +++ b/drivers/net/hyperv/hyperv_net.h
> > @@ -793,6 +793,7 @@ struct netvsc_device {
> >
> > /* Receive buffer allocated by us but manages by NetVSP */
> > void *recv_buf;
> > + u32 recv_buf_size; /* allocated bytes */
> > u32 recv_buf_gpadl_handle;
> > u32 recv_section_cnt;
> > u32 recv_section_size;
> > diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
> > index 1ddb2c39b6e4..a6700d65f206 100644
> > --- a/drivers/net/hyperv/netvsc.c
> > +++ b/drivers/net/hyperv/netvsc.c
> > @@ -289,6 +289,8 @@ static int netvsc_init_buf(struct hv_device *device,
> > goto cleanup;
> > }
> >
> > + net_device->recv_buf_size = buf_size;
> > +
> > /*
> > * Establish the gpadl handle for this buffer on this
> > * channel. Note: This call uses the vmbus connection rather @@
> > -1095,11 +1097,22 @@ static int netvsc_receive(struct net_device
> > *ndev,
> >
> > /* Each range represents 1 RNDIS pkt that contains 1 ethernet frame */
> > for (i = 0; i < count; i++) {
> > - void *data = recv_buf
> > - + vmxferpage_packet->ranges[i].byte_offset;
> > + u32 offset = vmxferpage_packet->ranges[i].byte_offset;
> > u32 buflen = vmxferpage_packet->ranges[i].byte_count;
> > + void *data;
> > int ret;
> >
> > + if (unlikely(offset + buflen > net_device->recv_buf_size)) {
> > + status = NVSP_STAT_FAIL;
> > + netif_err(net_device_ctx, rx_err, ndev,
> > + "Packet offset:%u + len:%u too big\n",
> > + offset, buflen);
> > +
> > + continue;
> > + }
> > +
>
> If one part of the RNDIS packet is wrong then the whole receive buffer is
> damaged. Just return, don't continue.
>
> It could really just be a statistic and a one shot log message.
I will let the loop terminates and send NVSP status fail to the host.
For statistics, this range check is to catch potential host side issues, just like
these checks in the same function earlier:
/* Make sure this is a valid nvsp packet */
if (unlikely(nvsp->hdr.msg_type != NVSP_MSG1_TYPE_SEND_RNDIS_PKT)) {
netif_err(net_device_ctx, rx_err, ndev,
"Unknown nvsp packet type received %u\n",
nvsp->hdr.msg_type);
return 0;
}
if (unlikely(vmxferpage_packet->xfer_pageset_id != NETVSC_RECEIVE_BUFFER_ID)) {
netif_err(net_device_ctx, rx_err, ndev,
"Invalid xfer page set id - expecting %x got %x\n",
NETVSC_RECEIVE_BUFFER_ID,
vmxferpage_packet->xfer_pageset_id);
return 0;
}
If these kinds of errors need statistics, there will be many stat variables... Maybe we
should just create one stat variable for all of the "invalid format from host"?
Thanks,
- Haiyang
^ permalink raw reply
* Re: [PATCH 5/6] net/ipv4: Update ip_tunnel_metadata_cnt static key to modern api
From: Davidlohr Bueso @ 2018-03-27 15:37 UTC (permalink / raw)
To: David Miller; +Cc: peterz, mingo, akpm, linux-kernel, netdev, dbueso
In-Reply-To: <20180327.114402.619275306009710582.davem@davemloft.net>
On Tue, 27 Mar 2018, David Miller wrote:
>From: Davidlohr Bueso <dave@stgolabs.net>
>Date: Mon, 26 Mar 2018 14:09:28 -0700
>
>> No changes in refcount semantics -- key init is false; replace
>>
>> static_key_slow_inc|dec with static_branch_inc|dec
>> static_key_false with static_branch_unlikely
>>
>> Added a '_key' suffix to i2c_trace_msg, for better self
>> documentation.
>
>I see no reference to i2c_trace_msg in this patch.
Sorry, that was from a previous changelog. The reason why it was not updated was
because I chose not to rename to ip_tunnel_metadata_cnt_key, so the whole sentence
needed removal. Do you want a v3 or can you pick up as is?
Thanks,
Davidlohr
^ permalink raw reply
* [bpf-next PATCH] net: br_vlan build error
From: John Fastabend @ 2018-03-27 15:38 UTC (permalink / raw)
To: stephen, 3chas3; +Cc: netdev, davem
Fix build error in br_if.c
net/bridge/br_if.c: In function ‘br_mtu’:
net/bridge/br_if.c:458:8: error: ‘const struct net_bridge’ has no member named ‘vlan_enabled’
if (br->vlan_enabled)
^
net/bridge/br_if.c:462:1: warning: control reaches end of non-void function [-Wreturn-type]
}
^
Fixes: 419d14af9e07f ("bridge: Allow max MTU when multiple VLANs present")
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
---
net/bridge/br_if.c | 27 +++++++++++++++++----------
1 file changed, 17 insertions(+), 10 deletions(-)
diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index 48dc4d2..2262424 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -424,16 +424,6 @@ int br_del_bridge(struct net *net, const char *name)
return ret;
}
-static bool min_mtu(int a, int b)
-{
- return a < b ? 1 : 0;
-}
-
-static bool max_mtu(int a, int b)
-{
- return a > b ? 1 : 0;
-}
-
/* MTU of the bridge pseudo-device: ETH_DATA_LEN or the minimum of the ports */
static int __br_mtu(const struct net_bridge *br, bool (compare_fn)(int, int))
{
@@ -453,6 +443,17 @@ static int __br_mtu(const struct net_bridge *br, bool (compare_fn)(int, int))
return mtu;
}
+static bool min_mtu(int a, int b)
+{
+ return a < b ? 1 : 0;
+}
+
+#ifdef CONFIG_BRIDGE_VLAN_FILTERING
+static bool max_mtu(int a, int b)
+{
+ return a > b ? 1 : 0;
+}
+
int br_mtu(const struct net_bridge *br)
{
if (br->vlan_enabled)
@@ -460,6 +461,12 @@ int br_mtu(const struct net_bridge *br)
else
return __br_mtu(br, min_mtu);
}
+#else
+int br_mtu(const struct net_bridge *br)
+{
+ return __br_mtu(br, min_mtu);
+}
+#endif
static void br_set_gso_limits(struct net_bridge *br)
{
^ permalink raw reply related
* Re: [pull request][net V2 0/8] Mellanox, mlx5 fixes 2018-03-23
From: David Miller @ 2018-03-27 15:42 UTC (permalink / raw)
To: saeedm; +Cc: netdev
In-Reply-To: <20180326210505.11007-1-saeedm@mellanox.com>
From: Saeed Mahameed <saeedm@mellanox.com>
Date: Mon, 26 Mar 2018 14:04:57 -0700
> The following series includes fixes for mlx5 netdev and eswitch.
>
> v1->v2:
> - Fixed commit message quotation marks in patch #7
Pulled.
> For -stable v4.12
> ('net/mlx5e: Avoid using the ipv6 stub in the TC offload neigh update path')
> ('net/mlx5e: Fix traffic being dropped on VF representor')
>
> For -stable v4.13
> ('net/mlx5e: Fix memory usage issues in offloading TC flows')
> ('net/mlx5e: Verify coalescing parameters in range')
>
> For -stable v4.14
> ('net/mlx5e: Don't override vport admin link state in switchdev mode')
>
> For -stable v4.15
> ('108b2b6d5c02 net/mlx5e: Sync netdev vxlan ports at open')
Queued up for -stable.
Thanks.
^ permalink raw reply
* Re: [B.A.T.M.A.N.] [PATCH 03/17] batman-adv: Add network_coding and mcast sysfs files to README
From: Linus Lüssing @ 2018-03-27 15:43 UTC (permalink / raw)
To: Jiri Pirko
Cc: The list for a Better Approach To Mobile Ad-hoc Networking,
Sven Eckelmann, netdev, davem
In-Reply-To: <20161029105628.GE1692@nanopsycho.orion>
On Sat, Oct 29, 2016 at 12:56:28PM +0200, Jiri Pirko wrote:
> >> I strongly believe it is a huge mistake to use sysfs for things like
> >> this. This should be done via generic netlink api.
> >
> >This doesn't change the problem that it is already that way. This patch
> >only adds the list of available files to the README.
>
> Sure. Just found out you did it like that. Therefore I commented. I
> suggest to rework the api to use genl entirely.
Hi Jiri,
Thanks for sharing your thoughts!
Could you explain a bit more on which disadvantages you see in
the usage of sysfs here?
Regards, Linus
^ permalink raw reply
* Re: [PATCH 5/6] net/ipv4: Update ip_tunnel_metadata_cnt static key to modern api
From: David Miller @ 2018-03-27 15:44 UTC (permalink / raw)
To: dave; +Cc: peterz, mingo, akpm, linux-kernel, netdev, dbueso
In-Reply-To: <20180326210929.5244-6-dave@stgolabs.net>
From: Davidlohr Bueso <dave@stgolabs.net>
Date: Mon, 26 Mar 2018 14:09:28 -0700
> No changes in refcount semantics -- key init is false; replace
>
> static_key_slow_inc|dec with static_branch_inc|dec
> static_key_false with static_branch_unlikely
>
> Added a '_key' suffix to i2c_trace_msg, for better self
> documentation.
I see no reference to i2c_trace_msg in this patch.
^ permalink raw reply
* RE: [PATCH v2 iproute2-next 3/6] rdma: Add CM_ID resource tracking information
From: Steve Wise @ 2018-03-27 15:45 UTC (permalink / raw)
To: 'Jason Gunthorpe', 'Leon Romanovsky'
Cc: 'David Ahern', stephen, netdev, linux-rdma
In-Reply-To: <20180327152356.GH12318@ziepe.ca>
>
> On Tue, Mar 27, 2018 at 06:15:44PM +0300, Leon Romanovsky wrote:
> > On Tue, Mar 27, 2018 at 08:44:55AM -0600, Jason Gunthorpe wrote:
> > > On Tue, Mar 27, 2018 at 06:21:41AM +0300, Leon Romanovsky wrote:
> > > > On Mon, Mar 26, 2018 at 04:30:33PM -0600, Jason Gunthorpe wrote:
> > > > > On Mon, Mar 26, 2018 at 04:34:44PM -0500, Steve Wise wrote:
> > > > > >
> > > > > > On 3/26/2018 4:15 PM, Jason Gunthorpe wrote:
> > > > > > > On Mon, Mar 26, 2018 at 09:30:41AM -0500, Steve Wise wrote:
> > > > > > >>
> > > > > > >> On 3/26/2018 9:17 AM, David Ahern wrote:
> > > > > > >>> On 2/27/18 9:07 AM, Steve Wise wrote:
> > > > > > >>>> diff --git a/rdma/rdma.h b/rdma/rdma.h
> > > > > > >>>> index 5809f70..e55205b 100644
> > > > > > >>>> +++ b/rdma/rdma.h
> > > > > > >>>> @@ -18,10 +18,12 @@
> > > > > > >>>> #include <libmnl/libmnl.h>
> > > > > > >>>> #include <rdma/rdma_netlink.h>
> > > > > > >>>> #include <time.h>
> > > > > > >>>> +#include <net/if_arp.h>
> > > > > > >>>>
> > > > > > >>>> #include "list.h"
> > > > > > >>>> #include "utils.h"
> > > > > > >>>> #include "json_writer.h"
> > > > > > >>>> +#include <rdma/rdma_cma.h>
> > > > > > >>>>
> > > > > > >>> did you forget to add rdma_cma.h? I don't see that file in my
> repo.
> > > > > > >> It is provided by the rdma-core package, upon which rdma tool
> now
> > > > > > >> depends for the rdma_port_space enum.
> > > > > > > It is a kernel bug that enum is not in an include/uapi/rdma
> header
> > > > > > >
> > > > > > > Fix it there and don't try to use rdma-core headers to get kernel
> ABI.
> > > > > > >
> > > > > > > Jason
> > > > > >
> > > > > > I wish you'd commented on this just a little sooner. I just resent
> v3
> > > > > > of this series... with rdma_cma.h included. :)
> > > > > >
> > > > > > How about the restrack/nldev code just translates the port space
> from
> > > > > > enum rdma_port_space to a new ABI enum, say
> nldev_rdma_port_space, that
> > > > > > i add to rdma_netlink.h? I'd hate to open the can of worms of
> trying to
> > > > > > split rdma_cma.h into uabi and no uabi headers. :(
> > > > >
> > > > > If port space is already part of the ABI there isn't much reason to
> > > > > translate it.
> > > > >
> > > > > You just need to pick the right header to put it in, since it is a verbs
> > > > > define it doesn't belong in the netlink header.
> > > >
> > > > I completely understand Steve's concerns.
> > > >
> > > > I tried to do such thing (expose kernel headers) in first incarnation of
> > > > rdmatool with attempt to clean IB/core as well to ensure that we
> won't expose
> > > > anything that is not implemented. It didn't go well.
> > >
> > > rdma-core is now using the kernel uapi/ headers natively, seems to be
> > > going OK. What problem did you face?
> >
> > I didn't agree to move to UAPI defines which are not implemented and
> not
> > used in the kernel, so I sent small number of patches similar to those [1,
> 2].
> >
> > Those patches were rejected.
> >
> > So please don't mix Steve's need to use 3 defines with very large and
> > painful task to expose proper UAPIs.
>
> Steve can just move the 3 defines he needs to the uapi, we are doing
> this incrementally..
>
> rdma_core does not define kernel ABI and it is totally wrong to use
> random constants from rdma_cma.h as kernel ABI.
>
Proposal:
Since the cm_id port space is part of the rdma_ucm_create_id struct in include/uapi/rdma/rdma_user_cm.h, I'll move the rdma_port_space enum there. And then my iproute2 series will have to add a copy of rdma_user_cm.h locally into rdma/include/uapi/rdma, right?
Will that work for everyone?
Steve.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox