* [net-next PATCH 09/14] net: sched: check for frozen queue before skb_bad_txq check
From: John Fastabend @ 2017-12-07 17:56 UTC (permalink / raw)
To: willemdebruijn.kernel, daniel, eric.dumazet, davem
Cc: netdev, jiri, xiyou.wangcong
In-Reply-To: <20171207173500.5771.41198.stgit@john-Precision-Tower-5810>
I can not think of any reason to pull the bad txq skb off the qdisc if
the txq we plan to send this on is still frozen. So check for frozen
queue first and abort before dequeuing either skb_bad_txq skb or
normal qdisc dequeue() skb.
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
---
net/sched/sch_generic.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 84cef05..5ff93c2 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -204,7 +204,7 @@ static struct sk_buff *dequeue_skb(struct Qdisc *q, bool *validate,
int *packets)
{
const struct netdev_queue *txq = q->dev_queue;
- struct sk_buff *skb;
+ struct sk_buff *skb = NULL;
*packets = 1;
if (unlikely(!skb_queue_empty(&q->gso_skb))) {
@@ -248,12 +248,15 @@ static struct sk_buff *dequeue_skb(struct Qdisc *q, bool *validate,
}
validate:
*validate = true;
+
+ if ((q->flags & TCQ_F_ONETXQUEUE) &&
+ netif_xmit_frozen_or_stopped(txq))
+ return skb;
+
skb = qdisc_dequeue_skb_bad_txq(q);
if (unlikely(skb))
goto bulk;
- if (!(q->flags & TCQ_F_ONETXQUEUE) ||
- !netif_xmit_frozen_or_stopped(txq))
- skb = q->dequeue(q);
+ skb = q->dequeue(q);
if (skb) {
bulk:
if (qdisc_may_bulk(q))
^ permalink raw reply related
* [net-next PATCH 08/14] net: sched: use skb list for skb_bad_tx
From: John Fastabend @ 2017-12-07 17:56 UTC (permalink / raw)
To: willemdebruijn.kernel, daniel, eric.dumazet, davem
Cc: netdev, jiri, xiyou.wangcong
In-Reply-To: <20171207173500.5771.41198.stgit@john-Precision-Tower-5810>
Similar to how gso is handled use skb list for skb_bad_tx this is
required with lockless qdiscs because we may have multiple cores
attempting to push skbs into skb_bad_tx concurrently
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
---
include/net/sch_generic.h | 2 -
net/sched/sch_generic.c | 106 +++++++++++++++++++++++++++++++++++++--------
2 files changed, 87 insertions(+), 21 deletions(-)
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 6e329f0..4717c4b 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -95,7 +95,7 @@ struct Qdisc {
struct gnet_stats_queue qstats;
unsigned long state;
struct Qdisc *next_sched;
- struct sk_buff *skb_bad_txq;
+ struct sk_buff_head skb_bad_txq;
int padded;
refcount_t refcnt;
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 482ba22..84cef05 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -45,6 +45,68 @@
* - ingress filtering is also serialized via qdisc root lock
* - updates to tree and tree walking are only done under the rtnl mutex.
*/
+
+static inline struct sk_buff *__skb_dequeue_bad_txq(struct Qdisc *q)
+{
+ const struct netdev_queue *txq = q->dev_queue;
+ spinlock_t *lock = NULL;
+ struct sk_buff *skb;
+
+ if (q->flags & TCQ_F_NOLOCK) {
+ lock = qdisc_lock(q);
+ spin_lock(lock);
+ }
+
+ skb = skb_peek(&q->skb_bad_txq);
+ if (skb) {
+ /* check the reason of requeuing without tx lock first */
+ txq = skb_get_tx_queue(txq->dev, skb);
+ if (!netif_xmit_frozen_or_stopped(txq)) {
+ skb = __skb_dequeue(&q->skb_bad_txq);
+ if (qdisc_is_percpu_stats(q)) {
+ qdisc_qstats_cpu_backlog_dec(q, skb);
+ qdisc_qstats_cpu_qlen_dec(q);
+ } else {
+ qdisc_qstats_backlog_dec(q, skb);
+ q->q.qlen--;
+ }
+ } else {
+ skb = NULL;
+ }
+ }
+
+ if (lock)
+ spin_unlock(lock);
+
+ return skb;
+}
+
+static inline struct sk_buff *qdisc_dequeue_skb_bad_txq(struct Qdisc *q)
+{
+ struct sk_buff *skb = skb_peek(&q->skb_bad_txq);
+
+ if (unlikely(skb))
+ skb = __skb_dequeue_bad_txq(q);
+
+ return skb;
+}
+
+static inline void qdisc_enqueue_skb_bad_txq(struct Qdisc *q,
+ struct sk_buff *skb)
+{
+ spinlock_t *lock = NULL;
+
+ if (q->flags & TCQ_F_NOLOCK) {
+ lock = qdisc_lock(q);
+ spin_lock(lock);
+ }
+
+ __skb_queue_tail(&q->skb_bad_txq, skb);
+
+ if (lock)
+ spin_unlock(lock);
+}
+
static inline int __dev_requeue_skb(struct sk_buff *skb, struct Qdisc *q)
{
__skb_queue_head(&q->gso_skb, skb);
@@ -117,9 +179,15 @@ static void try_bulk_dequeue_skb_slow(struct Qdisc *q,
if (!nskb)
break;
if (unlikely(skb_get_queue_mapping(nskb) != mapping)) {
- q->skb_bad_txq = nskb;
- qdisc_qstats_backlog_inc(q, nskb);
- q->q.qlen++;
+ qdisc_enqueue_skb_bad_txq(q, nskb);
+
+ if (qdisc_is_percpu_stats(q)) {
+ qdisc_qstats_cpu_backlog_inc(q, nskb);
+ qdisc_qstats_cpu_qlen_inc(q);
+ } else {
+ qdisc_qstats_backlog_inc(q, nskb);
+ q->q.qlen++;
+ }
break;
}
skb->next = nskb;
@@ -180,19 +248,9 @@ static struct sk_buff *dequeue_skb(struct Qdisc *q, bool *validate,
}
validate:
*validate = true;
- skb = q->skb_bad_txq;
- if (unlikely(skb)) {
- /* check the reason of requeuing without tx lock first */
- txq = skb_get_tx_queue(txq->dev, skb);
- if (!netif_xmit_frozen_or_stopped(txq)) {
- q->skb_bad_txq = NULL;
- qdisc_qstats_backlog_dec(q, skb);
- q->q.qlen--;
- goto bulk;
- }
- skb = NULL;
- goto trace;
- }
+ skb = qdisc_dequeue_skb_bad_txq(q);
+ if (unlikely(skb))
+ goto bulk;
if (!(q->flags & TCQ_F_ONETXQUEUE) ||
!netif_xmit_frozen_or_stopped(txq))
skb = q->dequeue(q);
@@ -680,6 +738,7 @@ struct Qdisc *qdisc_alloc(struct netdev_queue *dev_queue,
sch->padded = (char *) sch - (char *) p;
}
__skb_queue_head_init(&sch->gso_skb);
+ __skb_queue_head_init(&sch->skb_bad_txq);
qdisc_skb_head_init(&sch->q);
spin_lock_init(&sch->q.lock);
@@ -753,14 +812,16 @@ void qdisc_reset(struct Qdisc *qdisc)
if (ops->reset)
ops->reset(qdisc);
- kfree_skb(qdisc->skb_bad_txq);
- qdisc->skb_bad_txq = NULL;
-
skb_queue_walk_safe(&qdisc->gso_skb, skb, tmp) {
__skb_unlink(skb, &qdisc->gso_skb);
kfree_skb_list(skb);
}
+ skb_queue_walk_safe(&qdisc->skb_bad_txq, skb, tmp) {
+ __skb_unlink(skb, &qdisc->skb_bad_txq);
+ kfree_skb_list(skb);
+ }
+
qdisc->q.qlen = 0;
qdisc->qstats.backlog = 0;
}
@@ -804,7 +865,11 @@ void qdisc_destroy(struct Qdisc *qdisc)
kfree_skb_list(skb);
}
- kfree_skb(qdisc->skb_bad_txq);
+ skb_queue_walk_safe(&qdisc->skb_bad_txq, skb, tmp) {
+ __skb_unlink(skb, &qdisc->skb_bad_txq);
+ kfree_skb_list(skb);
+ }
+
qdisc_free(qdisc);
}
EXPORT_SYMBOL(qdisc_destroy);
@@ -1042,6 +1107,7 @@ static void dev_init_scheduler_queue(struct net_device *dev,
rcu_assign_pointer(dev_queue->qdisc, qdisc);
dev_queue->qdisc_sleeping = qdisc;
__skb_queue_head_init(&qdisc->gso_skb);
+ __skb_queue_head_init(&qdisc->skb_bad_txq);
}
void dev_init_scheduler(struct net_device *dev)
^ permalink raw reply related
* Re: [Intel-wired-lan] [next-queue 08/10] ixgbe: process the Tx ipsec offload
From: Alexander Duyck @ 2017-12-07 17:56 UTC (permalink / raw)
To: Shannon Nelson
Cc: intel-wired-lan, Jeff Kirsher, Steffen Klassert, Sowmini Varadhan,
Netdev
In-Reply-To: <1c5fddf2-4d93-d1cf-2ac0-d3b2bcb0c682@oracle.com>
On Wed, Dec 6, 2017 at 9:43 PM, Shannon Nelson
<shannon.nelson@oracle.com> wrote:
> On 12/5/2017 10:13 AM, Alexander Duyck wrote:
>>
>> On Mon, Dec 4, 2017 at 9:35 PM, Shannon Nelson
>> <shannon.nelson@oracle.com> wrote:
>>>
>>> If the skb has a security association referenced in the skb, then
>>> set up the Tx descriptor with the ipsec offload bits. While we're
>>> here, we fix an oddly named field in the context descriptor struct.
>>>
>>> Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com>
>>> ---
>>> drivers/net/ethernet/intel/ixgbe/ixgbe.h | 10 +++-
>>> drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c | 77
>>> ++++++++++++++++++++++++++
>>> drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c | 4 +-
>>> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 38 ++++++++++---
>>> drivers/net/ethernet/intel/ixgbe/ixgbe_type.h | 2 +-
>>> 5 files changed, 118 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>>> b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>>> index 77f07dc..68097fe 100644
>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>>> @@ -171,10 +171,11 @@ enum ixgbe_tx_flags {
>>> IXGBE_TX_FLAGS_CC = 0x08,
>>> IXGBE_TX_FLAGS_IPV4 = 0x10,
>>> IXGBE_TX_FLAGS_CSUM = 0x20,
>>> + IXGBE_TX_FLAGS_IPSEC = 0x40,
>>>
>>> /* software defined flags */
>>> - IXGBE_TX_FLAGS_SW_VLAN = 0x40,
>>> - IXGBE_TX_FLAGS_FCOE = 0x80,
>>> + IXGBE_TX_FLAGS_SW_VLAN = 0x80,
>>> + IXGBE_TX_FLAGS_FCOE = 0x100,
>>> };
>>>
>>> /* VLAN info */
>>> @@ -1012,12 +1013,17 @@ void ixgbe_init_ipsec_offload(struct
>>> ixgbe_adapter *adapter);
>>> void ixgbe_ipsec_rx(struct ixgbe_ring *rx_ring,
>>> union ixgbe_adv_rx_desc *rx_desc,
>>> struct sk_buff *skb);
>>> +int ixgbe_ipsec_tx(struct ixgbe_ring *tx_ring, struct sk_buff *skb,
>>> + __be16 protocol, struct ixgbe_ipsec_tx_data *itd);
>>> void ixgbe_ipsec_restore(struct ixgbe_adapter *adapter);
>>> #else
>>> static inline void ixgbe_init_ipsec_offload(struct ixgbe_adapter
>>> *adapter) { };
>>> static inline void ixgbe_ipsec_rx(struct ixgbe_ring *rx_ring,
>>> union ixgbe_adv_rx_desc *rx_desc,
>>> struct sk_buff *skb) { };
>>> +static inline int ixgbe_ipsec_tx(struct ixgbe_ring *tx_ring,
>>> + struct sk_buff *skb, __be16 protocol,
>>> + struct ixgbe_ipsec_tx_data *itd) {
>>> return 0; };
>>> static inline void ixgbe_ipsec_restore(struct ixgbe_adapter *adapter) {
>>> };
>>> #endif /* CONFIG_XFRM_OFFLOAD */
>>> #endif /* _IXGBE_H_ */
>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>>> index fd06d9b..2a0dd7a 100644
>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>>> @@ -703,12 +703,89 @@ static void ixgbe_ipsec_del_sa(struct xfrm_state
>>> *xs)
>>> }
>>> }
>>>
>>> +/**
>>> + * ixgbe_ipsec_offload_ok - can this packet use the xfrm hw offload
>>> + * @skb: current data packet
>>> + * @xs: pointer to transformer state struct
>>> + **/
>>> +static bool ixgbe_ipsec_offload_ok(struct sk_buff *skb, struct
>>> xfrm_state *xs)
>>> +{
>>> + if (xs->props.family == AF_INET) {
>>> + /* Offload with IPv4 options is not supported yet */
>>> + if (ip_hdr(skb)->ihl > 5)
>>
>>
>> I would make this ihl != 5 instead of "> 5" since smaller values would
>> be invalid as well.
>
>
> Sure
>
>
>>
>>> + return false;
>>> + } else {
>>> + /* Offload with IPv6 extension headers is not support yet
>>> */
>>> + if (ipv6_ext_hdr(ipv6_hdr(skb)->nexthdr))
>>> + return false;
>>> + }
>>> +
>>> + return true;
>>> +}
>>> +
>>> static const struct xfrmdev_ops ixgbe_xfrmdev_ops = {
>>> .xdo_dev_state_add = ixgbe_ipsec_add_sa,
>>> .xdo_dev_state_delete = ixgbe_ipsec_del_sa,
>>> + .xdo_dev_offload_ok = ixgbe_ipsec_offload_ok,
>>> };
>>>
>>> /**
>>> + * ixgbe_ipsec_tx - setup Tx flags for ipsec offload
>>> + * @tx_ring: outgoing context
>>> + * @skb: current data packet
>>> + * @protocol: network protocol
>>> + * @itd: ipsec Tx data for later use in building context descriptor
>>> + **/
>>> +int ixgbe_ipsec_tx(struct ixgbe_ring *tx_ring, struct sk_buff *skb,
>>> + __be16 protocol, struct ixgbe_ipsec_tx_data *itd)
>>> +{
>>> + struct ixgbe_adapter *adapter = netdev_priv(tx_ring->netdev);
>>> + struct ixgbe_ipsec *ipsec = adapter->ipsec;
>>> + struct xfrm_state *xs;
>>> + struct tx_sa *tsa;
>>> +
>>> + if (!skb->sp->len) {
>>> + netdev_err(tx_ring->netdev, "%s: no xfrm state len =
>>> %d\n",
>>> + __func__, skb->sp->len);
>>> + return 0;
>>> + }
>>> +
>>> + xs = xfrm_input_state(skb);
>>> + if (!xs) {
>>> + netdev_err(tx_ring->netdev, "%s: no xfrm_input_state() xs
>>> = %p\n",
>>> + __func__, xs);
>>> + return 0;
>>> + }
>>> +
>>> + itd->sa_idx = xs->xso.offload_handle - IXGBE_IPSEC_BASE_TX_INDEX;
>>> + if (itd->sa_idx > IXGBE_IPSEC_MAX_SA_COUNT) {
>>> + netdev_err(tx_ring->netdev, "%s: bad sa_idx=%d
>>> handle=%lu\n",
>>> + __func__, itd->sa_idx,
>>> xs->xso.offload_handle);
>>> + return 0;
>>> + }
>>> +
>>> + tsa = &ipsec->tx_tbl[itd->sa_idx];
>>> + if (!tsa->used) {
>>> + netdev_err(tx_ring->netdev, "%s: unused sa_idx=%d\n",
>>> + __func__, itd->sa_idx);
>>> + return 0;
>>> + }
>>> +
>>> + itd->flags = 0;
>>> + if (xs->id.proto == IPPROTO_ESP) {
>>> + itd->flags |= IXGBE_ADVTXD_TUCMD_IPSEC_TYPE_ESP |
>>> + IXGBE_ADVTXD_TUCMD_L4T_TCP;
>>
>>
>> Why is the TCP value being set here? This doesn't seem correct either.
>> This implies TCP a TCP offload. It seems like this should only be
>> setting ESP.
>
>
> Honestly? Because when I was testing that, it didn't work without it. This
> was one of the things I was going to come back to when I started working on
> the csum and tso support.
We might want to try testing with that dropped to see if we need it or
not. I would suspect not since I would imagine this would cause bad
things for non-TCP traffic. Also the inner L4 header shouldn't matter
unless you are trying to offload it.
>>
>>> + if (protocol == htons(ETH_P_IP))
>>> + itd->flags |= IXGBE_ADVTXD_TUCMD_IPV4;
>>
>>
>> Does the IPsec offload need to know if the frame is v4 or v6? I'm just
>> wondering if it does or not.
>
>
> Yes, I believe this is how it knows how much header to skip to find the ESP
> header. However, I'll test that and see if it can come out.
Like I mentioned last time it might be better to have this handled in
ixgbe_tx_csum. If it is harmless we can probably just include it
there. We should be able to do it in the block after the no_csum
label. I'd be curious if not doing this up until now might have other
effects such as impacting RSS since I know the whole reason for us
having to do the CC stuff anyway was to actually get header split to
work correctly with PF/VF loopback packets. It wouldn't surprise me if
setting these fields defines the packet type received on the other
end.
>> If not then this probably isn't needed.
>> One thought on this line is you might look at moving it into
>> ixgbe_tx_csum. If setting the bit is harmless without setting IXSM we
>> might look at moving it into the end of ixgbe_tx_csum and just make it
>> compare against first->protocol there.
>
>
>>
>>> + itd->trailer_len = xs->props.trailer_len;
>>> + }
>>> + if (tsa->encrypt)
>>> + itd->flags |= IXGBE_ADVTXD_TUCMD_IPSEC_ENCRYPT_EN;
>>> +
>>> + return 1;
>>> +}
>>> +
>>> +/**
>>> * ixgbe_ipsec_rx - decode ipsec bits from Rx descriptor
>>> * @rx_ring: receiving ring
>>> * @rx_desc: receive data descriptor
>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c
>>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c
>>> index f1bfae0..d7875b3 100644
>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c
>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c
>>> @@ -1261,7 +1261,7 @@ void ixgbe_clear_interrupt_scheme(struct
>>> ixgbe_adapter *adapter)
>>> }
>>>
>>> void ixgbe_tx_ctxtdesc(struct ixgbe_ring *tx_ring, u32 vlan_macip_lens,
>>> - u32 fcoe_sof_eof, u32 type_tucmd, u32
>>> mss_l4len_idx)
>>> + u32 fceof_saidx, u32 type_tucmd, u32
>>> mss_l4len_idx)
>>> {
>>> struct ixgbe_adv_tx_context_desc *context_desc;
>>> u16 i = tx_ring->next_to_use;
>>> @@ -1275,7 +1275,7 @@ void ixgbe_tx_ctxtdesc(struct ixgbe_ring *tx_ring,
>>> u32 vlan_macip_lens,
>>> type_tucmd |= IXGBE_TXD_CMD_DEXT | IXGBE_ADVTXD_DTYP_CTXT;
>>>
>>> context_desc->vlan_macip_lens = cpu_to_le32(vlan_macip_lens);
>>> - context_desc->seqnum_seed = cpu_to_le32(fcoe_sof_eof);
>>> + context_desc->fceof_saidx = cpu_to_le32(fceof_saidx);
>>> context_desc->type_tucmd_mlhl = cpu_to_le32(type_tucmd);
>>> context_desc->mss_l4len_idx = cpu_to_le32(mss_l4len_idx);
>>> }
>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>> index 60f9f2d..c857594 100644
>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>> @@ -7659,9 +7659,10 @@ static void ixgbe_service_task(struct work_struct
>>> *work)
>>>
>>> static int ixgbe_tso(struct ixgbe_ring *tx_ring,
>>> struct ixgbe_tx_buffer *first,
>>> - u8 *hdr_len)
>>> + u8 *hdr_len,
>>> + struct ixgbe_ipsec_tx_data *itd)
>>> {
>>> - u32 vlan_macip_lens, type_tucmd, mss_l4len_idx;
>>> + u32 vlan_macip_lens, type_tucmd, mss_l4len_idx, fceof_saidx = 0;
>>> struct sk_buff *skb = first->skb;
>>> union {
>>> struct iphdr *v4;
>>> @@ -7740,7 +7741,12 @@ static int ixgbe_tso(struct ixgbe_ring *tx_ring,
>>> vlan_macip_lens |= (ip.hdr - skb->data) <<
>>> IXGBE_ADVTXD_MACLEN_SHIFT;
>>> vlan_macip_lens |= first->tx_flags & IXGBE_TX_FLAGS_VLAN_MASK;
>>>
>>> - ixgbe_tx_ctxtdesc(tx_ring, vlan_macip_lens, 0, type_tucmd,
>>> + if (first->tx_flags & IXGBE_TX_FLAGS_IPSEC) {
>>> + fceof_saidx |= itd->sa_idx;
>>> + type_tucmd |= itd->flags | itd->trailer_len;
>>> + }
So just a thought. Why bother with the TX_FLAGS_CHECK at all? It seems
like in the case that the flag isn't set you would have itd->sa_idx
equal to 0 anyway so it would still be the same result wouldn't it? It
would save you from having to zero both fceof_saidx and itd->sa_idx
since you could just pass itd->sa_idx and save yourself the extra
variable.
Also if flags and trailer_len are both being written to the same
location why not combine them in your structure into one single 32 bit
entry? It would allow you to essentially reduce this to one OR and you
could just pass itd->sa_idx directly which should be a pretty
significant savings in terms of instructions and cycles. Also you
might want to consider bumping itd->sa_idx up to a 32b value. It will
possibly cost you a cycle or so to convert the 16b value to a 32b
value before writing it. If you merge the flags and trailer length you
should have the space to spare to bump up the size.
>>> +
>>> + ixgbe_tx_ctxtdesc(tx_ring, vlan_macip_lens, fceof_saidx,
>>> type_tucmd,
>>> mss_l4len_idx);
>>>
>>> return 1;
>>> @@ -7756,10 +7762,12 @@ static inline bool ixgbe_ipv6_csum_is_sctp(struct
>>> sk_buff *skb)
>>> }
>>>
>>> static void ixgbe_tx_csum(struct ixgbe_ring *tx_ring,
>>> - struct ixgbe_tx_buffer *first)
>>> + struct ixgbe_tx_buffer *first,
>>> + struct ixgbe_ipsec_tx_data *itd)
>>> {
>>> struct sk_buff *skb = first->skb;
>>> u32 vlan_macip_lens = 0;
>>> + u32 fceof_saidx = 0;
>>> u32 type_tucmd = 0;
>>>
>>> if (skb->ip_summed != CHECKSUM_PARTIAL) {
>>> @@ -7800,7 +7808,12 @@ static void ixgbe_tx_csum(struct ixgbe_ring
>>> *tx_ring,
>>> vlan_macip_lens |= skb_network_offset(skb) <<
>>> IXGBE_ADVTXD_MACLEN_SHIFT;
>>> vlan_macip_lens |= first->tx_flags & IXGBE_TX_FLAGS_VLAN_MASK;
>>>
>>> - ixgbe_tx_ctxtdesc(tx_ring, vlan_macip_lens, 0, type_tucmd, 0);
>>> + if (first->tx_flags & IXGBE_TX_FLAGS_IPSEC) {
>>> + fceof_saidx |= itd->sa_idx;
>>> + type_tucmd |= itd->flags | itd->trailer_len;
>>> + }
>>> +
>>> + ixgbe_tx_ctxtdesc(tx_ring, vlan_macip_lens, fceof_saidx,
>>> type_tucmd, 0);
>>> }
>>>
>>> #define IXGBE_SET_FLAG(_input, _flag, _result) \
>>> @@ -7843,11 +7856,16 @@ static void ixgbe_tx_olinfo_status(union
>>> ixgbe_adv_tx_desc *tx_desc,
>>> IXGBE_TX_FLAGS_CSUM,
>>> IXGBE_ADVTXD_POPTS_TXSM);
>>>
>>> - /* enble IPv4 checksum for TSO */
>>> + /* enable IPv4 checksum for TSO */
>>> olinfo_status |= IXGBE_SET_FLAG(tx_flags,
>>> IXGBE_TX_FLAGS_IPV4,
>>> IXGBE_ADVTXD_POPTS_IXSM);
>>>
>>> + /* enable IPsec */
>>> + olinfo_status |= IXGBE_SET_FLAG(tx_flags,
>>> + IXGBE_TX_FLAGS_IPSEC,
>>> + IXGBE_ADVTXD_POPTS_IPSEC);
>>> +
>>> /*
>>> * Check Context must be set if Tx switch is enabled, which it
>>> * always is for case where virtual functions are running
>>> @@ -8306,6 +8324,7 @@ netdev_tx_t ixgbe_xmit_frame_ring(struct sk_buff
>>> *skb,
>>> u32 tx_flags = 0;
>>> unsigned short f;
>>> u16 count = TXD_USE_COUNT(skb_headlen(skb));
>>> + struct ixgbe_ipsec_tx_data ipsec_tx = { 0 };
>>> __be16 protocol = skb->protocol;
>>> u8 hdr_len = 0;
>>>
>>> @@ -8394,6 +8413,9 @@ netdev_tx_t ixgbe_xmit_frame_ring(struct sk_buff
>>> *skb,
>>> }
>>> }
>>>
>>> + if (skb->sp && ixgbe_ipsec_tx(tx_ring, skb, protocol, &ipsec_tx))
>>> + tx_flags |= IXGBE_TX_FLAGS_IPSEC | IXGBE_TX_FLAGS_CC;
>>
>>
>> You might just want to pull the skb->sp check into ixgbe_ipsec_tx and
>> could pass tx_flags as a part of the first buffer. It doesn't really
>> matter anyway as most of this will just be inlined so it will all end
>> up a part of the same function anyway.
>
>
> Since the function is defined in a different .o file, are you sure it will
> get inlined? I put the skb->sp check here to make sure we don't do an
> unnecessary jump.
You're right. I forgot you are defining this in a different file.
Still I would like to see this moved down though. Where it is at
doesn't really flow with everything else since FCoE and this aren't
likely to ever interact so I would rather us check for FCoE and then
get into the IPsec logic.
>>
>> Also I would move this down so that it is handled after the fields in
>> the first buffer_info structure are set. Then this can ll just fall
>> inline with the TSO block and get handled there.
>>
>>> +
>>> /* record initial flags and protocol */
>>> first->tx_flags = tx_flags;
>>> first->protocol = protocol;
>>> @@ -8410,11 +8432,11 @@ netdev_tx_t ixgbe_xmit_frame_ring(struct sk_buff
>>> *skb,
>>> }
>>>
>>> #endif /* IXGBE_FCOE */
>>
>>
>> So if you move the function down here it will help to avoid any other
>> complication. In addition you could follow the same logic that we do
>> for ixgbe_tso/fso so you could drop the frame instead of transmitting
>> it if it is requesting a bad offload.
>
>
> Sure
>
> sln
>
>
>>
>>> - tso = ixgbe_tso(tx_ring, first, &hdr_len);
>>> + tso = ixgbe_tso(tx_ring, first, &hdr_len, &ipsec_tx);
>>> if (tso < 0)
>>> goto out_drop;
>>> else if (!tso)
>>> - ixgbe_tx_csum(tx_ring, first);
>>> + ixgbe_tx_csum(tx_ring, first, &ipsec_tx);
>>>
>>> /* add the ATR filter if ATR is on */
>>> if (test_bit(__IXGBE_TX_FDIR_INIT_DONE, &tx_ring->state))
>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h
>>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h
>>> index 3df0763..0ac725fa 100644
>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h
>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h
>>> @@ -2856,7 +2856,7 @@ union ixgbe_adv_rx_desc {
>>> /* Context descriptors */
>>> struct ixgbe_adv_tx_context_desc {
>>> __le32 vlan_macip_lens;
>>> - __le32 seqnum_seed;
>>> + __le32 fceof_saidx;
>>> __le32 type_tucmd_mlhl;
>>> __le32 mss_l4len_idx;
>>> };
>>> --
>>> 2.7.4
>>>
>>> _______________________________________________
>>> Intel-wired-lan mailing list
>>> Intel-wired-lan@osuosl.org
>>> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply
* [net-next PATCH 07/14] net: sched: drop qdisc_reset from dev_graft_qdisc
From: John Fastabend @ 2017-12-07 17:56 UTC (permalink / raw)
To: willemdebruijn.kernel, daniel, eric.dumazet, davem
Cc: netdev, jiri, xiyou.wangcong
In-Reply-To: <20171207173500.5771.41198.stgit@john-Precision-Tower-5810>
In qdisc_graft_qdisc a "new" qdisc is attached and the 'qdisc_destroy'
operation is called on the old qdisc. The destroy operation will wait
a rcu grace period and call qdisc_rcu_free(). At which point
gso_cpu_skb is free'd along with all stats so no need to zero stats
and gso_cpu_skb from the graft operation itself.
Further after dropping the qdisc locks we can not continue to call
qdisc_reset before waiting an rcu grace period so that the qdisc is
detached from all cpus. By removing the qdisc_reset() here we get
the correct property of waiting an rcu grace period and letting the
qdisc_destroy operation clean up the qdisc correctly.
Note, a refcnt greater than 1 would cause the destroy operation to
be aborted however if this ever happened the reference to the qdisc
would be lost and we would have a memory leak.
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
---
net/sched/sch_generic.c | 28 +++++++++++++++++++---------
1 file changed, 19 insertions(+), 9 deletions(-)
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index dfeabe3..482ba22 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -819,10 +819,6 @@ struct Qdisc *dev_graft_qdisc(struct netdev_queue *dev_queue,
root_lock = qdisc_lock(oqdisc);
spin_lock_bh(root_lock);
- /* Prune old scheduler */
- if (oqdisc && refcount_read(&oqdisc->refcnt) <= 1)
- qdisc_reset(oqdisc);
-
/* ... and graft new one */
if (qdisc == NULL)
qdisc = &noop_qdisc;
@@ -977,6 +973,16 @@ static bool some_qdisc_is_busy(struct net_device *dev)
return false;
}
+static void dev_qdisc_reset(struct net_device *dev,
+ struct netdev_queue *dev_queue,
+ void *none)
+{
+ struct Qdisc *qdisc = dev_queue->qdisc_sleeping;
+
+ if (qdisc)
+ qdisc_reset(qdisc);
+}
+
/**
* dev_deactivate_many - deactivate transmissions on several devices
* @head: list of devices to deactivate
@@ -987,7 +993,6 @@ static bool some_qdisc_is_busy(struct net_device *dev)
void dev_deactivate_many(struct list_head *head)
{
struct net_device *dev;
- bool sync_needed = false;
list_for_each_entry(dev, head, close_list) {
netdev_for_each_tx_queue(dev, dev_deactivate_queue,
@@ -997,20 +1002,25 @@ void dev_deactivate_many(struct list_head *head)
&noop_qdisc);
dev_watchdog_down(dev);
- sync_needed |= !dev->dismantle;
}
/* Wait for outstanding qdisc-less dev_queue_xmit calls.
* This is avoided if all devices are in dismantle phase :
* Caller will call synchronize_net() for us
*/
- if (sync_needed)
- synchronize_net();
+ synchronize_net();
/* Wait for outstanding qdisc_run calls. */
- list_for_each_entry(dev, head, close_list)
+ list_for_each_entry(dev, head, close_list) {
while (some_qdisc_is_busy(dev))
yield();
+ /* The new qdisc is assigned at this point so we can safely
+ * unwind stale skb lists and qdisc statistics
+ */
+ netdev_for_each_tx_queue(dev, dev_qdisc_reset, NULL);
+ if (dev_ingress_queue(dev))
+ dev_qdisc_reset(dev, dev_ingress_queue(dev), NULL);
+ }
}
void dev_deactivate(struct net_device *dev)
^ permalink raw reply related
* [net-next PATCH 06/14] net: sched: explicit locking in gso_cpu fallback
From: John Fastabend @ 2017-12-07 17:55 UTC (permalink / raw)
To: willemdebruijn.kernel, daniel, eric.dumazet, davem
Cc: netdev, jiri, xiyou.wangcong
In-Reply-To: <20171207173500.5771.41198.stgit@john-Precision-Tower-5810>
This work is preparing the qdisc layer to support egress lockless
qdiscs. If we are running the egress qdisc lockless in the case we
overrun the netdev, for whatever reason, the netdev returns a busy
error code and the skb is parked on the gso_skb pointer. With many
cores all hitting this case at once its possible to have multiple
sk_buffs here so we turn gso_skb into a queue.
This should be the edge case and if we see this frequently then
the netdev/qdisc layer needs to back off.
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
---
include/net/sch_generic.h | 20 ++++++-----
net/sched/sch_generic.c | 85 ++++++++++++++++++++++++++++++++++++++-------
2 files changed, 84 insertions(+), 21 deletions(-)
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 7bc2826..6e329f0 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -88,7 +88,7 @@ struct Qdisc {
/*
* For performance sake on SMP, we put highly modified fields at the end
*/
- struct sk_buff *gso_skb ____cacheline_aligned_in_smp;
+ struct sk_buff_head gso_skb ____cacheline_aligned_in_smp;
struct qdisc_skb_head q;
struct gnet_stats_basic_packed bstats;
seqcount_t running;
@@ -795,26 +795,30 @@ static inline struct sk_buff *qdisc_peek_head(struct Qdisc *sch)
/* generic pseudo peek method for non-work-conserving qdisc */
static inline struct sk_buff *qdisc_peek_dequeued(struct Qdisc *sch)
{
+ struct sk_buff *skb = skb_peek(&sch->gso_skb);
+
/* we can reuse ->gso_skb because peek isn't called for root qdiscs */
- if (!sch->gso_skb) {
- sch->gso_skb = sch->dequeue(sch);
- if (sch->gso_skb) {
+ if (!skb) {
+ skb = sch->dequeue(sch);
+
+ if (skb) {
+ __skb_queue_head(&sch->gso_skb, skb);
/* it's still part of the queue */
- qdisc_qstats_backlog_inc(sch, sch->gso_skb);
+ qdisc_qstats_backlog_inc(sch, skb);
sch->q.qlen++;
}
}
- return sch->gso_skb;
+ return skb;
}
/* use instead of qdisc->dequeue() for all qdiscs queried with ->peek() */
static inline struct sk_buff *qdisc_dequeue_peeked(struct Qdisc *sch)
{
- struct sk_buff *skb = sch->gso_skb;
+ struct sk_buff *skb = skb_peek(&sch->gso_skb);
if (skb) {
- sch->gso_skb = NULL;
+ skb = __skb_dequeue(&sch->gso_skb);
qdisc_qstats_backlog_dec(sch, skb);
sch->q.qlen--;
} else {
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 80e4ae3..dfeabe3 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -45,10 +45,9 @@
* - ingress filtering is also serialized via qdisc root lock
* - updates to tree and tree walking are only done under the rtnl mutex.
*/
-
-static inline int dev_requeue_skb(struct sk_buff *skb, struct Qdisc *q)
+static inline int __dev_requeue_skb(struct sk_buff *skb, struct Qdisc *q)
{
- q->gso_skb = skb;
+ __skb_queue_head(&q->gso_skb, skb);
q->qstats.requeues++;
qdisc_qstats_backlog_inc(q, skb);
q->q.qlen++; /* it's still part of the queue */
@@ -57,6 +56,30 @@ static inline int dev_requeue_skb(struct sk_buff *skb, struct Qdisc *q)
return 0;
}
+static inline int dev_requeue_skb_locked(struct sk_buff *skb, struct Qdisc *q)
+{
+ spinlock_t *lock = qdisc_lock(q);
+
+ spin_lock(lock);
+ __skb_queue_tail(&q->gso_skb, skb);
+ spin_unlock(lock);
+
+ qdisc_qstats_cpu_requeues_inc(q);
+ qdisc_qstats_cpu_backlog_inc(q, skb);
+ qdisc_qstats_cpu_qlen_inc(q);
+ __netif_schedule(q);
+
+ return 0;
+}
+
+static inline int dev_requeue_skb(struct sk_buff *skb, struct Qdisc *q)
+{
+ if (q->flags & TCQ_F_NOLOCK)
+ return dev_requeue_skb_locked(skb, q);
+ else
+ return __dev_requeue_skb(skb, q);
+}
+
static void try_bulk_dequeue_skb(struct Qdisc *q,
struct sk_buff *skb,
const struct netdev_queue *txq,
@@ -112,23 +135,50 @@ static void try_bulk_dequeue_skb_slow(struct Qdisc *q,
static struct sk_buff *dequeue_skb(struct Qdisc *q, bool *validate,
int *packets)
{
- struct sk_buff *skb = q->gso_skb;
const struct netdev_queue *txq = q->dev_queue;
+ struct sk_buff *skb;
*packets = 1;
- if (unlikely(skb)) {
+ if (unlikely(!skb_queue_empty(&q->gso_skb))) {
+ spinlock_t *lock = NULL;
+
+ if (q->flags & TCQ_F_NOLOCK) {
+ lock = qdisc_lock(q);
+ spin_lock(lock);
+ }
+
+ skb = skb_peek(&q->gso_skb);
+
+ /* skb may be null if another cpu pulls gso_skb off in between
+ * empty check and lock.
+ */
+ if (!skb) {
+ if (lock)
+ spin_unlock(lock);
+ goto validate;
+ }
+
/* skb in gso_skb were already validated */
*validate = false;
/* check the reason of requeuing without tx lock first */
txq = skb_get_tx_queue(txq->dev, skb);
if (!netif_xmit_frozen_or_stopped(txq)) {
- q->gso_skb = NULL;
- qdisc_qstats_backlog_dec(q, skb);
- q->q.qlen--;
- } else
+ skb = __skb_dequeue(&q->gso_skb);
+ if (qdisc_is_percpu_stats(q)) {
+ qdisc_qstats_cpu_backlog_dec(q, skb);
+ qdisc_qstats_cpu_qlen_dec(q);
+ } else {
+ qdisc_qstats_backlog_dec(q, skb);
+ q->q.qlen--;
+ }
+ } else {
skb = NULL;
+ }
+ if (lock)
+ spin_unlock(lock);
goto trace;
}
+validate:
*validate = true;
skb = q->skb_bad_txq;
if (unlikely(skb)) {
@@ -629,6 +679,7 @@ struct Qdisc *qdisc_alloc(struct netdev_queue *dev_queue,
sch = (struct Qdisc *) QDISC_ALIGN((unsigned long) p);
sch->padded = (char *) sch - (char *) p;
}
+ __skb_queue_head_init(&sch->gso_skb);
qdisc_skb_head_init(&sch->q);
spin_lock_init(&sch->q.lock);
@@ -697,6 +748,7 @@ struct Qdisc *qdisc_create_dflt(struct netdev_queue *dev_queue,
void qdisc_reset(struct Qdisc *qdisc)
{
const struct Qdisc_ops *ops = qdisc->ops;
+ struct sk_buff *skb, *tmp;
if (ops->reset)
ops->reset(qdisc);
@@ -704,10 +756,11 @@ void qdisc_reset(struct Qdisc *qdisc)
kfree_skb(qdisc->skb_bad_txq);
qdisc->skb_bad_txq = NULL;
- if (qdisc->gso_skb) {
- kfree_skb_list(qdisc->gso_skb);
- qdisc->gso_skb = NULL;
+ skb_queue_walk_safe(&qdisc->gso_skb, skb, tmp) {
+ __skb_unlink(skb, &qdisc->gso_skb);
+ kfree_skb_list(skb);
}
+
qdisc->q.qlen = 0;
qdisc->qstats.backlog = 0;
}
@@ -726,6 +779,7 @@ static void qdisc_free(struct Qdisc *qdisc)
void qdisc_destroy(struct Qdisc *qdisc)
{
const struct Qdisc_ops *ops = qdisc->ops;
+ struct sk_buff *skb, *tmp;
if (qdisc->flags & TCQ_F_BUILTIN ||
!refcount_dec_and_test(&qdisc->refcnt))
@@ -745,7 +799,11 @@ void qdisc_destroy(struct Qdisc *qdisc)
module_put(ops->owner);
dev_put(qdisc_dev(qdisc));
- kfree_skb_list(qdisc->gso_skb);
+ skb_queue_walk_safe(&qdisc->gso_skb, skb, tmp) {
+ __skb_unlink(skb, &qdisc->gso_skb);
+ kfree_skb_list(skb);
+ }
+
kfree_skb(qdisc->skb_bad_txq);
qdisc_free(qdisc);
}
@@ -973,6 +1031,7 @@ static void dev_init_scheduler_queue(struct net_device *dev,
rcu_assign_pointer(dev_queue->qdisc, qdisc);
dev_queue->qdisc_sleeping = qdisc;
+ __skb_queue_head_init(&qdisc->gso_skb);
}
void dev_init_scheduler(struct net_device *dev)
^ permalink raw reply related
* [net-next PATCH 05/14] net: sched: a dflt qdisc may be used with per cpu stats
From: John Fastabend @ 2017-12-07 17:55 UTC (permalink / raw)
To: willemdebruijn.kernel, daniel, eric.dumazet, davem
Cc: netdev, jiri, xiyou.wangcong
In-Reply-To: <20171207173500.5771.41198.stgit@john-Precision-Tower-5810>
Enable dflt qdisc support for per cpu stats before this patch a dflt
qdisc was required to use the global statistics qstats and bstats.
This adds a static flags field to qdisc_ops that is propagated
into qdisc->flags in qdisc allocate call. This allows the allocation
block to completely allocate the qdisc object so we don't have
dangling allocations after qdisc init.
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
---
include/net/sch_generic.h | 1 +
net/sched/sch_generic.c | 16 ++++++++++++++++
2 files changed, 17 insertions(+)
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 7c4b96b..7bc2826 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -179,6 +179,7 @@ struct Qdisc_ops {
const struct Qdisc_class_ops *cl_ops;
char id[IFNAMSIZ];
int priv_size;
+ unsigned int static_flags;
int (*enqueue)(struct sk_buff *skb,
struct Qdisc *sch,
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index cbc0a9a..80e4ae3 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -632,6 +632,19 @@ struct Qdisc *qdisc_alloc(struct netdev_queue *dev_queue,
qdisc_skb_head_init(&sch->q);
spin_lock_init(&sch->q.lock);
+ if (ops->static_flags & TCQ_F_CPUSTATS) {
+ sch->cpu_bstats =
+ netdev_alloc_pcpu_stats(struct gnet_stats_basic_cpu);
+ if (!sch->cpu_bstats)
+ goto errout1;
+
+ sch->cpu_qstats = alloc_percpu(struct gnet_stats_queue);
+ if (!sch->cpu_qstats) {
+ free_percpu(sch->cpu_bstats);
+ goto errout1;
+ }
+ }
+
spin_lock_init(&sch->busylock);
lockdep_set_class(&sch->busylock,
dev->qdisc_tx_busylock ?: &qdisc_tx_busylock);
@@ -641,6 +654,7 @@ struct Qdisc *qdisc_alloc(struct netdev_queue *dev_queue,
dev->qdisc_running_key ?: &qdisc_running_key);
sch->ops = ops;
+ sch->flags = ops->static_flags;
sch->enqueue = ops->enqueue;
sch->dequeue = ops->dequeue;
sch->dev_queue = dev_queue;
@@ -648,6 +662,8 @@ struct Qdisc *qdisc_alloc(struct netdev_queue *dev_queue,
refcount_set(&sch->refcnt, 1);
return sch;
+errout1:
+ kfree(p);
errout:
return ERR_PTR(err);
}
^ permalink raw reply related
* [net-next PATCH 04/14] net: sched: provide per cpu qstat helpers
From: John Fastabend @ 2017-12-07 17:55 UTC (permalink / raw)
To: willemdebruijn.kernel, daniel, eric.dumazet, davem
Cc: netdev, jiri, xiyou.wangcong
In-Reply-To: <20171207173500.5771.41198.stgit@john-Precision-Tower-5810>
The per cpu qstats support was added with per cpu bstat support which
is currently used by the ingress qdisc. This patch adds a set of
helpers needed to make other qdiscs that use qstats per cpu as well.
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
---
include/net/sch_generic.h | 35 +++++++++++++++++++++++++++++++++++
1 file changed, 35 insertions(+)
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index bb806a0..7c4b96b 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -631,12 +631,39 @@ static inline void qdisc_qstats_backlog_dec(struct Qdisc *sch,
sch->qstats.backlog -= qdisc_pkt_len(skb);
}
+static inline void qdisc_qstats_cpu_backlog_dec(struct Qdisc *sch,
+ const struct sk_buff *skb)
+{
+ this_cpu_sub(sch->cpu_qstats->backlog, qdisc_pkt_len(skb));
+}
+
static inline void qdisc_qstats_backlog_inc(struct Qdisc *sch,
const struct sk_buff *skb)
{
sch->qstats.backlog += qdisc_pkt_len(skb);
}
+static inline void qdisc_qstats_cpu_backlog_inc(struct Qdisc *sch,
+ const struct sk_buff *skb)
+{
+ this_cpu_add(sch->cpu_qstats->backlog, qdisc_pkt_len(skb));
+}
+
+static inline void qdisc_qstats_cpu_qlen_inc(struct Qdisc *sch)
+{
+ this_cpu_inc(sch->cpu_qstats->qlen);
+}
+
+static inline void qdisc_qstats_cpu_qlen_dec(struct Qdisc *sch)
+{
+ this_cpu_dec(sch->cpu_qstats->qlen);
+}
+
+static inline void qdisc_qstats_cpu_requeues_inc(struct Qdisc *sch)
+{
+ this_cpu_inc(sch->cpu_qstats->requeues);
+}
+
static inline void __qdisc_qstats_drop(struct Qdisc *sch, int count)
{
sch->qstats.drops += count;
@@ -844,6 +871,14 @@ static inline void rtnl_qdisc_drop(struct sk_buff *skb, struct Qdisc *sch)
qdisc_qstats_drop(sch);
}
+static inline int qdisc_drop_cpu(struct sk_buff *skb, struct Qdisc *sch,
+ struct sk_buff **to_free)
+{
+ __qdisc_drop(skb, to_free);
+ qdisc_qstats_cpu_drop(sch);
+
+ return NET_XMIT_DROP;
+}
static inline int qdisc_drop(struct sk_buff *skb, struct Qdisc *sch,
struct sk_buff **to_free)
^ permalink raw reply related
* [net-next PATCH 03/14] net: sched: remove remaining uses for qdisc_qlen in xmit path
From: John Fastabend @ 2017-12-07 17:54 UTC (permalink / raw)
To: willemdebruijn.kernel, daniel, eric.dumazet, davem
Cc: netdev, jiri, xiyou.wangcong
In-Reply-To: <20171207173500.5771.41198.stgit@john-Precision-Tower-5810>
sch_direct_xmit() uses qdisc_qlen as a return value but all call sites
of the routine only check if it is zero or not. Simplify the logic so
that we don't need to return an actual queue length value.
This introduces a case now where sch_direct_xmit would have returned
a qlen of zero but now it returns true. However in this case all
call sites of sch_direct_xmit will implement a dequeue() and get
a null skb and abort. This trades tracking qlen in the hotpath for
an extra dequeue operation. Overall this seems to be good for
performance.
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
---
include/net/pkt_sched.h | 6 +++---
net/sched/sch_generic.c | 28 +++++++++++++---------------
2 files changed, 16 insertions(+), 18 deletions(-)
diff --git a/include/net/pkt_sched.h b/include/net/pkt_sched.h
index 4eea719..2404692 100644
--- a/include/net/pkt_sched.h
+++ b/include/net/pkt_sched.h
@@ -105,9 +105,9 @@ struct qdisc_rate_table *qdisc_get_rtab(struct tc_ratespec *r,
void qdisc_put_rtab(struct qdisc_rate_table *tab);
void qdisc_put_stab(struct qdisc_size_table *tab);
void qdisc_warn_nonwc(const char *txt, struct Qdisc *qdisc);
-int sch_direct_xmit(struct sk_buff *skb, struct Qdisc *q,
- struct net_device *dev, struct netdev_queue *txq,
- spinlock_t *root_lock, bool validate);
+bool sch_direct_xmit(struct sk_buff *skb, struct Qdisc *q,
+ struct net_device *dev, struct netdev_queue *txq,
+ spinlock_t *root_lock, bool validate);
void __qdisc_run(struct Qdisc *q);
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index ec757f6..cbc0a9a 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -164,12 +164,12 @@ static struct sk_buff *dequeue_skb(struct Qdisc *q, bool *validate,
* only one CPU can execute this function.
*
* Returns to the caller:
- * 0 - queue is empty or throttled.
- * >0 - queue is not empty.
+ * false - hardware queue frozen backoff
+ * true - feel free to send more pkts
*/
-int sch_direct_xmit(struct sk_buff *skb, struct Qdisc *q,
- struct net_device *dev, struct netdev_queue *txq,
- spinlock_t *root_lock, bool validate)
+bool sch_direct_xmit(struct sk_buff *skb, struct Qdisc *q,
+ struct net_device *dev, struct netdev_queue *txq,
+ spinlock_t *root_lock, bool validate)
{
int ret = NETDEV_TX_BUSY;
@@ -190,28 +190,26 @@ int sch_direct_xmit(struct sk_buff *skb, struct Qdisc *q,
} else {
if (root_lock)
spin_lock(root_lock);
- return qdisc_qlen(q);
+ return true;
}
if (root_lock)
spin_lock(root_lock);
- if (dev_xmit_complete(ret)) {
- /* Driver sent out skb successfully or skb was consumed */
- ret = qdisc_qlen(q);
- } else {
+ if (!dev_xmit_complete(ret)) {
/* Driver returned NETDEV_TX_BUSY - requeue skb */
if (unlikely(ret != NETDEV_TX_BUSY))
net_warn_ratelimited("BUG %s code %d qlen %d\n",
dev->name, ret, q->q.qlen);
- ret = dev_requeue_skb(skb, q);
+ dev_requeue_skb(skb, q);
+ return false;
}
if (ret && netif_xmit_frozen_or_stopped(txq))
- ret = 0;
+ return false;
- return ret;
+ return true;
}
/*
@@ -233,7 +231,7 @@ int sch_direct_xmit(struct sk_buff *skb, struct Qdisc *q,
* >0 - queue is not empty.
*
*/
-static inline int qdisc_restart(struct Qdisc *q, int *packets)
+static inline bool qdisc_restart(struct Qdisc *q, int *packets)
{
spinlock_t *root_lock = NULL;
struct netdev_queue *txq;
@@ -244,7 +242,7 @@ static inline int qdisc_restart(struct Qdisc *q, int *packets)
/* Dequeue packet */
skb = dequeue_skb(q, &validate, packets);
if (unlikely(!skb))
- return 0;
+ return false;
if (!(q->flags & TCQ_F_NOLOCK))
root_lock = qdisc_lock(q);
^ permalink raw reply related
* [net-next PATCH 02/14] net: sched: allow qdiscs to handle locking
From: John Fastabend @ 2017-12-07 17:54 UTC (permalink / raw)
To: willemdebruijn.kernel, daniel, eric.dumazet, davem
Cc: netdev, jiri, xiyou.wangcong
In-Reply-To: <20171207173500.5771.41198.stgit@john-Precision-Tower-5810>
This patch adds a flag for queueing disciplines to indicate the stack
does not need to use the qdisc lock to protect operations. This can
be used to build lockless scheduling algorithms and improving
performance.
The flag is checked in the tx path and the qdisc lock is only taken
if it is not set. For now use a conditional if statement. Later we
could be more aggressive if it proves worthwhile and use a static key
or wrap this in a likely().
Also the lockless case drops the TCQ_F_CAN_BYPASS logic. The reason
for this is synchronizing a qlen counter across threads proves to
cost more than doing the enqueue/dequeue operations when tested with
pktgen.
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
---
include/net/sch_generic.h | 1 +
net/core/dev.c | 26 ++++++++++++++++++++++----
net/sched/sch_generic.c | 30 ++++++++++++++++++++----------
3 files changed, 43 insertions(+), 14 deletions(-)
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 65d0d25..bb806a0 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -71,6 +71,7 @@ struct Qdisc {
* qdisc_tree_decrease_qlen() should stop.
*/
#define TCQ_F_INVISIBLE 0x80 /* invisible by default in dump */
+#define TCQ_F_NOLOCK 0x100 /* qdisc does not require locking */
u32 limit;
const struct Qdisc_ops *ops;
struct qdisc_size_table __rcu *stab;
diff --git a/net/core/dev.c b/net/core/dev.c
index 44c7de3..e32cf5c 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3162,6 +3162,21 @@ static inline int __dev_xmit_skb(struct sk_buff *skb, struct Qdisc *q,
int rc;
qdisc_calculate_pkt_len(skb, q);
+
+ if (q->flags & TCQ_F_NOLOCK) {
+ if (unlikely(test_bit(__QDISC_STATE_DEACTIVATED, &q->state))) {
+ __qdisc_drop(skb, &to_free);
+ rc = NET_XMIT_DROP;
+ } else {
+ rc = q->enqueue(skb, q, &to_free) & NET_XMIT_MASK;
+ __qdisc_run(q);
+ }
+
+ if (unlikely(to_free))
+ kfree_skb_list(to_free);
+ return rc;
+ }
+
/*
* Heuristic to force contended enqueues to serialize on a
* separate lock before trying to get qdisc main lock.
@@ -4144,19 +4159,22 @@ static __latent_entropy void net_tx_action(struct softirq_action *h)
while (head) {
struct Qdisc *q = head;
- spinlock_t *root_lock;
+ spinlock_t *root_lock = NULL;
head = head->next_sched;
- root_lock = qdisc_lock(q);
- spin_lock(root_lock);
+ if (!(q->flags & TCQ_F_NOLOCK)) {
+ root_lock = qdisc_lock(q);
+ spin_lock(root_lock);
+ }
/* We need to make sure head->next_sched is read
* before clearing __QDISC_STATE_SCHED
*/
smp_mb__before_atomic();
clear_bit(__QDISC_STATE_SCHED, &q->state);
qdisc_run(q);
- spin_unlock(root_lock);
+ if (root_lock)
+ spin_unlock(root_lock);
}
}
}
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index f6803e1..ec757f6 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -174,7 +174,8 @@ int sch_direct_xmit(struct sk_buff *skb, struct Qdisc *q,
int ret = NETDEV_TX_BUSY;
/* And release qdisc */
- spin_unlock(root_lock);
+ if (root_lock)
+ spin_unlock(root_lock);
/* Note that we validate skb (GSO, checksum, ...) outside of locks */
if (validate)
@@ -187,10 +188,13 @@ int sch_direct_xmit(struct sk_buff *skb, struct Qdisc *q,
HARD_TX_UNLOCK(dev, txq);
} else {
- spin_lock(root_lock);
+ if (root_lock)
+ spin_lock(root_lock);
return qdisc_qlen(q);
}
- spin_lock(root_lock);
+
+ if (root_lock)
+ spin_lock(root_lock);
if (dev_xmit_complete(ret)) {
/* Driver sent out skb successfully or skb was consumed */
@@ -231,9 +235,9 @@ int sch_direct_xmit(struct sk_buff *skb, struct Qdisc *q,
*/
static inline int qdisc_restart(struct Qdisc *q, int *packets)
{
+ spinlock_t *root_lock = NULL;
struct netdev_queue *txq;
struct net_device *dev;
- spinlock_t *root_lock;
struct sk_buff *skb;
bool validate;
@@ -242,7 +246,9 @@ static inline int qdisc_restart(struct Qdisc *q, int *packets)
if (unlikely(!skb))
return 0;
- root_lock = qdisc_lock(q);
+ if (!(q->flags & TCQ_F_NOLOCK))
+ root_lock = qdisc_lock(q);
+
dev = qdisc_dev(q);
txq = skb_get_tx_queue(dev, skb);
@@ -880,14 +886,18 @@ static bool some_qdisc_is_busy(struct net_device *dev)
dev_queue = netdev_get_tx_queue(dev, i);
q = dev_queue->qdisc_sleeping;
- root_lock = qdisc_lock(q);
- spin_lock_bh(root_lock);
+ if (q->flags & TCQ_F_NOLOCK) {
+ val = test_bit(__QDISC_STATE_SCHED, &q->state);
+ } else {
+ root_lock = qdisc_lock(q);
+ spin_lock_bh(root_lock);
- val = (qdisc_is_running(q) ||
- test_bit(__QDISC_STATE_SCHED, &q->state));
+ val = (qdisc_is_running(q) ||
+ test_bit(__QDISC_STATE_SCHED, &q->state));
- spin_unlock_bh(root_lock);
+ spin_unlock_bh(root_lock);
+ }
if (val)
return true;
^ permalink raw reply related
* Re: [PATCH net] gianfar: Disable EEE autoneg by default
From: Andrew Lunn @ 2017-12-07 17:54 UTC (permalink / raw)
To: Claudiu Manoil; +Cc: netdev, David S . Miller, Shaohui Xie
In-Reply-To: <1512665063-1680-1-git-send-email-claudiu.manoil@nxp.com>
On Thu, Dec 07, 2017 at 06:44:23PM +0200, Claudiu Manoil wrote:
> This controller does not support EEE, but it may connect to a PHY
> which supports EEE and advertises EEE by default, while its link
> partner also advertises EEE. If this happens, the PHY enters low
> power mode when the traffic rate is low and causes packet loss.
> This patch disables EEE advertisement by default for any PHY that
> gianfar connects to, to prevent the above unwanted outcome.
>
> Signed-off-by: Shaohui Xie <Shaohui.Xie@nxp.com>
> Tested-by: Yangbo Lu <Yangbo.lu@nxp.com>
> Signed-off-by: Claudiu Manoil <claudiu.manoil@nxp.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* [net-next PATCH 01/14] net: sched: cleanup qdisc_run and __qdisc_run semantics
From: John Fastabend @ 2017-12-07 17:54 UTC (permalink / raw)
To: willemdebruijn.kernel, daniel, eric.dumazet, davem
Cc: netdev, jiri, xiyou.wangcong
In-Reply-To: <20171207173500.5771.41198.stgit@john-Precision-Tower-5810>
Currently __qdisc_run calls qdisc_run_end() but does not call
qdisc_run_begin(). This makes it hard to track pairs of
qdisc_run_{begin,end} across function calls.
To simplify reading these code paths this patch moves begin/end calls
into qdisc_run().
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
---
include/net/pkt_sched.h | 4 +++-
net/core/dev.c | 5 +++--
net/sched/sch_generic.c | 2 --
3 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/include/net/pkt_sched.h b/include/net/pkt_sched.h
index d1f413f..4eea719 100644
--- a/include/net/pkt_sched.h
+++ b/include/net/pkt_sched.h
@@ -113,8 +113,10 @@ int sch_direct_xmit(struct sk_buff *skb, struct Qdisc *q,
static inline void qdisc_run(struct Qdisc *q)
{
- if (qdisc_run_begin(q))
+ if (qdisc_run_begin(q)) {
__qdisc_run(q);
+ qdisc_run_end(q);
+ }
}
static inline __be16 tc_skb_protocol(const struct sk_buff *skb)
diff --git a/net/core/dev.c b/net/core/dev.c
index 6bea893..44c7de3 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3192,9 +3192,9 @@ static inline int __dev_xmit_skb(struct sk_buff *skb, struct Qdisc *q,
contended = false;
}
__qdisc_run(q);
- } else
- qdisc_run_end(q);
+ }
+ qdisc_run_end(q);
rc = NET_XMIT_SUCCESS;
} else {
rc = q->enqueue(skb, q, &to_free) & NET_XMIT_MASK;
@@ -3204,6 +3204,7 @@ static inline int __dev_xmit_skb(struct sk_buff *skb, struct Qdisc *q,
contended = false;
}
__qdisc_run(q);
+ qdisc_run_end(q);
}
}
spin_unlock(root_lock);
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 3839cbb..f6803e1 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -266,8 +266,6 @@ void __qdisc_run(struct Qdisc *q)
break;
}
}
-
- qdisc_run_end(q);
}
unsigned long dev_trans_start(struct net_device *dev)
^ permalink raw reply related
* [net-next PATCH 00/14] lockless qdisc series
From: John Fastabend @ 2017-12-07 17:53 UTC (permalink / raw)
To: willemdebruijn.kernel, daniel, eric.dumazet, davem
Cc: netdev, jiri, xiyou.wangcong
This series adds support for building lockless qdiscs. This is
the result of noticing the qdisc lock is a common hot-spot in
perf analysis of the Linux network stack, especially when testing
with high packet per second rates. However, nothing is free and
most qdiscs rely on the qdisc lock for their data structures so
each qdisc must be converted on a case by case basis. In this
series, to kick things off, we make pfifo_fast, mq, and mqprio
lockless. Follow up series can address additional qdiscs as needed.
For example sch_tbf might be useful. To allow this the lockless
design is an opt-in flag. In some future utopia we convert all
qdiscs and we get to drop this case analysis, but in order to
make progress we live in the real-world.
There are also a handful of optimizations I have behind this
series and a few code cleanups that I couldn't figure out how
to fit neatly into this series with out increasing the patch
count. Once this is in additional patches can address this. The
most notable is in skb_dequeue we can push the consumer lock
out a bit and consume multiple skbs off the skb_array in pfifo
fast per iteration. Ideally we could push arrays of packets at
drivers as well but we would need the infrastructure for this.
The other notable improvement is to do less locking in the
overrun cases where bad tx queue list and gso_skb are being
hit. Although, nice in theory in practice this is the error
case and I haven't found a benchmark where this matters yet.
For testing...
My first test case uses multiple containers (via cilium) where
multiple client containers use 'wrk' to benchmark connections with
a server container running lighttpd. Where lighttpd is configured
to use multiple threads, one per core. Additionally this test has
a proxy agent running so all traffic takes an extra hop through a
proxy container. In these cases each TCP packet traverses the egress
qdisc layer at least four times and the ingress qdisc layer an
additional four times. This makes for a good stress test IMO, perf
details below.
The other micro-benchmark I run is injecting packets directly into
qdisc layer using pktgen. This uses the benchmark script,
./pktgen_bench_xmit_mode_queue_xmit.sh
Benchmarks taken in two cases, "base" running latest net-next no
changes to qdisc layer and "qdisc" tests run with qdisc lockless
updates. Numbers reported in req/sec. All virtual 'veth' devices
run with pfifo_fast in the qdisc test case.
`wrk -t16 -c $conns -d30 "http://[$SERVER_IP4]:80"`
conns 16 32 64 1024
-----------------------------------------------
base: 18831 20201 21393 29151
qdisc: 19309 21063 23899 29265
notice in all cases we see performance improvement when running
with qdisc case.
Microbenchmarks using pktgen are as follows,
`pktgen_bench_xmit_mode_queue_xmit.sh -t 1 -i eth2 -c 20000000
base(mq): 2.1Mpps
base(pfifo_fast): 2.1Mpps
qdisc(mq): 2.6Mpps
qdisc(pfifo_fast): 2.6Mpps
notice numbers are the same for mq and pfifo_fast because only
testing a single thread here. In both tests we see a nice bump
in performance gain. The key with 'mq' is it is already per
txq ring so contention is minimal in the above cases. Qdiscs
such as tbf or htb which have more contention will likely show
larger gains when/if lockless versions are implemented.
Thanks to everyone who helped with this work especially Daniel
Borkmann, Eric Dumazet and Willem de Bruijn for discussing the
design and reviewing versions of the code.
Changes from the RFC: dropped a couple patches off the end,
fixed a bug with skb_queue_walk_safe not unlinking skb in all
cases, fixed a lockdep splat with pfifo_fast_destroy not calling
*_bh lock variant, addressed _most_ of Willem's comments, there
was a bug in the bulk locking (final patch) of the RFC series.
@Willem, I left out lockdep annotation for a follow on series
to add lockdep more completely, rather than just in code I
touched.
Comments and feedback welcome.
Thanks,
John
---
John Fastabend (14):
net: sched: cleanup qdisc_run and __qdisc_run semantics
net: sched: allow qdiscs to handle locking
net: sched: remove remaining uses for qdisc_qlen in xmit path
net: sched: provide per cpu qstat helpers
net: sched: a dflt qdisc may be used with per cpu stats
net: sched: explicit locking in gso_cpu fallback
net: sched: drop qdisc_reset from dev_graft_qdisc
net: sched: use skb list for skb_bad_tx
net: sched: check for frozen queue before skb_bad_txq check
net: sched: helpers to sum qlen and qlen for per cpu logic
net: sched: add support for TCQ_F_NOLOCK subqueues to sch_mq
net: sched: add support for TCQ_F_NOLOCK subqueues to sch_mqprio
net: skb_array: expose peek API
net: sched: pfifo_fast use skb_array
include/linux/skb_array.h | 5 +
include/net/gen_stats.h | 3
include/net/pkt_sched.h | 10 +
include/net/sch_generic.h | 79 +++++++-
net/core/dev.c | 31 +++
net/core/gen_stats.c | 9 +
net/sched/sch_api.c | 8 +
net/sched/sch_generic.c | 440 ++++++++++++++++++++++++++++++++-------------
net/sched/sch_mq.c | 34 +++
net/sched/sch_mqprio.c | 69 +++++--
10 files changed, 512 insertions(+), 176 deletions(-)
^ permalink raw reply
* Re: [PATCH net-next 1/6] net: sched: sch_api: handle generic qdisc errors
From: David Ahern @ 2017-12-07 17:52 UTC (permalink / raw)
To: Jamal Hadi Salim, Alexander Aring, davem
Cc: xiyou.wangcong, jiri, netdev, kernel
In-Reply-To: <61d9b390-3193-3e17-ea65-30f457647eae@mojatatu.com>
On 12/7/17 5:04 AM, Jamal Hadi Salim wrote:
> On 17-12-07 12:28 AM, David Ahern wrote:
>
>>> - err = nla_parse_nested(tb, TCA_STAB_MAX, opt, stab_policy, NULL);
>>> - if (err < 0)
>>> + err = nla_parse_nested(tb, TCA_STAB_MAX, opt, stab_policy, extack);
>>> + if (err < 0) {
>>> + NL_SET_ERR_MSG(extack, "Failed to parse stab netlink msg");
>>
>> you don't want to set extack here; it should be set in nla_parse_nested
>> since it is passed as an arg.
>>
>
> Can you really have a generic message in nla_parse(_nested)? What
> would it say?
> Note: the bad attribute is saved in the bowels of nla_parsing.
sure, nla_parse only sets the bad attr arg. If you keep the above, then
it needs to be corrected - it is failing to parse the TCA_STAB nested
attribute.
>
> -
>>> stab = kmalloc(sizeof(*stab) + tsize * sizeof(u16), GFP_KERNEL);
>>> - if (!stab)
>>> + if (!stab) {
>>> + NL_SET_ERR_MSG(extack, "Failed to allocate memory for stab
>>> data");
>>
>> ENOMEM does not need a text message. Which memory allocation failed is
>> not really relevant.
>>
>
> YMMV.
> On the one hand it is useful to distinguish which allocation
> in the code path failed(if there was a bug for example).
> On the other hand you could argue an alloc failure is as dramatic
> as the "sky is falling" - doesnt matter what part of the globe it is
> falling at. If the cost of sending or not is the same why not include
> the message?
What value are the messages providing above and beyond the standard libc
strerror(errno)? In the case of ENOMEM, there is nothing in the user can
do to fix that particular command, so why bloat the code with extraneous
messages? Similarly with other errors like ENODEV -- if there is only 1
device in question AND the user specified the device then you do not
need to augment with an additional error message.
The value of extack is in explaining EINVAL, EOPNOTSUPP, or the
confusing inter-dependencies in command arguments. It is not a matter of
adding an extack message for every single error path; add messages that
provide value in helping the user.
^ permalink raw reply
* Re: [PATCH net 0/3] TCP BBR sampling fixes for loss recovery undo
From: Neal Cardwell @ 2017-12-07 17:48 UTC (permalink / raw)
To: David Miller; +Cc: Netdev, Neal Cardwell
In-Reply-To: <20171207174332.17689-1-ncardwell@google.com>
On Thu, Dec 7, 2017 at 12:43 PM, Neal Cardwell <ncardwell@google.com> wrote:
> This patch series has a few minor bug fixes for cases where spurious
> loss recoveries can trick BBR estimators into estimating that the
> available bandwidth is much lower than the true available bandwidth.
> In both cases the fix here is to just reset the estimator upon loss
> recovery undo.
>
> Neal Cardwell (3):
> tcp_bbr: record "full bw reached" decision in new full_bw_reached bit
> tcp_bbr: reset full pipe detection on loss recovery undo
> tcp_bbr: reset long-term bandwidth sampling on loss recovery undo
>
> net/ipv4/tcp_bbr.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
Sorry, I should have mentioned these patches are all stable candidates
and should have all carried a footer of:
Fixes: 0f8782ea1497 ("tcp_bbr: add BBR congestion control")
David, I can resend a v2 with that footer on each patch if you want.
neal
^ permalink raw reply
* [PATCH net 3/3] tcp_bbr: reset long-term bandwidth sampling on loss recovery undo
From: Neal Cardwell @ 2017-12-07 17:43 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Neal Cardwell
In-Reply-To: <20171207174332.17689-1-ncardwell@google.com>
Fix BBR so that upon notification of a loss recovery undo BBR resets
long-term bandwidth sampling.
Under high reordering, reordering events can be interpreted as loss.
If the reordering and spurious loss estimates are high enough, this
can cause BBR to spuriously estimate that we are seeing loss rates
high enough to trigger long-term bandwidth estimation. To avoid that
problem, this commit resets long-term bandwidth sampling on loss
recovery undo events.
Signed-off-by: Neal Cardwell <ncardwell@google.com>
Reviewed-by: Yuchung Cheng <ycheng@google.com>
Acked-by: Soheil Hassas Yeganeh <soheil@google.com>
---
net/ipv4/tcp_bbr.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/ipv4/tcp_bbr.c b/net/ipv4/tcp_bbr.c
index ab3ff14ea7f7..8322f26e770e 100644
--- a/net/ipv4/tcp_bbr.c
+++ b/net/ipv4/tcp_bbr.c
@@ -878,6 +878,7 @@ static u32 bbr_undo_cwnd(struct sock *sk)
bbr->full_bw = 0; /* spurious slow-down; reset full pipe detection */
bbr->full_bw_cnt = 0;
+ bbr_reset_lt_bw_sampling(sk);
return tcp_sk(sk)->snd_cwnd;
}
--
2.15.1.424.g9478a66081-goog
^ permalink raw reply related
* [PATCH net 2/3] tcp_bbr: reset full pipe detection on loss recovery undo
From: Neal Cardwell @ 2017-12-07 17:43 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Neal Cardwell
In-Reply-To: <20171207174332.17689-1-ncardwell@google.com>
Fix BBR so that upon notification of a loss recovery undo BBR resets
the full pipe detection (STARTUP exit) state machine.
Under high reordering, reordering events can be interpreted as loss.
If the reordering and spurious loss estimates are high enough, this
could previously cause BBR to spuriously estimate that the pipe is
full.
Since spurious loss recovery means that our overall sending will have
slowed down spuriously, this commit gives a flow more time to probe
robustly for bandwidth and decide the pipe is really full.
Signed-off-by: Neal Cardwell <ncardwell@google.com>
Reviewed-by: Yuchung Cheng <ycheng@google.com>
Acked-by: Soheil Hassas Yeganeh <soheil@google.com>
---
net/ipv4/tcp_bbr.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/net/ipv4/tcp_bbr.c b/net/ipv4/tcp_bbr.c
index 3089c956b9f9..ab3ff14ea7f7 100644
--- a/net/ipv4/tcp_bbr.c
+++ b/net/ipv4/tcp_bbr.c
@@ -874,6 +874,10 @@ static u32 bbr_sndbuf_expand(struct sock *sk)
*/
static u32 bbr_undo_cwnd(struct sock *sk)
{
+ struct bbr *bbr = inet_csk_ca(sk);
+
+ bbr->full_bw = 0; /* spurious slow-down; reset full pipe detection */
+ bbr->full_bw_cnt = 0;
return tcp_sk(sk)->snd_cwnd;
}
--
2.15.1.424.g9478a66081-goog
^ permalink raw reply related
* [PATCH net 1/3] tcp_bbr: record "full bw reached" decision in new full_bw_reached bit
From: Neal Cardwell @ 2017-12-07 17:43 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Neal Cardwell
In-Reply-To: <20171207174332.17689-1-ncardwell@google.com>
This commit records the "full bw reached" decision in a new
full_bw_reached bit. This is a pure refactor that does not change the
current behavior, but enables subsequent fixes and improvements.
In particular, this enables simple and clean fixes because the full_bw
and full_bw_cnt can be unconditionally zeroed without worrying about
forgetting that we estimated we filled the pipe in Startup. And it
enables future improvements because multiple code paths can be used
for estimating that we filled the pipe in Startup; any new code paths
only need to set this bit when they think the pipe is full.
Note that this fix intentionally reduces the width of the full_bw_cnt
counter, since we have never used the most significant bit.
Signed-off-by: Neal Cardwell <ncardwell@google.com>
Reviewed-by: Yuchung Cheng <ycheng@google.com>
Acked-by: Soheil Hassas Yeganeh <soheil@google.com>
---
net/ipv4/tcp_bbr.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/net/ipv4/tcp_bbr.c b/net/ipv4/tcp_bbr.c
index 69ee877574d0..3089c956b9f9 100644
--- a/net/ipv4/tcp_bbr.c
+++ b/net/ipv4/tcp_bbr.c
@@ -110,7 +110,8 @@ struct bbr {
u32 lt_last_lost; /* LT intvl start: tp->lost */
u32 pacing_gain:10, /* current gain for setting pacing rate */
cwnd_gain:10, /* current gain for setting cwnd */
- full_bw_cnt:3, /* number of rounds without large bw gains */
+ full_bw_reached:1, /* reached full bw in Startup? */
+ full_bw_cnt:2, /* number of rounds without large bw gains */
cycle_idx:3, /* current index in pacing_gain cycle array */
has_seen_rtt:1, /* have we seen an RTT sample yet? */
unused_b:5;
@@ -180,7 +181,7 @@ static bool bbr_full_bw_reached(const struct sock *sk)
{
const struct bbr *bbr = inet_csk_ca(sk);
- return bbr->full_bw_cnt >= bbr_full_bw_cnt;
+ return bbr->full_bw_reached;
}
/* Return the windowed max recent bandwidth sample, in pkts/uS << BW_SCALE. */
@@ -717,6 +718,7 @@ static void bbr_check_full_bw_reached(struct sock *sk,
return;
}
++bbr->full_bw_cnt;
+ bbr->full_bw_reached = bbr->full_bw_cnt >= bbr_full_bw_cnt;
}
/* If pipe is probably full, drain the queue and then enter steady-state. */
@@ -850,6 +852,7 @@ static void bbr_init(struct sock *sk)
bbr->restore_cwnd = 0;
bbr->round_start = 0;
bbr->idle_restart = 0;
+ bbr->full_bw_reached = 0;
bbr->full_bw = 0;
bbr->full_bw_cnt = 0;
bbr->cycle_mstamp = 0;
--
2.15.1.424.g9478a66081-goog
^ permalink raw reply related
* [PATCH net 0/3] TCP BBR sampling fixes for loss recovery undo
From: Neal Cardwell @ 2017-12-07 17:43 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Neal Cardwell
This patch series has a few minor bug fixes for cases where spurious
loss recoveries can trick BBR estimators into estimating that the
available bandwidth is much lower than the true available bandwidth.
In both cases the fix here is to just reset the estimator upon loss
recovery undo.
Neal Cardwell (3):
tcp_bbr: record "full bw reached" decision in new full_bw_reached bit
tcp_bbr: reset full pipe detection on loss recovery undo
tcp_bbr: reset long-term bandwidth sampling on loss recovery undo
net/ipv4/tcp_bbr.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
--
2.15.1.424.g9478a66081-goog
^ permalink raw reply
* Re: [PATCH v4.1] phylib: Add device reset GPIO support
From: Sergei Shtylyov @ 2017-12-07 17:40 UTC (permalink / raw)
To: Geert Uytterhoeven, David S . Miller, Andrew Lunn,
Florian Fainelli, Simon Horman, Magnus Damm
Cc: Rob Herring, Mark Rutland, Nicolas Ferre, Richard Leitner,
netdev-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-renesas-soc-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <69fe9a6d-4039-6f80-b80d-7dc0de31e5bf-M4DtvfQ/ZS1MRgGoP+s0PdBPR1lH4CV8@public.gmane.org>
On 12/07/2017 08:20 PM, Sergei Shtylyov wrote:
>> The PHY devices sometimes do have their reset signal (maybe even power
>> supply?) tied to some GPIO and sometimes it also does happen that a boot
>> loader does not leave it deasserted. So far this issue has been attacked
>> from (as I believe) a wrong angle: by teaching the MAC driver to manipulate
>> the GPIO in question; that solution, when applied to the device trees, led
>> to adding the PHY reset GPIO properties to the MAC device node, with one
>> exception: Cadence MACB driver which could handle the "reset-gpios" prop
>> in a PHY device subnode. I believe that the correct approach is to teach
>> the 'phylib' to get the MDIO device reset GPIO from the device tree node
>> corresponding to this device -- which this patch is doing...
>>
>> Note that I had to modify the AT803x PHY driver as it would stop working
>> otherwise -- it made use of the reset GPIO for its own purposes...
>>
>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov-M4DtvfQ/ZS1MRgGoP+s0PdBPR1lH4CV8@public.gmane.org>
>> Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> [geert: Propagate actual errors from fwnode_get_named_gpiod()]
>> [geert: Avoid destroying initial setup]
>> [geert: Consolidate GPIO descriptor acquiring code]
>> Signed-off-by: Geert Uytterhoeven <geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>
> [...]
>> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
>> index 2df7b62c1a36811e..8f8b7747c54bc478 100644
>> --- a/drivers/net/phy/mdio_bus.c
>> +++ b/drivers/net/phy/mdio_bus.c
> [...]
>> @@ -48,9 +49,26 @@
>> int mdiobus_register_device(struct mdio_device *mdiodev)
>> {
>> + struct gpio_desc *gpiod = NULL;
>> +
>> if (mdiodev->bus->mdio_map[mdiodev->addr])
>> return -EBUSY;
>> + /* Deassert the optional reset signal */
>
> Umm, but why deassert it here for such a short time?
>
>> + if (mdiodev->dev.of_node)
>> + gpiod = fwnode_get_named_gpiod(&mdiodev->dev.of_node->fwnode,
>> + "reset-gpios", 0, GPIOD_OUT_LOW,
>> + "PHY reset");
>> + if (PTR_ERR(gpiod) == -ENOENT)
>> + gpiod = NULL;
>> + else if (IS_ERR(gpiod))
>> + return PTR_ERR(gpiod);
>
> Hm, returning on error with reset deasserted?
Oops, error means we couldn't drive the GPIO at all...
The 1st question remains though...
[...]
MBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* David Lamparter
From: Алексей Болдырев @ 2017-12-07 17:30 UTC (permalink / raw)
To: netdev
And what happened to him?
^ permalink raw reply
* Re: [Intel-wired-lan] [next-queue 07/10] ixgbe: process the Rx ipsec offload
From: Alexander Duyck @ 2017-12-07 17:20 UTC (permalink / raw)
To: Shannon Nelson
Cc: intel-wired-lan, Jeff Kirsher, Steffen Klassert, Sowmini Varadhan,
Netdev
In-Reply-To: <eb6a3c29-ba18-ec42-0a95-76f0af0cd1b4@oracle.com>
On Wed, Dec 6, 2017 at 9:43 PM, Shannon Nelson
<shannon.nelson@oracle.com> wrote:
> On 12/5/2017 9:40 AM, Alexander Duyck wrote:
>>
>> On Mon, Dec 4, 2017 at 9:35 PM, Shannon Nelson
>> <shannon.nelson@oracle.com> wrote:
>>>
>>> If the chip sees and decrypts an ipsec offload, set up the skb
>>> sp pointer with the ralated SA info. Since the chip is rude
>>> enough to keep to itself the table index it used for the
>>> decryption, we have to do our own table lookup, using the
>>> hash for speed.
>>>
>>> Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com>
>>> ---
>>> drivers/net/ethernet/intel/ixgbe/ixgbe.h | 6 ++
>>> drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c | 89
>>> ++++++++++++++++++++++++++
>>> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 3 +
>>> 3 files changed, 98 insertions(+)
>>>
>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>>> b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>>> index 7e8bca7..77f07dc 100644
>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>>> @@ -1009,9 +1009,15 @@ s32 ixgbe_negotiate_fc(struct ixgbe_hw *hw, u32
>>> adv_reg, u32 lp_reg,
>>> u32 adv_sym, u32 adv_asm, u32 lp_sym, u32
>>> lp_asm);
>>> #ifdef CONFIG_XFRM_OFFLOAD
>>> void ixgbe_init_ipsec_offload(struct ixgbe_adapter *adapter);
>>> +void ixgbe_ipsec_rx(struct ixgbe_ring *rx_ring,
>>> + union ixgbe_adv_rx_desc *rx_desc,
>>> + struct sk_buff *skb);
>>> void ixgbe_ipsec_restore(struct ixgbe_adapter *adapter);
>>> #else
>>> static inline void ixgbe_init_ipsec_offload(struct ixgbe_adapter
>>> *adapter) { };
>>> +static inline void ixgbe_ipsec_rx(struct ixgbe_ring *rx_ring,
>>> + union ixgbe_adv_rx_desc *rx_desc,
>>> + struct sk_buff *skb) { };
>>> static inline void ixgbe_ipsec_restore(struct ixgbe_adapter *adapter) {
>>> };
>>> #endif /* CONFIG_XFRM_OFFLOAD */
>>> #endif /* _IXGBE_H_ */
>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>>> index b93ee7f..fd06d9b 100644
>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>>> @@ -379,6 +379,35 @@ static int ixgbe_ipsec_find_empty_idx(struct
>>> ixgbe_ipsec *ipsec, bool rxtable)
>>> }
>>>
>>> /**
>>> + * ixgbe_ipsec_find_rx_state - find the state that matches
>>> + * @ipsec: pointer to ipsec struct
>>> + * @daddr: inbound address to match
>>> + * @proto: protocol to match
>>> + * @spi: SPI to match
>>> + *
>>> + * Returns a pointer to the matching SA state information
>>> + **/
>>> +static struct xfrm_state *ixgbe_ipsec_find_rx_state(struct ixgbe_ipsec
>>> *ipsec,
>>> + __be32 daddr, u8
>>> proto,
>>> + __be32 spi)
>>> +{
>>> + struct rx_sa *rsa;
>>> + struct xfrm_state *ret = NULL;
>>> +
>>> + rcu_read_lock();
>>> + hash_for_each_possible_rcu(ipsec->rx_sa_list, rsa, hlist, spi)
>>> + if (spi == rsa->xs->id.spi &&
>>> + daddr == rsa->xs->id.daddr.a4 &&
>>> + proto == rsa->xs->id.proto) {
>>> + ret = rsa->xs;
>>> + xfrm_state_hold(ret);
>>> + break;
>>> + }
>>> + rcu_read_unlock();
>>> + return ret;
>>> +}
>>> +
>>
>>
>> You need to choose a bucket, not just walk through all buckets.
>
>
> I may be wrong, but I believe that is what is happening here, where the spi
> is the hash key. As the function description says "iterate over all
> possible objects hashing to the same bucket". Besides, I basically cribbed
> this directly from our Mellanox friends (thanks!).
You're right. I misread that.
^ permalink raw reply
* Re: [PATCH v5 2/2] sock: Move the socket inuse to namespace.
From: Eric Dumazet @ 2017-12-07 17:20 UTC (permalink / raw)
To: Tonghao Zhang, davem, xiyou.wangcong, edumazet, willemb; +Cc: netdev
In-Reply-To: <1512665148-2413-2-git-send-email-xiangxia.m.yue@gmail.com>
On Thu, 2017-12-07 at 08:45 -0800, Tonghao Zhang wrote:
> In some case, we want to know how many sockets are in use in
> different _net_ namespaces. It's a key resource metric.
>
...
> +static void sock_inuse_add(struct net *net, int val)
> +{
> + if (net->core.prot_inuse)
> + this_cpu_add(*net->core.sock_inuse, val);
> +}
This is very confusing.
Why testing net->core.prot_inuse for NULL is needed at all ?
Why not testing net->core.sock_inuse instead ?
^ permalink raw reply
* Re: [PATCH v4.1] phylib: Add device reset GPIO support
From: Sergei Shtylyov @ 2017-12-07 17:20 UTC (permalink / raw)
To: Geert Uytterhoeven, David S . Miller, Andrew Lunn,
Florian Fainelli, Simon Horman, Magnus Damm
Cc: Rob Herring, Mark Rutland, Nicolas Ferre, Richard Leitner, netdev,
devicetree, linux-renesas-soc, linux-kernel
In-Reply-To: <1512390905-28094-1-git-send-email-geert+renesas@glider.be>
Hello!
On 12/04/2017 03:35 PM, Geert Uytterhoeven wrote:
> From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>
> The PHY devices sometimes do have their reset signal (maybe even power
> supply?) tied to some GPIO and sometimes it also does happen that a boot
> loader does not leave it deasserted. So far this issue has been attacked
> from (as I believe) a wrong angle: by teaching the MAC driver to manipulate
> the GPIO in question; that solution, when applied to the device trees, led
> to adding the PHY reset GPIO properties to the MAC device node, with one
> exception: Cadence MACB driver which could handle the "reset-gpios" prop
> in a PHY device subnode. I believe that the correct approach is to teach
> the 'phylib' to get the MDIO device reset GPIO from the device tree node
> corresponding to this device -- which this patch is doing...
>
> Note that I had to modify the AT803x PHY driver as it would stop working
> otherwise -- it made use of the reset GPIO for its own purposes...
>
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> Acked-by: Rob Herring <robh@kernel.org>
> [geert: Propagate actual errors from fwnode_get_named_gpiod()]
> [geert: Avoid destroying initial setup]
> [geert: Consolidate GPIO descriptor acquiring code]
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
[...]
> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> index 2df7b62c1a36811e..8f8b7747c54bc478 100644
> --- a/drivers/net/phy/mdio_bus.c
> +++ b/drivers/net/phy/mdio_bus.c
[...]
> @@ -48,9 +49,26 @@
>
> int mdiobus_register_device(struct mdio_device *mdiodev)
> {
> + struct gpio_desc *gpiod = NULL;
> +
> if (mdiodev->bus->mdio_map[mdiodev->addr])
> return -EBUSY;
>
> + /* Deassert the optional reset signal */
Umm, but why deassert it here for such a short time?
> + if (mdiodev->dev.of_node)
> + gpiod = fwnode_get_named_gpiod(&mdiodev->dev.of_node->fwnode,
> + "reset-gpios", 0, GPIOD_OUT_LOW,
> + "PHY reset");
> + if (PTR_ERR(gpiod) == -ENOENT)
> + gpiod = NULL;
> + else if (IS_ERR(gpiod))
> + return PTR_ERR(gpiod);
Hm, returning on error with reset deasserted?
> +
> + mdiodev->reset = gpiod;
> +
> + /* Assert the reset signal again */
> + mdio_device_reset(mdiodev, 1);
> +
> mdiodev->bus->mdio_map[mdiodev->addr] = mdiodev;
>
> return 0;
[...]
MBR, Sergei
^ permalink raw reply
* [PATCH net] sfc: pass valid pointers from efx_enqueue_unwind
From: Bert Kenward @ 2017-12-07 17:18 UTC (permalink / raw)
To: Dave Miller; +Cc: netdev, linux-net-drivers, Jarod Wilson
The bytes_compl and pkts_compl pointers passed to efx_dequeue_buffers
cannot be NULL. Add a paranoid warning to check this condition and fix
the one case where they were NULL.
efx_enqueue_unwind() is called very rarely, during error handling.
Without this fix it would fail with a NULL pointer dereference in
efx_dequeue_buffer, with efx_enqueue_skb in the call stack.
Fixes: e9117e5099ea ("sfc: Firmware-Assisted TSO version 2")
Reported-by: Jarod Wilson <jarod@redhat.com>
Signed-off-by: Bert Kenward <bkenward@solarflare.com>
---
drivers/net/ethernet/sfc/tx.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/sfc/tx.c b/drivers/net/ethernet/sfc/tx.c
index 0ea7e16f2e6e..9937a2450e57 100644
--- a/drivers/net/ethernet/sfc/tx.c
+++ b/drivers/net/ethernet/sfc/tx.c
@@ -77,6 +77,7 @@ static void efx_dequeue_buffer(struct efx_tx_queue *tx_queue,
}
if (buffer->flags & EFX_TX_BUF_SKB) {
+ EFX_WARN_ON_PARANOID(!pkts_compl || !bytes_compl);
(*pkts_compl)++;
(*bytes_compl) += buffer->skb->len;
dev_consume_skb_any((struct sk_buff *)buffer->skb);
@@ -426,12 +427,14 @@ static int efx_tx_map_data(struct efx_tx_queue *tx_queue, struct sk_buff *skb,
static void efx_enqueue_unwind(struct efx_tx_queue *tx_queue)
{
struct efx_tx_buffer *buffer;
+ unsigned int bytes_compl = 0;
+ unsigned int pkts_compl = 0;
/* Work backwards until we hit the original insert pointer value */
while (tx_queue->insert_count != tx_queue->write_count) {
--tx_queue->insert_count;
buffer = __efx_tx_queue_get_insert_buffer(tx_queue);
- efx_dequeue_buffer(tx_queue, buffer, NULL, NULL);
+ efx_dequeue_buffer(tx_queue, buffer, &pkts_compl, &bytes_compl);
}
}
--
2.13.6
^ permalink raw reply related
* Re: [Intel-wired-lan] [next-queue 06/10] ixgbe: restore offloaded SAs after a reset
From: Alexander Duyck @ 2017-12-07 17:16 UTC (permalink / raw)
To: Shannon Nelson
Cc: intel-wired-lan, Jeff Kirsher, Steffen Klassert, Sowmini Varadhan,
Netdev
In-Reply-To: <90efb41d-2cab-f623-8b3d-318f3ad6e135@oracle.com>
On Wed, Dec 6, 2017 at 9:43 PM, Shannon Nelson
<shannon.nelson@oracle.com> wrote:
> On 12/5/2017 9:30 AM, Alexander Duyck wrote:
>>
>> On Mon, Dec 4, 2017 at 9:35 PM, Shannon Nelson
>> <shannon.nelson@oracle.com> wrote:
>>>
>>> On a chip reset most of the table contents are lost, so must be
>>> restored. This scans the driver's ipsec tables and restores both
>>> the filled and empty table slots to their pre-reset values.
>>>
>>> Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com>
>>> ---
>>> drivers/net/ethernet/intel/ixgbe/ixgbe.h | 2 +
>>> drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c | 53
>>> ++++++++++++++++++++++++++
>>> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 1 +
>>> 3 files changed, 56 insertions(+)
>>>
>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>>> b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>>> index 9487750..7e8bca7 100644
>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>>> @@ -1009,7 +1009,9 @@ s32 ixgbe_negotiate_fc(struct ixgbe_hw *hw, u32
>>> adv_reg, u32 lp_reg,
>>> u32 adv_sym, u32 adv_asm, u32 lp_sym, u32
>>> lp_asm);
>>> #ifdef CONFIG_XFRM_OFFLOAD
>>> void ixgbe_init_ipsec_offload(struct ixgbe_adapter *adapter);
>>> +void ixgbe_ipsec_restore(struct ixgbe_adapter *adapter);
>>> #else
>>> static inline void ixgbe_init_ipsec_offload(struct ixgbe_adapter
>>> *adapter) { };
>>> +static inline void ixgbe_ipsec_restore(struct ixgbe_adapter *adapter) {
>>> };
>>> #endif /* CONFIG_XFRM_OFFLOAD */
>>> #endif /* _IXGBE_H_ */
>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>>> index 7b01d92..b93ee7f 100644
>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>>> @@ -292,6 +292,59 @@ static void ixgbe_ipsec_start_engine(struct
>>> ixgbe_adapter *adapter)
>>> }
>>>
>>> /**
>>> + * ixgbe_ipsec_restore - restore the ipsec HW settings after a reset
>>> + * @adapter: board private structure
>>> + **/
>>> +void ixgbe_ipsec_restore(struct ixgbe_adapter *adapter)
>>> +{
>>> + struct ixgbe_ipsec *ipsec = adapter->ipsec;
>>> + struct ixgbe_hw *hw = &adapter->hw;
>>> + u32 zbuf[4] = {0, 0, 0, 0};
>>
>>
>> zbuf should be a static const.
>
>
> Yep
>
>>
>>> + int i;
>>> +
>>> + if (!(adapter->flags2 & IXGBE_FLAG2_IPSEC_ENABLED))
>>> + return;
>>> +
>>> + /* clean up the engine settings */
>>> + ixgbe_ipsec_stop_engine(adapter);
>>> +
>>> + /* start the engine */
>>> + ixgbe_ipsec_start_engine(adapter);
>>> +
>>> + /* reload the IP addrs */
>>> + for (i = 0; i < IXGBE_IPSEC_MAX_RX_IP_COUNT; i++) {
>>> + struct rx_ip_sa *ipsa = &ipsec->ip_tbl[i];
>>> +
>>> + if (ipsa->used)
>>> + ixgbe_ipsec_set_rx_ip(hw, i, ipsa->ipaddr);
>>> + else
>>> + ixgbe_ipsec_set_rx_ip(hw, i, zbuf);
>>
>>
>> If we are doing a restore do we actually need to write the zero
>> values? If we did a reset I thought you had a function that was going
>> through and zeroing everything out. If so this now becomes redundant.
>
>
> Currently ixgbe_ipsec_clear_hw_tables() only gets run at at probe. It
> should probably get run at remove as well. Doing this is a bit of safety
> paranoia, and making sure the CAM memory structures that don't get cleared
> on reset have exactly what I expect in them.
You might just move ixgbe_ipsec_clear_hw_tables into the rest logic
itself. Then it covers all cases where you would be resetting the
hardware and expecting a consistent state. It will mean writing some
registers twice during the reset but it is probably better just to
make certain everything stays in a known good state after a reset.
>>
>>> + }
>>> +
>>> + /* reload the Rx keys */
>>> + for (i = 0; i < IXGBE_IPSEC_MAX_SA_COUNT; i++) {
>>> + struct rx_sa *rsa = &ipsec->rx_tbl[i];
>>> +
>>> + if (rsa->used)
>>> + ixgbe_ipsec_set_rx_sa(hw, i, rsa->xs->id.spi,
>>> + rsa->key, rsa->salt,
>>> + rsa->mode, rsa->iptbl_ind);
>>> + else
>>> + ixgbe_ipsec_set_rx_sa(hw, i, 0, zbuf, 0, 0, 0);
>>
>>
>> same here
>>
>>> + }
>>> +
>>> + /* reload the Tx keys */
>>> + for (i = 0; i < IXGBE_IPSEC_MAX_SA_COUNT; i++) {
>>> + struct tx_sa *tsa = &ipsec->tx_tbl[i];
>>> +
>>> + if (tsa->used)
>>> + ixgbe_ipsec_set_tx_sa(hw, i, tsa->key,
>>> tsa->salt);
>>> + else
>>> + ixgbe_ipsec_set_tx_sa(hw, i, zbuf, 0);
>>
>>
>> and here
>>
>>> + }
>>> +}
>>> +
>>> +/**
>>> * ixgbe_ipsec_find_empty_idx - find the first unused security
>>> parameter index
>>> * @ipsec: pointer to ipsec struct
>>> * @rxtable: true if we need to look in the Rx table
>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>> index 01fd89b..6eabf92 100644
>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>> @@ -5347,6 +5347,7 @@ static void ixgbe_configure(struct ixgbe_adapter
>>> *adapter)
>>>
>>> ixgbe_set_rx_mode(adapter->netdev);
>>> ixgbe_restore_vlan(adapter);
>>> + ixgbe_ipsec_restore(adapter);
>>>
>>> switch (hw->mac.type) {
>>> case ixgbe_mac_82599EB:
>>> --
>>> 2.7.4
>>>
>>> _______________________________________________
>>> Intel-wired-lan mailing list
>>> Intel-wired-lan@osuosl.org
>>> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ 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