* Re: [PATCH net-next 14/18] ionic: Add Tx and Rx handling
From: Shannon Nelson @ 2019-06-26 16:49 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: netdev
In-Reply-To: <20190625170803.2a23650b@cakuba.netronome.com>
On 6/25/19 5:08 PM, Jakub Kicinski wrote:
> On Thu, 20 Jun 2019 13:24:20 -0700, Shannon Nelson wrote:
>> Add both the Tx and Rx queue setup and handling. The related
>> stats display come later. Instead of using the generic napi
>> routines used by the slow-path command, the Tx and Rx paths
>> are simplified and inlined in one file in order to get better
>> compiler optimizations.
>>
>> Signed-off-by: Shannon Nelson <snelson@pensando.io>
>> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_debugfs.c b/drivers/net/ethernet/pensando/ionic/ionic_debugfs.c
>> index 5ebfaa320edf..6dfcada9e822 100644
>> --- a/drivers/net/ethernet/pensando/ionic/ionic_debugfs.c
>> +++ b/drivers/net/ethernet/pensando/ionic/ionic_debugfs.c
>> @@ -351,6 +351,54 @@ int ionic_debugfs_add_qcq(struct lif *lif, struct qcq *qcq)
>> desc_blob);
>> }
>>
>> + if (qcq->flags & QCQ_F_TX_STATS) {
>> + stats_dentry = debugfs_create_dir("tx_stats", q_dentry);
>> + if (IS_ERR_OR_NULL(stats_dentry))
>> + return PTR_ERR(stats_dentry);
>> +
>> + debugfs_create_u64("dma_map_err", 0400, stats_dentry,
>> + &qcq->stats->tx.dma_map_err);
>> + debugfs_create_u64("pkts", 0400, stats_dentry,
>> + &qcq->stats->tx.pkts);
>> + debugfs_create_u64("bytes", 0400, stats_dentry,
>> + &qcq->stats->tx.bytes);
>> + debugfs_create_u64("clean", 0400, stats_dentry,
>> + &qcq->stats->tx.clean);
>> + debugfs_create_u64("linearize", 0400, stats_dentry,
>> + &qcq->stats->tx.linearize);
>> + debugfs_create_u64("no_csum", 0400, stats_dentry,
>> + &qcq->stats->tx.no_csum);
>> + debugfs_create_u64("csum", 0400, stats_dentry,
>> + &qcq->stats->tx.csum);
>> + debugfs_create_u64("crc32_csum", 0400, stats_dentry,
>> + &qcq->stats->tx.crc32_csum);
>> + debugfs_create_u64("tso", 0400, stats_dentry,
>> + &qcq->stats->tx.tso);
>> + debugfs_create_u64("frags", 0400, stats_dentry,
>> + &qcq->stats->tx.frags);
> I wonder why debugfs over ethtool -S?
I believe this was from early engineering, before ethtool -S had been
filled out. I'll clean that up.
>
>> +static int ionic_tx(struct queue *q, struct sk_buff *skb)
>> +{
>> + struct tx_stats *stats = q_to_tx_stats(q);
>> + int err;
>> +
>> + if (skb->ip_summed == CHECKSUM_PARTIAL)
>> + err = ionic_tx_calc_csum(q, skb);
>> + else
>> + err = ionic_tx_calc_no_csum(q, skb);
>> + if (err)
>> + return err;
>> +
>> + err = ionic_tx_skb_frags(q, skb);
>> + if (err)
>> + return err;
>> +
>> + skb_tx_timestamp(skb);
>> + stats->pkts++;
>> + stats->bytes += skb->len;
> nit: I think counting stats on completion may be a better idea,
> otherwise when you can a full ring on stop your HW counters are
> guaranteed to be different than SW counters. Am I wrong?
You are not wrong, that is how many drivers handle it. I like seeing
how much the driver was given (ethtool -S) versus how much the HW
actually pushed out (netstat -i or ip -s link show). These numbers
shouldn't be very often be very different, but it is interesting when
they are.
>
>> + ionic_txq_post(q, !netdev_xmit_more(), ionic_tx_clean, skb);
>> +
>> + return 0;
>> +}
>> +
>> +static int ionic_tx_descs_needed(struct queue *q, struct sk_buff *skb)
>> +{
>> + struct tx_stats *stats = q_to_tx_stats(q);
>> + int err;
>> +
>> + /* If TSO, need roundup(skb->len/mss) descs */
>> + if (skb_is_gso(skb))
>> + return (skb->len / skb_shinfo(skb)->gso_size) + 1;
> This doesn't look correct, are you sure you don't want
> skb_shinfo(skb)->gso_segs ?
That would probably work as well.
>
>> +
>> + /* If non-TSO, just need 1 desc and nr_frags sg elems */
>> + if (skb_shinfo(skb)->nr_frags <= IONIC_TX_MAX_SG_ELEMS)
>> + return 1;
>> +
>> + /* Too many frags, so linearize */
>> + err = skb_linearize(skb);
>> + if (err)
>> + return err;
>> +
>> + stats->linearize++;
>> +
>> + /* Need 1 desc and zero sg elems */
>> + return 1;
>> +}
>> +
>> +netdev_tx_t ionic_start_xmit(struct sk_buff *skb, struct net_device *netdev)
>> +{
>> + u16 queue_index = skb_get_queue_mapping(skb);
>> + struct lif *lif = netdev_priv(netdev);
>> + struct queue *q;
>> + int ndescs;
>> + int err;
>> +
>> + if (unlikely(!test_bit(LIF_UP, lif->state))) {
>> + dev_kfree_skb(skb);
>> + return NETDEV_TX_OK;
>> + }
> Surely you stop TX before taking the queues down?
Yes, in ionic_lif_stop()
>
>> + if (likely(lif_to_txqcq(lif, queue_index)))
>> + q = lif_to_txq(lif, queue_index);
>> + else
>> + q = lif_to_txq(lif, 0);
>> +
>> + ndescs = ionic_tx_descs_needed(q, skb);
>> + if (ndescs < 0)
>> + goto err_out_drop;
>> +
>> + if (!ionic_q_has_space(q, ndescs)) {
>> + netif_stop_subqueue(netdev, queue_index);
>> + q->stop++;
>> +
>> + /* Might race with ionic_tx_clean, check again */
>> + smp_rmb();
>> + if (ionic_q_has_space(q, ndescs)) {
>> + netif_wake_subqueue(netdev, queue_index);
>> + q->wake++;
>> + } else {
>> + return NETDEV_TX_BUSY;
>> + }
>> + }
>> +
>> + if (skb_is_gso(skb))
>> + err = ionic_tx_tso(q, skb);
>> + else
>> + err = ionic_tx(q, skb);
>> +
>> + if (err)
>> + goto err_out_drop;
>> +
>> + return NETDEV_TX_OK;
>> +
>> +err_out_drop:
>> + q->drop++;
>> + dev_kfree_skb(skb);
>> + return NETDEV_TX_OK;
>> +}
^ permalink raw reply
* Re: [PATCH] bonding: Always enable vlan tx offload
From: Jiri Pirko @ 2019-06-26 16:48 UTC (permalink / raw)
To: mirq-linux
Cc: YueHaibing, davem, sdf, jianbol, jiri, willemb, sdf, j.vosburgh,
vfalico, andy, linux-kernel, netdev
In-Reply-To: <20190626161337.GA18953@qmqm.qmqm.pl>
Wed, Jun 26, 2019 at 06:13:38PM CEST, mirq-linux@rere.qmqm.pl wrote:
>On Wed, Jun 26, 2019 at 04:08:44PM +0800, YueHaibing wrote:
>> We build vlan on top of bonding interface, which vlan offload
>> is off, bond mode is 802.3ad (LACP) and xmit_hash_policy is
>> BOND_XMIT_POLICY_ENCAP34.
>>
>> Because vlan tx offload is off, vlan tci is cleared and skb push
>> the vlan header in validate_xmit_vlan() while sending from vlan
>> devices. Then in bond_xmit_hash, __skb_flow_dissect() fails to
>> get information from protocol headers encapsulated within vlan,
>> because 'nhoff' is points to IP header, so bond hashing is based
>> on layer 2 info, which fails to distribute packets across slaves.
>>
>> This patch always enable bonding's vlan tx offload, pass the vlan
>> packets to the slave devices with vlan tci, let them to handle
>> vlan implementation.
>[...]
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index 407f4095a37a..799fc38c5c34 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -4320,12 +4320,12 @@ void bond_setup(struct net_device *bond_dev)
>> bond_dev->features |= NETIF_F_NETNS_LOCAL;
>>
>> bond_dev->hw_features = BOND_VLAN_FEATURES |
>> - NETIF_F_HW_VLAN_CTAG_TX |
>> NETIF_F_HW_VLAN_CTAG_RX |
>> NETIF_F_HW_VLAN_CTAG_FILTER;
>>
>> bond_dev->hw_features |= NETIF_F_GSO_ENCAP_ALL | NETIF_F_GSO_UDP_L4;
>> bond_dev->features |= bond_dev->hw_features;
>> + bond_dev->features |= NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_STAG_TX;
>> }
>>
>> /* Destroy a bonding device.
>>
>
>I can see that bonding driver uses dev_queue_xmit() to pass packets to
>slave links, but I can't see where in the path it does software fallback
>for devices without HW VLAN tagging. Generally drivers that don't ever
>do VLAN offload also ignore vlan_tci presence. Am I missing something
>here?
validate_xmit_skb->validate_xmit_vlan
>
>Best Regards,
>Michał Mirosław
^ permalink raw reply
* Re: [PATCH bpf-next V6 00/16] AF_XDP infrastructure improvements and mlx5e support
From: Jonathan Lemon @ 2019-06-26 16:46 UTC (permalink / raw)
To: Tariq Toukan
Cc: Alexei Starovoitov, Daniel Borkmann, bjorn.topel, Magnus Karlsson,
bpf, netdev, David S. Miller, Maxim Mikityanskiy
In-Reply-To: <1561559738-4213-1-git-send-email-tariqt@mellanox.com>
On 26 Jun 2019, at 7:35, Tariq Toukan wrote:
> This series contains improvements to the AF_XDP kernel infrastructure
> and AF_XDP support in mlx5e. The infrastructure improvements are
> required for mlx5e, but also some of them benefit to all drivers, and
> some can be useful for other drivers that want to implement AF_XDP.
>
> The performance testing was performed on a machine with the following
> configuration:
>
> - 24 cores of Intel Xeon E5-2620 v3 @ 2.40 GHz
> - Mellanox ConnectX-5 Ex with 100 Gbit/s link
>
> The results with retpoline disabled, single stream:
>
> txonly: 33.3 Mpps (21.5 Mpps with queue and app pinned to the same
> CPU)
> rxdrop: 12.2 Mpps
> l2fwd: 9.4 Mpps
>
> The results with retpoline enabled, single stream:
>
> txonly: 21.3 Mpps (14.1 Mpps with queue and app pinned to the same
> CPU)
> rxdrop: 9.9 Mpps
> l2fwd: 6.8 Mpps
>
> v2 changes:
>
> Added patches for mlx5e and addressed the comments for v1. Rebased for
> bpf-next.
>
> v3 changes:
>
> Rebased for the newer bpf-next, resolved conflicts in libbpf.
> Addressed
> Björn's comments for coding style. Fixed a bug in error handling flow
> in
> mlx5e_open_xsk.
>
> v4 changes:
>
> UAPI is not changed, XSK RX queues are exposed to the kernel. The
> lower
> half of the available amount of RX queues are regular queues, and the
> upper half are XSK RX queues. The patch "xsk: Extend channels to
> support
> combined XSK/non-XSK traffic" was dropped. The final patch was
> reworked
> accordingly.
>
> Added "net/mlx5e: Attach/detach XDP program safely", as the changes
> introduced in the XSK patch base on the stuff from this one.
>
> Added "libbpf: Support drivers with non-combined channels", which
> aligns
> the condition in libbpf with the condition in the kernel.
>
> Rebased over the newer bpf-next.
>
> v5 changes:
>
> In v4, ethtool reports the number of channels as 'combined' and the
> number of XSK RX queues as 'rx' for mlx5e. It was changed, so that
> 'rx'
> is 0, and 'combined' reports the double amount of channels if there is
> an active UMEM - to make libbpf happy.
>
> The patch for libbpf was dropped. Although it's still useful and fixes
> things, it raises some disagreement, so I'm dropping it - it's no
> longer
> useful for mlx5e anymore after the change above.
>
> v6 changes:
>
> As Maxim is out of office, I rebased the series on behalf of him,
> solved some conflicts, and re-spinned.
If this is just a re-spin, you can add back:
Tested-by: Jonathan Lemon <jonathan.lemon@gmail.com>
>
> Series generated against bpf-next commit:
> 572a6928f9e3 xdp: Make __mem_id_disconnect static
>
>
> Maxim Mikityanskiy (16):
> net/mlx5e: Attach/detach XDP program safely
> xsk: Add API to check for available entries in FQ
> xsk: Add getsockopt XDP_OPTIONS
> libbpf: Support getsockopt XDP_OPTIONS
> xsk: Change the default frame size to 4096 and allow controlling it
> xsk: Return the whole xdp_desc from xsk_umem_consume_tx
> net/mlx5e: Replace deprecated PCI_DMA_TODEVICE
> net/mlx5e: Calculate linear RX frag size considering XSK
> net/mlx5e: Allow ICO SQ to be used by multiple RQs
> net/mlx5e: Refactor struct mlx5e_xdp_info
> net/mlx5e: Share the XDP SQ for XDP_TX between RQs
> net/mlx5e: XDP_TX from UMEM support
> net/mlx5e: Consider XSK in XDP MTU limit calculation
> net/mlx5e: Encapsulate open/close queues into a function
> net/mlx5e: Move queue param structs to en/params.h
> net/mlx5e: Add XSK zero-copy support
>
> drivers/net/ethernet/intel/i40e/i40e_xsk.c | 12 +-
> drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c | 15 +-
> drivers/net/ethernet/mellanox/mlx5/core/Makefile | 2 +-
> drivers/net/ethernet/mellanox/mlx5/core/en.h | 155 ++++-
> .../net/ethernet/mellanox/mlx5/core/en/params.c | 108 +--
> .../net/ethernet/mellanox/mlx5/core/en/params.h | 118 +++-
> drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c | 231 +++++--
> drivers/net/ethernet/mellanox/mlx5/core/en/xdp.h | 36 +-
> .../ethernet/mellanox/mlx5/core/en/xsk/Makefile | 1 +
> .../net/ethernet/mellanox/mlx5/core/en/xsk/rx.c | 192 ++++++
> .../net/ethernet/mellanox/mlx5/core/en/xsk/rx.h | 27 +
> .../net/ethernet/mellanox/mlx5/core/en/xsk/setup.c | 223 +++++++
> .../net/ethernet/mellanox/mlx5/core/en/xsk/setup.h | 25 +
> .../net/ethernet/mellanox/mlx5/core/en/xsk/tx.c | 111 ++++
> .../net/ethernet/mellanox/mlx5/core/en/xsk/tx.h | 15 +
> .../net/ethernet/mellanox/mlx5/core/en/xsk/umem.c | 267 ++++++++
> .../net/ethernet/mellanox/mlx5/core/en/xsk/umem.h | 31 +
> .../net/ethernet/mellanox/mlx5/core/en_ethtool.c | 25 +-
> .../ethernet/mellanox/mlx5/core/en_fs_ethtool.c | 18 +-
> drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 730
> +++++++++++++--------
> drivers/net/ethernet/mellanox/mlx5/core/en_rep.c | 12 +-
> drivers/net/ethernet/mellanox/mlx5/core/en_rx.c | 104 ++-
> drivers/net/ethernet/mellanox/mlx5/core/en_stats.c | 115 +++-
> drivers/net/ethernet/mellanox/mlx5/core/en_stats.h | 30 +
> drivers/net/ethernet/mellanox/mlx5/core/en_txrx.c | 42 +-
> .../net/ethernet/mellanox/mlx5/core/ipoib/ipoib.c | 14 +-
> drivers/net/ethernet/mellanox/mlx5/core/wq.h | 5 -
> include/net/xdp_sock.h | 27 +-
> include/uapi/linux/if_xdp.h | 8 +
> net/xdp/xsk.c | 36 +-
> net/xdp/xsk_queue.h | 14 +
> samples/bpf/xdpsock_user.c | 44 +-
> tools/include/uapi/linux/if_xdp.h | 8 +
> tools/lib/bpf/xsk.c | 12 +
> tools/lib/bpf/xsk.h | 2 +-
> 35 files changed, 2331 insertions(+), 484 deletions(-)
> create mode 100644
> drivers/net/ethernet/mellanox/mlx5/core/en/xsk/Makefile
> create mode 100644
> drivers/net/ethernet/mellanox/mlx5/core/en/xsk/rx.c
> create mode 100644
> drivers/net/ethernet/mellanox/mlx5/core/en/xsk/rx.h
> create mode 100644
> drivers/net/ethernet/mellanox/mlx5/core/en/xsk/setup.c
> create mode 100644
> drivers/net/ethernet/mellanox/mlx5/core/en/xsk/setup.h
> create mode 100644
> drivers/net/ethernet/mellanox/mlx5/core/en/xsk/tx.c
> create mode 100644
> drivers/net/ethernet/mellanox/mlx5/core/en/xsk/tx.h
> create mode 100644
> drivers/net/ethernet/mellanox/mlx5/core/en/xsk/umem.c
> create mode 100644
> drivers/net/ethernet/mellanox/mlx5/core/en/xsk/umem.h
>
> --
> 1.8.3.1
^ permalink raw reply
* [PATCH] net/mlx5: remove redundant assignment to ret
From: Colin King @ 2019-06-26 16:44 UTC (permalink / raw)
To: Saeed Mahameed, Leon Romanovsky, David S . Miller, netdev,
linux-rdma
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
Variable ret is being initialized with a value that is never read and
ret is being re-assigned later on. The initialization is redundant
and can be removed.
Addresses-Coverity: ("Unused value")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/net/ethernet/mellanox/mlx5/core/main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c b/drivers/net/ethernet/mellanox/mlx5/core/main.c
index 072b56fda27e..dd47c6d03dad 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
@@ -1477,7 +1477,7 @@ static const struct pci_error_handlers mlx5_err_handler = {
static int mlx5_try_fast_unload(struct mlx5_core_dev *dev)
{
bool fast_teardown = false, force_teardown = false;
- int ret = 1;
+ int ret;
fast_teardown = MLX5_CAP_GEN(dev, fast_teardown);
force_teardown = MLX5_CAP_GEN(dev, force_teardown);
--
2.20.1
^ permalink raw reply related
* Re: XDP multi-buffer incl. jumbo-frames (Was: [RFC V1 net-next 1/1] net: ena: implement XDP drop support)
From: Jonathan Lemon @ 2019-06-26 16:42 UTC (permalink / raw)
To: Willem de Bruijn
Cc: Toke Høiland-Jørgensen, Jesper Dangaard Brouer,
Machulsky, Zorik, Jubran, Samih, davem, netdev, Woodhouse, David,
Matushevsky, Alexander, Bshara, Saeed, Wilson, Matt,
Liguori, Anthony, Bshara, Nafea, Tzalik, Guy, Belgazal, Netanel,
Saidi, Ali, Herrenschmidt, Benjamin, Kiyanovski, Arthur,
Daniel Borkmann, Ilias Apalodimas, Alexei Starovoitov,
Jakub Kicinski, xdp-newbies
In-Reply-To: <CA+FuTSfKnhv9rr=cDa_4m7Dd9qkEm_oabDfyvH0T0sM+fQTU=w@mail.gmail.com>
On 26 Jun 2019, at 8:20, Willem de Bruijn wrote:
> On Wed, Jun 26, 2019 at 11:01 AM Toke Høiland-Jørgensen
> <toke@redhat.com> wrote:
>>
>> Jesper Dangaard Brouer <brouer@redhat.com> writes:
>>
>>> On Wed, 26 Jun 2019 13:52:16 +0200
>>> Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>>
>>>> Jesper Dangaard Brouer <brouer@redhat.com> writes:
>>>>
>>>>> On Tue, 25 Jun 2019 03:19:22 +0000
>>>>> "Machulsky, Zorik" <zorik@amazon.com> wrote:
>>>>>
>>>>>> On 6/23/19, 7:21 AM, "Jesper Dangaard Brouer"
>>>>>> <brouer@redhat.com> wrote:
>>>>>>
>>>>>> On Sun, 23 Jun 2019 10:06:49 +0300 <sameehj@amazon.com>
>>>>>> wrote:
>>>>>>
>>>>>> > This commit implements the basic functionality of drop/pass
>>>>>> logic in the
>>>>>> > ena driver.
>>>>>>
>>>>>> Usually we require a driver to implement all the XDP return
>>>>>> codes,
>>>>>> before we accept it. But as Daniel and I discussed with
>>>>>> Zorik during
>>>>>> NetConf[1], we are going to make an exception and accept the
>>>>>> driver
>>>>>> if you also implement XDP_TX.
>>>>>>
>>>>>> As we trust that Zorik/Amazon will follow and implement
>>>>>> XDP_REDIRECT
>>>>>> later, given he/you wants AF_XDP support which requires
>>>>>> XDP_REDIRECT.
>>>>>>
>>>>>> Jesper, thanks for your comments and very helpful discussion
>>>>>> during
>>>>>> NetConf! That's the plan, as we agreed. From our side I would
>>>>>> like to
>>>>>> reiterate again the importance of multi-buffer support by xdp
>>>>>> frame.
>>>>>> We would really prefer not to see our MTU shrinking because of
>>>>>> xdp
>>>>>> support.
>>>>>
>>>>> Okay we really need to make a serious attempt to find a way to
>>>>> support
>>>>> multi-buffer packets with XDP. With the important criteria of not
>>>>> hurting performance of the single-buffer per packet design.
>>>>>
>>>>> I've created a design document[2], that I will update based on our
>>>>> discussions: [2]
>>>>> https://github.com/xdp-project/xdp-project/blob/master/areas/core/xdp-multi-buffer01-design.org
>>>>>
>>>>> The use-case that really convinced me was Eric's packet
>>>>> header-split.
>
> Thanks for starting this discussion Jesper!
>
>>>>>
>>>>>
>>>>> Lets refresh: Why XDP don't have multi-buffer support:
>>>>>
>>>>> XDP is designed for maximum performance, which is why certain
>>>>> driver-level
>>>>> use-cases were not supported, like multi-buffer packets (like
>>>>> jumbo-frames).
>>>>> As it e.g. complicated the driver RX-loop and memory model
>>>>> handling.
>>>>>
>>>>> The single buffer per packet design, is also tied into eBPF
>>>>> Direct-Access
>>>>> (DA) to packet data, which can only be allowed if the packet
>>>>> memory is in
>>>>> contiguous memory. This DA feature is essential for XDP
>>>>> performance.
>>>>>
>>>>>
>>>>> One way forward is to define that XDP only get access to the first
>>>>> packet buffer, and it cannot see subsequent buffers. For XDP_TX
>>>>> and
>>>>> XDP_REDIRECT to work then XDP still need to carry pointers (plus
>>>>> len+offset) to the other buffers, which is 16 bytes per extra
>>>>> buffer.
>>>>
>>>> Yeah, I think this would be reasonable. As long as we can have a
>>>> metadata field with the full length + still give XDP programs the
>>>> ability to truncate the packet (i.e., discard the subsequent pages)
>>>
>>> You touch upon some interesting complications already:
>>>
>>> 1. It is valuable for XDP bpf_prog to know "full" length?
>>> (if so, then we need to extend xdp ctx with info)
>>
>> Valuable, quite likely. A hard requirement, probably not (for all use
>> cases).
>
> Agreed.
>
> One common validation use would be to drop any packets whose header
> length disagrees with the actual packet length.
>
>>> But if we need to know the full length, when the first-buffer is
>>> processed. Then realize that this affect the drivers RX-loop,
>>> because
>>> then we need to "collect" all the buffers before we can know the
>>> length (although some HW provide this in first descriptor).
>>>
>>> We likely have to change drivers RX-loop anyhow, as XDP_TX and
>>> XDP_REDIRECT will also need to "collect" all buffers before the
>>> packet
>>> can be forwarded. (Although this could potentially happen later in
>>> driver loop when it meet/find the End-Of-Packet descriptor bit).
>
> Yes, this might be quite a bit of refactoring of device driver code.
>
> Should we move forward with some initial constraints, e.g., no
> XDP_REDIRECT, no "full" length and no bpf_xdp_adjust_tail?
>
> That already allows many useful programs.
>
> As long as we don't arrive at a design that cannot be extended with
> those features later.
I think collecting all frames until EOP and then processing them
at once sounds reasonable.
>>> 2. Can we even allow helper bpf_xdp_adjust_tail() ?
>>>
>>> Wouldn't it be easier to disallow a BPF-prog with this helper, when
>>> driver have configured multi-buffer?
>>
>> Easier, certainly. But then it's even easier to not implement this at
>> all ;)
>>
>>> Or will it be too restrictive, if jumbo-frame is very uncommon and
>>> only enabled because switch infra could not be changed (like Amazon
>>> case).
>
> Header-split, LRO and jumbo frame are certainly not limited to the
> Amazon case.
>
>> I think it would be preferable to support it; but maybe we can let
>> that
>> depend on how difficult it actually turns out to be to allow it?
>>
>>> Perhaps it is better to let bpf_xdp_adjust_tail() fail runtime?
>>
>> If we do disallow it, I think I'd lean towards failing the call at
>> runtime...
>
> Disagree. I'd rather have a program fail at load if it depends on
> multi-frag support while the (driver) implementation does not yet
> support it.
If all packets are collected together (like the bulk queue does), and
then
passed to XDP, this could easily be made backwards compatible. If the
XDP
program isn't 'multi-frag' aware, then each packet is just passed in
individually.
Of course, passing in the equivalent of a iovec requires some form of
loop
support on the BPF side, doesn't it?
--
Jonathan
^ permalink raw reply
* Re: [PATCH net-next 2/5] net: sched: em_ipt: set the family based on the protocol when matching
From: nikolay @ 2019-06-26 16:38 UTC (permalink / raw)
To: Eyal Birger; +Cc: netdev, roopa, pablo, xiyou.wangcong, davem, jiri, jhs
In-Reply-To: <20190626192254.2bd41a40@eyal-ubuntu>
On 26 June 2019 19:22:54 EEST, Eyal Birger <eyal.birger@gmail.com> wrote:
>On Wed, 26 Jun 2019 16:45:28 +0300
>Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
>
>> On 26/06/2019 16:33, Eyal Birger wrote:
>> > Hi Nikolay,
>> >
>> > On Wed, 26 Jun 2019 14:58:52 +0300
>> > Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
>> >
>> >> Set the family based on the protocol otherwise protocol-neutral
>> >> matches will have wrong information (e.g. NFPROTO_UNSPEC). In
>> >> preparation for using NFPROTO_UNSPEC xt matches.
>> >>
>> >> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
>> >> ---
>> >> net/sched/em_ipt.c | 4 +++-
>> >> 1 file changed, 3 insertions(+), 1 deletion(-)
>> >>
>...
>> >> - nf_hook_state_init(&state, im->hook, im->match->family,
>> >> + nf_hook_state_init(&state, im->hook, state.pf,
>> >> indev ?: skb->dev, skb->dev, NULL,
>> >> em->net, NULL);
>> >> acpar.match = im->match;
>> >
>> > I think this change is incompatible with current behavior.
>> >
>> > Consider the 'policy' match which matches the packet's xfrm state
>> > (sec_path) with the provided user space parameters. The sec_path
>> > includes information about the encapsulating packet's parameters
>> > whereas the current skb points to the encapsulated packet, and the
>> > match is done on the encapsulating packet's info.
>> >
>> > So if you have an IPv6 packet encapsulated within an IPv4 packet,
>> > the match parameters should be done using IPv4 parameters, not
>IPv6.
>> >
>> > Maybe use the packet's family only if the match family is UNSPEC?
>> >
>> > Eyal.
>> >
>>
>> Hi Eyal,
>> I see your point, I was wondering about the xfrm cases. :)
>> In such case I think we can simplify the set and do it only on UNSPEC
>> matches as you suggest.
>>
>> Maybe we should enforce the tc protocol based on the user-specified
>> nfproto at least from iproute2 otherwise people can add mismatching
>> rules (e.g. nfproto == v6, tc proto == v4).
>>
>Hi Nik,
>
>I think for iproute2 the issue is the same. For encapsulated IPv6 in
>IPv4 for example, tc proto will be IPv6 (tc sees the encapsulated
>packet after decryption) whereas nfproto will be IPv4 (policy match is
>done on the encapsulating state metadata which is IPv4).
>
>I think the part missing in iproute2 is the ability to specify
>NFPROTO_UNSPEC.
>
>Thanks,
>Eyal
Right, I answered too quickly, it makes sense to mix them for xt policy.
I also plan to add support for clsact, it should be trivial and iproute2-only
change.
Thanks,
Nik
^ permalink raw reply
* Re: XDP multi-buffer incl. jumbo-frames (Was: [RFC V1 net-next 1/1] net: ena: implement XDP drop support)
From: Jesper Dangaard Brouer @ 2019-06-26 16:36 UTC (permalink / raw)
To: Toke Høiland-Jørgensen
Cc: Machulsky, Zorik, Jubran, Samih, davem@davemloft.net,
netdev@vger.kernel.org, Woodhouse, David, Matushevsky, Alexander,
Bshara, Saeed, Wilson, Matt, Liguori, Anthony, Bshara, Nafea,
Tzalik, Guy, Belgazal, Netanel, Saidi, Ali,
Herrenschmidt, Benjamin, Kiyanovski, Arthur, Daniel Borkmann,
Ilias Apalodimas, Alexei Starovoitov, Jakub Kicinski,
xdp-newbies@vger.kernel.org, brouer
In-Reply-To: <87ef3gbcpz.fsf@toke.dk>
On Wed, 26 Jun 2019 17:14:32 +0200
Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> Jesper Dangaard Brouer <brouer@redhat.com> writes:
>
> > On Wed, 26 Jun 2019 13:52:16 +0200
> > Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> >
> >> Jesper Dangaard Brouer <brouer@redhat.com> writes:
> >>
> >> > On Tue, 25 Jun 2019 03:19:22 +0000
> >> > "Machulsky, Zorik" <zorik@amazon.com> wrote:
> >> >
> >> >> On 6/23/19, 7:21 AM, "Jesper Dangaard Brouer" <brouer@redhat.com> wrote:
> >> >>
> >> >> On Sun, 23 Jun 2019 10:06:49 +0300 <sameehj@amazon.com> wrote:
> >> >>
> >> >> > This commit implements the basic functionality of drop/pass logic in the
> >> >> > ena driver.
> >> >>
> >> >> Usually we require a driver to implement all the XDP return codes,
> >> >> before we accept it. But as Daniel and I discussed with Zorik during
> >> >> NetConf[1], we are going to make an exception and accept the driver
> >> >> if you also implement XDP_TX.
> >> >>
> >> >> As we trust that Zorik/Amazon will follow and implement XDP_REDIRECT
> >> >> later, given he/you wants AF_XDP support which requires XDP_REDIRECT.
> >> >>
> >> >> Jesper, thanks for your comments and very helpful discussion during
> >> >> NetConf! That's the plan, as we agreed. From our side I would like to
> >> >> reiterate again the importance of multi-buffer support by xdp frame.
> >> >> We would really prefer not to see our MTU shrinking because of xdp
> >> >> support.
> >> >
> >> > Okay we really need to make a serious attempt to find a way to support
> >> > multi-buffer packets with XDP. With the important criteria of not
> >> > hurting performance of the single-buffer per packet design.
> >> >
> >> > I've created a design document[2], that I will update based on our
> >> > discussions: [2] https://github.com/xdp-project/xdp-project/blob/master/areas/core/xdp-multi-buffer01-design.org
> >> >
> >> > The use-case that really convinced me was Eric's packet header-split.
> >> >
> >> >
> >> > Lets refresh: Why XDP don't have multi-buffer support:
> >> >
> >> > XDP is designed for maximum performance, which is why certain driver-level
> >> > use-cases were not supported, like multi-buffer packets (like jumbo-frames).
> >> > As it e.g. complicated the driver RX-loop and memory model handling.
> >> >
> >> > The single buffer per packet design, is also tied into eBPF Direct-Access
> >> > (DA) to packet data, which can only be allowed if the packet memory is in
> >> > contiguous memory. This DA feature is essential for XDP performance.
> >> >
> >> >
> >> > One way forward is to define that XDP only get access to the first
> >> > packet buffer, and it cannot see subsequent buffers. For XDP_TX and
> >> > XDP_REDIRECT to work then XDP still need to carry pointers (plus
> >> > len+offset) to the other buffers, which is 16 bytes per extra buffer.
> >>
> >> Yeah, I think this would be reasonable. As long as we can have a
> >> metadata field with the full length + still give XDP programs the
> >> ability to truncate the packet (i.e., discard the subsequent pages)
> >
> > You touch upon some interesting complications already:
> >
> > 1. It is valuable for XDP bpf_prog to know "full" length?
> > (if so, then we need to extend xdp ctx with info)
> >
> > But if we need to know the full length, when the first-buffer is
> > processed. Then realize that this affect the drivers RX-loop, because
> > then we need to "collect" all the buffers before we can know the
> > length (although some HW provide this in first descriptor).
> >
> > We likely have to change drivers RX-loop anyhow, as XDP_TX and
> > XDP_REDIRECT will also need to "collect" all buffers before the packet
> > can be forwarded. (Although this could potentially happen later in
> > driver loop when it meet/find the End-Of-Packet descriptor bit).
>
> A few more points (mostly thinking out loud here):
>
> - In any case we probably need to loop through the subsequent
> descriptors in all cases, right? (i.e., if we run XDP on first
> segment, and that returns DROP, the rest that are part of the packet
> still need to be discarded). So we may as well delay XDP execution
> until we have the whole packet?
For the XDP_DROP case, drivers usually have way to discard remaining
buffers/segments, to handle error cases. But it heavily depend on the
driver, how tricky/convoluted this code is...
Generally I would say it would make sense to delay XDP execution until
all buffers/segments are "collected". It would be the clean approach,
but will likely require refactoring of driver level code.
> - Will this allow us to run XDP on hardware-assembled GRO super-packets?
Big YES. This is usually called LRO or TSO packets. And yes, I also
want to support this use-case, which is also listed in [2]. If we go
down this road, this use-case is also important. (Especially related to
my alloc SKBs outside drivers[3], this is a hardware offload we must
support).
[2] https://github.com/xdp-project/xdp-project/blob/master/areas/core/xdp-multi-buffer01-design.org
[3] http://vger.kernel.org/netconf2019_files/xdp-metadata-discussion.pdf
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* Re: [PATCH net-next v2 3/4] net: sched: em_ipt: keep the user-specified nfproto and use it
From: nikolay @ 2019-06-26 16:33 UTC (permalink / raw)
To: Eyal Birger; +Cc: netdev, roopa, pablo, xiyou.wangcong, davem, jiri, jhs
In-Reply-To: <20190626191835.1e306147@eyal-ubuntu>
On 26 June 2019 19:18:35 EEST, Eyal Birger <eyal.birger@gmail.com> wrote:
>Hi Nik,
>
>On Wed, 26 Jun 2019 18:56:14 +0300
>Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
>
>> For NFPROTO_UNSPEC xt_matches there's no way to restrict the matching
>> to a specific family, in order to do so we record the user-specified
>> family and later enforce it while doing the match.
>>
>> v2: adjust changes to missing patch, was patch 04 in v1
>>
>> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
>> ---
>> net/sched/em_ipt.c | 17 +++++++++++++++--
>> 1 file changed, 15 insertions(+), 2 deletions(-)
>>
>..snip..
>> @@ -182,8 +195,8 @@ static int em_ipt_match(struct sk_buff *skb,
>> struct tcf_ematch *em, const struct em_ipt_match *im = (const void
>> *)em->data; struct xt_action_param acpar = {};
>> struct net_device *indev = NULL;
>> - u8 nfproto = im->match->family;
>> struct nf_hook_state state;
>> + u8 nfproto = im->nfproto;
>
>Maybe I'm missing something now - but it's not really clear to me now
>why keeping im->nfproto would be useful:
>
>If NFPROTO_UNSPEC was provided by userspace then the actual nfproto
>used
>will be taken from the packet, and if NFPROTO_IPV4/IPV6 was specified
>from userspace then it will equal im->match->family.
>
>Is there any case where the resulting nfproto would differ as a result
>of this patch?
>
>Otherwise the patchset looks excellent to me.
>
>Thanks!
>Eyal.
Hi,
It's needed to limit the match only to the user-specified family
for unspec xt matches. The problem is otherwise im->match->family
stays at nfproto_unspec regardless of the user choice.
Thanks for reviewing the set.
Cheers,
Nik
^ permalink raw reply
* Re: [PATCH net-next 01/10] net: stmmac: dwxgmac: Enable EDMA by default
From: David Miller @ 2019-06-26 16:33 UTC (permalink / raw)
To: Jose.Abreu
Cc: linux-kernel, netdev, Joao.Pinto, peppe.cavallaro,
alexandre.torgue
In-Reply-To: <6df599b8c2d57db9d82e42861ce897d7cf003424.1561556555.git.joabreu@synopsys.com>
From: Jose Abreu <Jose.Abreu@synopsys.com>
Date: Wed, 26 Jun 2019 15:47:35 +0200
> @@ -122,6 +122,8 @@ static void dwxgmac2_dma_axi(void __iomem *ioaddr, struct stmmac_axi *axi)
> }
>
> writel(value, ioaddr + XGMAC_DMA_SYSBUS_MODE);
> + writel(GENMASK(29, 0), ioaddr + XGMAC_TX_EDMA_CTRL);
> + writel(GENMASK(29, 0), ioaddr + XGMAC_RX_EDMA_CTRL);
> }
This mask is magic and there is no indication what the bits mean and
in particular what it means to set bits 0 -- 29
You have to document what these bits mean and thus what these register
writes actually do.
^ permalink raw reply
* Re: [PATCH bpf-next V6 00/16] AF_XDP infrastructure improvements and mlx5e support
From: Björn Töpel @ 2019-06-26 16:29 UTC (permalink / raw)
To: Tariq Toukan
Cc: Alexei Starovoitov, Daniel Borkmann, Björn Töpel,
Magnus Karlsson, bpf@vger.kernel.org, netdev@vger.kernel.org,
Jonathan Lemon, David S. Miller, Maxim Mikityanskiy
In-Reply-To: <1561559738-4213-1-git-send-email-tariqt@mellanox.com>
On Wed, 26 Jun 2019 at 16:36, Tariq Toukan <tariqt@mellanox.com> wrote:
>
> This series contains improvements to the AF_XDP kernel infrastructure
> and AF_XDP support in mlx5e. The infrastructure improvements are
> required for mlx5e, but also some of them benefit to all drivers, and
> some can be useful for other drivers that want to implement AF_XDP.
>
> The performance testing was performed on a machine with the following
> configuration:
>
> - 24 cores of Intel Xeon E5-2620 v3 @ 2.40 GHz
> - Mellanox ConnectX-5 Ex with 100 Gbit/s link
>
> The results with retpoline disabled, single stream:
>
> txonly: 33.3 Mpps (21.5 Mpps with queue and app pinned to the same CPU)
> rxdrop: 12.2 Mpps
> l2fwd: 9.4 Mpps
>
> The results with retpoline enabled, single stream:
>
> txonly: 21.3 Mpps (14.1 Mpps with queue and app pinned to the same CPU)
> rxdrop: 9.9 Mpps
> l2fwd: 6.8 Mpps
>
> v2 changes:
>
> Added patches for mlx5e and addressed the comments for v1. Rebased for
> bpf-next.
>
> v3 changes:
>
> Rebased for the newer bpf-next, resolved conflicts in libbpf. Addressed
> Björn's comments for coding style. Fixed a bug in error handling flow in
> mlx5e_open_xsk.
>
> v4 changes:
>
> UAPI is not changed, XSK RX queues are exposed to the kernel. The lower
> half of the available amount of RX queues are regular queues, and the
> upper half are XSK RX queues. The patch "xsk: Extend channels to support
> combined XSK/non-XSK traffic" was dropped. The final patch was reworked
> accordingly.
>
> Added "net/mlx5e: Attach/detach XDP program safely", as the changes
> introduced in the XSK patch base on the stuff from this one.
>
> Added "libbpf: Support drivers with non-combined channels", which aligns
> the condition in libbpf with the condition in the kernel.
>
> Rebased over the newer bpf-next.
>
> v5 changes:
>
> In v4, ethtool reports the number of channels as 'combined' and the
> number of XSK RX queues as 'rx' for mlx5e. It was changed, so that 'rx'
> is 0, and 'combined' reports the double amount of channels if there is
> an active UMEM - to make libbpf happy.
>
> The patch for libbpf was dropped. Although it's still useful and fixes
> things, it raises some disagreement, so I'm dropping it - it's no longer
> useful for mlx5e anymore after the change above.
>
> v6 changes:
>
> As Maxim is out of office, I rebased the series on behalf of him,
> solved some conflicts, and re-spinned.
>
> Series generated against bpf-next commit:
> 572a6928f9e3 xdp: Make __mem_id_disconnect static
>
Thanks Tariq, re-adding my ack for the series.
Acked-by: Björn Töpel <bjorn.topel@intel.com>
>
> Maxim Mikityanskiy (16):
> net/mlx5e: Attach/detach XDP program safely
> xsk: Add API to check for available entries in FQ
> xsk: Add getsockopt XDP_OPTIONS
> libbpf: Support getsockopt XDP_OPTIONS
> xsk: Change the default frame size to 4096 and allow controlling it
> xsk: Return the whole xdp_desc from xsk_umem_consume_tx
> net/mlx5e: Replace deprecated PCI_DMA_TODEVICE
> net/mlx5e: Calculate linear RX frag size considering XSK
> net/mlx5e: Allow ICO SQ to be used by multiple RQs
> net/mlx5e: Refactor struct mlx5e_xdp_info
> net/mlx5e: Share the XDP SQ for XDP_TX between RQs
> net/mlx5e: XDP_TX from UMEM support
> net/mlx5e: Consider XSK in XDP MTU limit calculation
> net/mlx5e: Encapsulate open/close queues into a function
> net/mlx5e: Move queue param structs to en/params.h
> net/mlx5e: Add XSK zero-copy support
>
> drivers/net/ethernet/intel/i40e/i40e_xsk.c | 12 +-
> drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c | 15 +-
> drivers/net/ethernet/mellanox/mlx5/core/Makefile | 2 +-
> drivers/net/ethernet/mellanox/mlx5/core/en.h | 155 ++++-
> .../net/ethernet/mellanox/mlx5/core/en/params.c | 108 +--
> .../net/ethernet/mellanox/mlx5/core/en/params.h | 118 +++-
> drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c | 231 +++++--
> drivers/net/ethernet/mellanox/mlx5/core/en/xdp.h | 36 +-
> .../ethernet/mellanox/mlx5/core/en/xsk/Makefile | 1 +
> .../net/ethernet/mellanox/mlx5/core/en/xsk/rx.c | 192 ++++++
> .../net/ethernet/mellanox/mlx5/core/en/xsk/rx.h | 27 +
> .../net/ethernet/mellanox/mlx5/core/en/xsk/setup.c | 223 +++++++
> .../net/ethernet/mellanox/mlx5/core/en/xsk/setup.h | 25 +
> .../net/ethernet/mellanox/mlx5/core/en/xsk/tx.c | 111 ++++
> .../net/ethernet/mellanox/mlx5/core/en/xsk/tx.h | 15 +
> .../net/ethernet/mellanox/mlx5/core/en/xsk/umem.c | 267 ++++++++
> .../net/ethernet/mellanox/mlx5/core/en/xsk/umem.h | 31 +
> .../net/ethernet/mellanox/mlx5/core/en_ethtool.c | 25 +-
> .../ethernet/mellanox/mlx5/core/en_fs_ethtool.c | 18 +-
> drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 730 +++++++++++++--------
> drivers/net/ethernet/mellanox/mlx5/core/en_rep.c | 12 +-
> drivers/net/ethernet/mellanox/mlx5/core/en_rx.c | 104 ++-
> drivers/net/ethernet/mellanox/mlx5/core/en_stats.c | 115 +++-
> drivers/net/ethernet/mellanox/mlx5/core/en_stats.h | 30 +
> drivers/net/ethernet/mellanox/mlx5/core/en_txrx.c | 42 +-
> .../net/ethernet/mellanox/mlx5/core/ipoib/ipoib.c | 14 +-
> drivers/net/ethernet/mellanox/mlx5/core/wq.h | 5 -
> include/net/xdp_sock.h | 27 +-
> include/uapi/linux/if_xdp.h | 8 +
> net/xdp/xsk.c | 36 +-
> net/xdp/xsk_queue.h | 14 +
> samples/bpf/xdpsock_user.c | 44 +-
> tools/include/uapi/linux/if_xdp.h | 8 +
> tools/lib/bpf/xsk.c | 12 +
> tools/lib/bpf/xsk.h | 2 +-
> 35 files changed, 2331 insertions(+), 484 deletions(-)
> create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/en/xsk/Makefile
> create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/en/xsk/rx.c
> create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/en/xsk/rx.h
> create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/en/xsk/setup.c
> create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/en/xsk/setup.h
> create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/en/xsk/tx.c
> create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/en/xsk/tx.h
> create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/en/xsk/umem.c
> create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/en/xsk/umem.h
>
> --
> 1.8.3.1
>
^ permalink raw reply
* Re: [PATCH net-next v3 0/8] net: aquantia: implement vlan offloads
From: David Miller @ 2019-06-26 16:26 UTC (permalink / raw)
To: Igor.Russkikh; +Cc: netdev
In-Reply-To: <cover.1561552290.git.igor.russkikh@aquantia.com>
From: Igor Russkikh <Igor.Russkikh@aquantia.com>
Date: Wed, 26 Jun 2019 12:35:30 +0000
> This patchset introduces hardware VLAN offload support and also does some
> maintenance: we replace driver version with uts version string, add
> documentation file for atlantic driver, and update maintainers
> adding Igor as a maintainer.
>
> v3: shuffle doc sections, per Andrew's comments
>
> v2: updates in doc, gpl spdx tag cleanup
This looks good to me, I'll let Andrew et al. have one last chance at re-review.
^ permalink raw reply
* Re: [PATCH net-next 2/5] net: sched: em_ipt: set the family based on the protocol when matching
From: Eyal Birger @ 2019-06-26 16:22 UTC (permalink / raw)
To: Nikolay Aleksandrov
Cc: netdev, roopa, pablo, xiyou.wangcong, davem, jiri, jhs
In-Reply-To: <9a3be271-af15-3fef-9612-7a3232d09b32@cumulusnetworks.com>
On Wed, 26 Jun 2019 16:45:28 +0300
Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
> On 26/06/2019 16:33, Eyal Birger wrote:
> > Hi Nikolay,
> >
> > On Wed, 26 Jun 2019 14:58:52 +0300
> > Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
> >
> >> Set the family based on the protocol otherwise protocol-neutral
> >> matches will have wrong information (e.g. NFPROTO_UNSPEC). In
> >> preparation for using NFPROTO_UNSPEC xt matches.
> >>
> >> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
> >> ---
> >> net/sched/em_ipt.c | 4 +++-
> >> 1 file changed, 3 insertions(+), 1 deletion(-)
> >>
...
> >> - nf_hook_state_init(&state, im->hook, im->match->family,
> >> + nf_hook_state_init(&state, im->hook, state.pf,
> >> indev ?: skb->dev, skb->dev, NULL,
> >> em->net, NULL);
> >> acpar.match = im->match;
> >
> > I think this change is incompatible with current behavior.
> >
> > Consider the 'policy' match which matches the packet's xfrm state
> > (sec_path) with the provided user space parameters. The sec_path
> > includes information about the encapsulating packet's parameters
> > whereas the current skb points to the encapsulated packet, and the
> > match is done on the encapsulating packet's info.
> >
> > So if you have an IPv6 packet encapsulated within an IPv4 packet,
> > the match parameters should be done using IPv4 parameters, not IPv6.
> >
> > Maybe use the packet's family only if the match family is UNSPEC?
> >
> > Eyal.
> >
>
> Hi Eyal,
> I see your point, I was wondering about the xfrm cases. :)
> In such case I think we can simplify the set and do it only on UNSPEC
> matches as you suggest.
>
> Maybe we should enforce the tc protocol based on the user-specified
> nfproto at least from iproute2 otherwise people can add mismatching
> rules (e.g. nfproto == v6, tc proto == v4).
>
Hi Nik,
I think for iproute2 the issue is the same. For encapsulated IPv6 in
IPv4 for example, tc proto will be IPv6 (tc sees the encapsulated
packet after decryption) whereas nfproto will be IPv4 (policy match is
done on the encapsulating state metadata which is IPv4).
I think the part missing in iproute2 is the ability to specify
NFPROTO_UNSPEC.
Thanks,
Eyal
^ permalink raw reply
* [PATCH iproute] devlink: replace print macros with functions
From: Stephen Hemminger @ 2019-06-26 16:20 UTC (permalink / raw)
To: arkadis; +Cc: netdev, Stephen Hemminger
Using functions is safer than macros, and printing is not performance
critical.
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
devlink/devlink.c | 62 ++++++++++++++++++++++++++++++++---------------
1 file changed, 42 insertions(+), 20 deletions(-)
diff --git a/devlink/devlink.c b/devlink/devlink.c
index 559f624e3666..4e277f7b0bc3 100644
--- a/devlink/devlink.c
+++ b/devlink/devlink.c
@@ -11,6 +11,7 @@
#include <stdio.h>
#include <stdlib.h>
+#include <stdarg.h>
#include <string.h>
#include <stdbool.h>
#include <unistd.h>
@@ -48,32 +49,53 @@
#define HEALTH_REPORTER_TIMESTAMP_FMT_LEN 80
static int g_new_line_count;
-
-#define pr_err(args...) fprintf(stderr, ##args)
-#define pr_out(args...) \
- do { \
- if (g_indent_newline) { \
- fprintf(stdout, "%s", g_indent_str); \
- g_indent_newline = false; \
- } \
- fprintf(stdout, ##args); \
- g_new_line_count = 0; \
- } while (0)
-
-#define pr_out_sp(num, args...) \
- do { \
- int ret = fprintf(stdout, ##args); \
- if (ret < num) \
- fprintf(stdout, "%*s", num - ret, ""); \
- g_new_line_count = 0; \
- } while (0)
-
static int g_indent_level;
static bool g_indent_newline;
+
#define INDENT_STR_STEP 2
#define INDENT_STR_MAXLEN 32
static char g_indent_str[INDENT_STR_MAXLEN + 1] = "";
+static void __attribute__((format(printf, 1, 2)))
+pr_err(const char *fmt, ...)
+{
+ va_list ap;
+
+ va_start(ap, fmt);
+ vfprintf(stderr, fmt, ap);
+ va_end(ap);
+}
+
+static void __attribute__((format(printf, 1, 2)))
+pr_out(const char *fmt, ...)
+{
+ va_list ap;
+
+ if (g_indent_newline) {
+ printf("%s", g_indent_str);
+ g_indent_newline = false;
+ }
+ va_start(ap, fmt);
+ vprintf(fmt, ap);
+ va_end(ap);
+ g_new_line_count = 0;
+}
+
+static void __attribute__((format(printf, 2, 3)))
+pr_out_sp(unsigned int num, const char *fmt, ...)
+{
+ va_list ap;
+ int ret;
+
+ va_start(ap, fmt);
+ ret = vprintf(fmt, ap);
+ va_end(ap);
+
+ if (ret < num)
+ printf("%*s", num - ret, "");
+ g_new_line_count = 0; \
+}
+
static void __pr_out_indent_inc(void)
{
if (g_indent_level + INDENT_STR_STEP > INDENT_STR_MAXLEN)
--
2.20.1
^ permalink raw reply related
* Re: [PATCH net-next v2 3/4] net: sched: em_ipt: keep the user-specified nfproto and use it
From: Eyal Birger @ 2019-06-26 16:18 UTC (permalink / raw)
To: Nikolay Aleksandrov
Cc: netdev, roopa, pablo, xiyou.wangcong, davem, jiri, jhs
In-Reply-To: <20190626155615.16639-4-nikolay@cumulusnetworks.com>
Hi Nik,
On Wed, 26 Jun 2019 18:56:14 +0300
Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
> For NFPROTO_UNSPEC xt_matches there's no way to restrict the matching
> to a specific family, in order to do so we record the user-specified
> family and later enforce it while doing the match.
>
> v2: adjust changes to missing patch, was patch 04 in v1
>
> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
> ---
> net/sched/em_ipt.c | 17 +++++++++++++++--
> 1 file changed, 15 insertions(+), 2 deletions(-)
>
..snip..
> @@ -182,8 +195,8 @@ static int em_ipt_match(struct sk_buff *skb,
> struct tcf_ematch *em, const struct em_ipt_match *im = (const void
> *)em->data; struct xt_action_param acpar = {};
> struct net_device *indev = NULL;
> - u8 nfproto = im->match->family;
> struct nf_hook_state state;
> + u8 nfproto = im->nfproto;
Maybe I'm missing something now - but it's not really clear to me now
why keeping im->nfproto would be useful:
If NFPROTO_UNSPEC was provided by userspace then the actual nfproto used
will be taken from the packet, and if NFPROTO_IPV4/IPV6 was specified
from userspace then it will equal im->match->family.
Is there any case where the resulting nfproto would differ as a result
of this patch?
Otherwise the patchset looks excellent to me.
Thanks!
Eyal.
^ permalink raw reply
* Re: [PATCH net-next 13/18] ionic: Add initial ethtool support
From: Jakub Kicinski @ 2019-06-26 16:18 UTC (permalink / raw)
To: Shannon Nelson; +Cc: netdev
In-Reply-To: <4ffa2c70-9ae7-15eb-3b21-34148de89b44@pensando.io>
On Wed, 26 Jun 2019 09:07:29 -0700, Shannon Nelson wrote:
> On 6/25/19 4:54 PM, Jakub Kicinski wrote:
> > On Thu, 20 Jun 2019 13:24:19 -0700, Shannon Nelson wrote:
> >> + running = test_bit(LIF_UP, lif->state);
> >> + if (running)
> >> + ionic_stop(netdev);
> >> +
> >> + lif->ntxq_descs = ring->tx_pending;
> >> + lif->nrxq_descs = ring->rx_pending;
> >> +
> >> + if (running)
> >> + ionic_open(netdev);
> >> + clear_bit(LIF_QUEUE_RESET, lif->state);
> >> + running = test_bit(LIF_UP, lif->state);
> >> + if (running)
> >> + ionic_stop(netdev);
> >> +
> >> + lif->nxqs = ch->combined_count;
> >> +
> >> + if (running)
> >> + ionic_open(netdev);
> >> + clear_bit(LIF_QUEUE_RESET, lif->state);
> > I think we'd rather see the drivers allocate/reserve the resources
> > first, and then perform the configuration once they are as sure as
> > possible it will succeed :( I'm not sure it's a hard requirement,
> > but I think certainly it'd be nice in new drivers.
> I think I know what you mean, but I suspect it depends upon which
> resources. I think the point of the range checking already being done
> covers what the driver is pretty sure it can handle, as early on it went
> through some sizing work to figure out the max queues, interrupts,
> filters, etc.
Yes, hopefully those don't fail.
> If we're looking at memory resources, then it may be a little harder:
> should we try to allocate a whole new set of buffers before dropping
> what we have, straining memory resources even more, or do we try to
> extend or contract what we currently have, a little more complex
> depending on layout?
>
> Interesting...
Indeed. I think whichever is simpler :) Either way we get shorter
traffic disruption and avoid the risk of "half up" interfaces..
^ permalink raw reply
* Re: [PATCH] bonding: Always enable vlan tx offload
From: mirq-linux @ 2019-06-26 16:13 UTC (permalink / raw)
To: YueHaibing
Cc: davem, sdf, jianbol, jiri, willemb, sdf, jiri, j.vosburgh,
vfalico, andy, linux-kernel, netdev
In-Reply-To: <20190626080844.20796-1-yuehaibing@huawei.com>
On Wed, Jun 26, 2019 at 04:08:44PM +0800, YueHaibing wrote:
> We build vlan on top of bonding interface, which vlan offload
> is off, bond mode is 802.3ad (LACP) and xmit_hash_policy is
> BOND_XMIT_POLICY_ENCAP34.
>
> Because vlan tx offload is off, vlan tci is cleared and skb push
> the vlan header in validate_xmit_vlan() while sending from vlan
> devices. Then in bond_xmit_hash, __skb_flow_dissect() fails to
> get information from protocol headers encapsulated within vlan,
> because 'nhoff' is points to IP header, so bond hashing is based
> on layer 2 info, which fails to distribute packets across slaves.
>
> This patch always enable bonding's vlan tx offload, pass the vlan
> packets to the slave devices with vlan tci, let them to handle
> vlan implementation.
[...]
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 407f4095a37a..799fc38c5c34 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -4320,12 +4320,12 @@ void bond_setup(struct net_device *bond_dev)
> bond_dev->features |= NETIF_F_NETNS_LOCAL;
>
> bond_dev->hw_features = BOND_VLAN_FEATURES |
> - NETIF_F_HW_VLAN_CTAG_TX |
> NETIF_F_HW_VLAN_CTAG_RX |
> NETIF_F_HW_VLAN_CTAG_FILTER;
>
> bond_dev->hw_features |= NETIF_F_GSO_ENCAP_ALL | NETIF_F_GSO_UDP_L4;
> bond_dev->features |= bond_dev->hw_features;
> + bond_dev->features |= NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_STAG_TX;
> }
>
> /* Destroy a bonding device.
>
I can see that bonding driver uses dev_queue_xmit() to pass packets to
slave links, but I can't see where in the path it does software fallback
for devices without HW VLAN tagging. Generally drivers that don't ever
do VLAN offload also ignore vlan_tci presence. Am I missing something
here?
Best Regards,
Michał Mirosław
^ permalink raw reply
* Re: [bpf-next v2 08/10] bpf: Implement bpf_prog_test_run for perf event programs
From: Stanislav Fomichev @ 2019-06-26 16:12 UTC (permalink / raw)
To: Krzesimir Nowak
Cc: netdev, Alban Crequy, Iago López Galeiras,
Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, linux-kernel, bpf
In-Reply-To: <CAGGp+cE3m1+ZWFBmjTgKFEHYVJ-L1dE=+iVUXvXCxWAxRG9YTA@mail.gmail.com>
On 06/26, Krzesimir Nowak wrote:
> On Tue, Jun 25, 2019 at 10:12 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
> >
> > On 06/25, Krzesimir Nowak wrote:
> > > As an input, test run for perf event program takes struct
> > > bpf_perf_event_data as ctx_in and struct bpf_perf_event_value as
> > > data_in. For an output, it basically ignores ctx_out and data_out.
> > >
> > > The implementation sets an instance of struct bpf_perf_event_data_kern
> > > in such a way that the BPF program reading data from context will
> > > receive what we passed to the bpf prog test run in ctx_in. Also BPF
> > > program can call bpf_perf_prog_read_value to receive what was passed
> > > in data_in.
> > >
> > > Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
> > > ---
> > > kernel/trace/bpf_trace.c | 107 ++++++++++++++++++
> > > .../bpf/verifier/perf_event_sample_period.c | 8 ++
> > > 2 files changed, 115 insertions(+)
> > >
> > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > > index c102c240bb0b..2fa49ea8a475 100644
> > > --- a/kernel/trace/bpf_trace.c
> > > +++ b/kernel/trace/bpf_trace.c
> > > @@ -16,6 +16,8 @@
> > >
> > > #include <asm/tlb.h>
> > >
> > > +#include <trace/events/bpf_test_run.h>
> > > +
> > > #include "trace_probe.h"
> > > #include "trace.h"
> > >
> > > @@ -1160,7 +1162,112 @@ const struct bpf_verifier_ops perf_event_verifier_ops = {
> > > .convert_ctx_access = pe_prog_convert_ctx_access,
> > > };
> > >
> > > +static int pe_prog_test_run(struct bpf_prog *prog,
> > > + const union bpf_attr *kattr,
> > > + union bpf_attr __user *uattr)
> > > +{
> > > + void __user *ctx_in = u64_to_user_ptr(kattr->test.ctx_in);
> > > + void __user *data_in = u64_to_user_ptr(kattr->test.data_in);
> > > + u32 data_size_in = kattr->test.data_size_in;
> > > + u32 ctx_size_in = kattr->test.ctx_size_in;
> > > + u32 repeat = kattr->test.repeat;
> > > + u32 retval = 0, duration = 0;
> > > + int err = -EINVAL;
> > > + u64 time_start, time_spent = 0;
> > > + int i;
> > > + struct perf_sample_data sample_data = {0, };
> > > + struct perf_event event = {0, };
> > > + struct bpf_perf_event_data_kern real_ctx = {0, };
> > > + struct bpf_perf_event_data fake_ctx = {0, };
> > > + struct bpf_perf_event_value value = {0, };
> > > +
> > > + if (ctx_size_in != sizeof(fake_ctx))
> > > + goto out;
> > > + if (data_size_in != sizeof(value))
> > > + goto out;
> > > +
> > > + if (copy_from_user(&fake_ctx, ctx_in, ctx_size_in)) {
> > > + err = -EFAULT;
> > > + goto out;
> > > + }
> > Move this to net/bpf/test_run.c? I have a bpf_ctx_init helper to deal
> > with ctx input, might save you some code above wrt ctx size/etc.
>
> My impression about net/bpf/test_run.c was that it was a collection of
> helpers for test runs of the network-related BPF programs, because
> they are so similar to each other. So kernel/trace/bpf_trace.c looked
> like an obvious place for the test_run implementation since other perf
> trace BPF stuff was already there.
Maybe net/bpf/test_run.c should be renamed to kernel/bpf/test_run.c?
> And about bpf_ctx_init - looks useful as it seems to me that it
> handles the scenario where the size of the ctx struct grows, but still
> allows passing older version of the struct (thus smaller) from
> userspace for compatibility. Maybe that checking and copying part of
> the function could be moved into some non-static helper function, so I
> could use it and still skip the need for allocating memory for the
> context?
You can always make bpf_ctx_init non-static and export it.
But, again, consider adding your stuff to the net/bpf/test_run.c
and exporting only pe_prog_test_run. That way you can reuse
bpf_ctx_init and bpf_test_run.
Why do you care about memory allocation though? It's a one time
operation and doesn't affect the performance measurements.
> > > + if (copy_from_user(&value, data_in, data_size_in)) {
> > > + err = -EFAULT;
> > > + goto out;
> > > + }
> > > +
> > > + real_ctx.regs = &fake_ctx.regs;
> > > + real_ctx.data = &sample_data;
> > > + real_ctx.event = &event;
> > > + perf_sample_data_init(&sample_data, fake_ctx.addr,
> > > + fake_ctx.sample_period);
> > > + event.cpu = smp_processor_id();
> > > + event.oncpu = -1;
> > > + event.state = PERF_EVENT_STATE_OFF;
> > > + local64_set(&event.count, value.counter);
> > > + event.total_time_enabled = value.enabled;
> > > + event.total_time_running = value.running;
> > > + /* make self as a leader - it is used only for checking the
> > > + * state field
> > > + */
> > > + event.group_leader = &event;
> > > +
> > > + /* slightly changed copy pasta from bpf_test_run() in
> > > + * net/bpf/test_run.c
> > > + */
> > > + if (!repeat)
> > > + repeat = 1;
> > > +
> > > + rcu_read_lock();
> > > + preempt_disable();
> > > + time_start = ktime_get_ns();
> > > + for (i = 0; i < repeat; i++) {
> > Any reason for not using bpf_test_run?
>
> Two, mostly. One was that it is a static function and my code was
> elsewhere. Second was that it does some cgroup storage setup and I'm
> not sure if the perf event BPF program needs that.
You can always make it non-static.
Regarding cgroup storage: do we care? If you can see it affecting
your performance numbers, then yes, but you can try to measure to see
if it gives you any noticeable overhead. Maybe add an argument to
bpf_test_run to skip cgroup storage stuff?
> > > + retval = BPF_PROG_RUN(prog, &real_ctx);
> > > +
> > > + if (signal_pending(current)) {
> > > + err = -EINTR;
> > > + preempt_enable();
> > > + rcu_read_unlock();
> > > + goto out;
> > > + }
> > > +
> > > + if (need_resched()) {
> > > + time_spent += ktime_get_ns() - time_start;
> > > + preempt_enable();
> > > + rcu_read_unlock();
> > > +
> > > + cond_resched();
> > > +
> > > + rcu_read_lock();
> > > + preempt_disable();
> > > + time_start = ktime_get_ns();
> > > + }
> > > + }
> > > + time_spent += ktime_get_ns() - time_start;
> > > + preempt_enable();
> > > + rcu_read_unlock();
> > > +
> > > + do_div(time_spent, repeat);
> > > + duration = time_spent > U32_MAX ? U32_MAX : (u32)time_spent;
> > > + /* end of slightly changed copy pasta from bpf_test_run() in
> > > + * net/bpf/test_run.c
> > > + */
> > > +
> > > + if (copy_to_user(&uattr->test.retval, &retval, sizeof(retval))) {
> > > + err = -EFAULT;
> > > + goto out;
> > > + }
> > > + if (copy_to_user(&uattr->test.duration, &duration, sizeof(duration))) {
> > > + err = -EFAULT;
> > > + goto out;
> > > + }
> > Can BPF program modify fake_ctx? Do we need/want to copy it back?
>
> Reading the pe_prog_is_valid_access function tells me that it's not
> possible - the only type of valid access is read. So maybe I should be
> stricter about the requirements for the data_out and ctx_out sizes
> (should be zero or return -EINVAL).
Yes, better to explicitly prohibit anything that we don't support.
> > > + err = 0;
> > > +out:
> > > + trace_bpf_test_finish(&err);
> > > + return err;
> > > +}
> > > +
> > > const struct bpf_prog_ops perf_event_prog_ops = {
> > > + .test_run = pe_prog_test_run,
> > > };
> > >
> > > static DEFINE_MUTEX(bpf_event_mutex);
> > > diff --git a/tools/testing/selftests/bpf/verifier/perf_event_sample_period.c b/tools/testing/selftests/bpf/verifier/perf_event_sample_period.c
> > > index 471c1a5950d8..16e9e5824d14 100644
> > > --- a/tools/testing/selftests/bpf/verifier/perf_event_sample_period.c
> > > +++ b/tools/testing/selftests/bpf/verifier/perf_event_sample_period.c
> > This should probably go in another patch.
>
> Yeah, I was wondering about it. These changes are here to avoid
> breaking the tests, since perf event program can actually be run now
> and the test_run for perf event required certain sizes for ctx and
> data.
You need to make sure the context is optional, that way you don't break
any existing tests out in the wild and can move those changes to
another patch.
> So, I will either move them to a separate patch or rework the test_run
> for perf event to accept the size between 0 and sizeof(struct
> something), so the changes in tests maybe will not be necessary.
>
> >
> > > @@ -13,6 +13,8 @@
> > > },
> > > .result = ACCEPT,
> > > .prog_type = BPF_PROG_TYPE_PERF_EVENT,
> > > + .ctx_len = sizeof(struct bpf_perf_event_data),
> > > + .data_len = sizeof(struct bpf_perf_event_value),
> > > },
> > > {
> > > "check bpf_perf_event_data->sample_period half load permitted",
> > > @@ -29,6 +31,8 @@
> > > },
> > > .result = ACCEPT,
> > > .prog_type = BPF_PROG_TYPE_PERF_EVENT,
> > > + .ctx_len = sizeof(struct bpf_perf_event_data),
> > > + .data_len = sizeof(struct bpf_perf_event_value),
> > > },
> > > {
> > > "check bpf_perf_event_data->sample_period word load permitted",
> > > @@ -45,6 +49,8 @@
> > > },
> > > .result = ACCEPT,
> > > .prog_type = BPF_PROG_TYPE_PERF_EVENT,
> > > + .ctx_len = sizeof(struct bpf_perf_event_data),
> > > + .data_len = sizeof(struct bpf_perf_event_value),
> > > },
> > > {
> > > "check bpf_perf_event_data->sample_period dword load permitted",
> > > @@ -56,4 +62,6 @@
> > > },
> > > .result = ACCEPT,
> > > .prog_type = BPF_PROG_TYPE_PERF_EVENT,
> > > + .ctx_len = sizeof(struct bpf_perf_event_data),
> > > + .data_len = sizeof(struct bpf_perf_event_value),
> > > },
> > > --
> > > 2.20.1
> > >
>
>
>
> --
> Kinvolk GmbH | Adalbertstr.6a, 10999 Berlin | tel: +491755589364
> Geschäftsführer/Directors: Alban Crequy, Chris Kühl, Iago López Galeiras
> Registergericht/Court of registration: Amtsgericht Charlottenburg
> Registernummer/Registration number: HRB 171414 B
> Ust-ID-Nummer/VAT ID number: DE302207000
^ permalink raw reply
* Re: [PATCH bpf-next 1/4] bpf: unprivileged BPF access via /dev/bpf
From: Song Liu @ 2019-06-26 16:10 UTC (permalink / raw)
To: Lorenz Bauer
Cc: Networking, bpf, Alexei Starovoitov, Daniel Borkmann, Kernel Team
In-Reply-To: <CACAyw9-MAXOsAz7DnCBq+32yc575TEiwm_6P-3KWKmZWmAqUfg@mail.gmail.com>
> On Jun 26, 2019, at 8:26 AM, Lorenz Bauer <lmb@cloudflare.com> wrote:
>
> On Wed, 26 Jun 2019 at 16:19, Song Liu <songliubraving@fb.com> wrote:
>>> I know nothing about the scheduler, so pardon my ignorance. Does
>>> TASK_BPF_FLAG_PERMITTED apply per user-space process, or per thread?
>>
>> It is per thread. clone() also clears the bit. I will make it more
>> clear int the commit log.
>
> In that case this is going to be very hard if not impossible to use
> from languages that
> don't allow controlling threads, aka Go. I'm sure there are other
> examples as well.
>
> Is it possible to make this per-process instead?
We can probably use CLONE_THREAD flag to differentiate clone() and
fork(). I need to read it more carefully to determine whether this is
accurate and safe.
Thanks,
Song
^ permalink raw reply
* Re: [PATCH 2/2] net: stmmac: Fix crash observed if PHY does not support EEE
From: David Miller @ 2019-06-26 16:11 UTC (permalink / raw)
To: jonathanh
Cc: peppe.cavallaro, alexandre.torgue, joabreu, netdev, linux-kernel,
linux-tegra
In-Reply-To: <20190626102322.18821-2-jonathanh@nvidia.com>
From: Jon Hunter <jonathanh@nvidia.com>
Date: Wed, 26 Jun 2019 11:23:22 +0100
> If the PHY does not support EEE mode, then a crash is observed when the
> ethernet interface is enabled. The crash occurs, because if the PHY does
> not support EEE, then although the EEE timer is never configured, it is
> still marked as enabled and so the stmmac ethernet driver is still
> trying to update the timer by calling mod_timer(). This triggers a BUG()
> in the mod_timer() because we are trying to update a timer when there is
> no callback function set because timer_setup() was never called for this
> timer.
>
> The problem is caused because we return true from the function
> stmmac_eee_init(), marking the EEE timer as enabled, even when we have
> not configured the EEE timer. Fix this by ensuring that we return false
> if the PHY does not support EEE and hence, 'eee_active' is not set.
>
> Fixes: 74371272f97f ("net: stmmac: Convert to phylink and remove phylib logic")
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
Applied.
^ permalink raw reply
* Re: [PATCH 1/2] net: stmmac: Fix possible deadlock when disabling EEE support
From: David Miller @ 2019-06-26 16:10 UTC (permalink / raw)
To: jonathanh
Cc: peppe.cavallaro, alexandre.torgue, joabreu, netdev, linux-kernel,
linux-tegra
In-Reply-To: <20190626102322.18821-1-jonathanh@nvidia.com>
From: Jon Hunter <jonathanh@nvidia.com>
Date: Wed, 26 Jun 2019 11:23:21 +0100
> When stmmac_eee_init() is called to disable EEE support, then the timer
> for EEE support is stopped and we return from the function. Prior to
> stopping the timer, a mutex was acquired but in this case it is never
> released and so could cause a deadlock. Fix this by releasing the mutex
> prior to returning from stmmax_eee_init() when stopping the EEE timer.
>
> Fixes: 74371272f97f ("net: stmmac: Convert to phylink and remove phylib logic")
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
When targetting net-next for a set of changes, make this explicit and clear
by saying "[PATCH net-next ...] ..." in your Subject lines in the future.
Applied.
^ permalink raw reply
* Re: [PATCH net] ipv6: fix suspicious RCU usage in rt6_dump_route()
From: David Miller @ 2019-06-26 16:08 UTC (permalink / raw)
To: edumazet; +Cc: netdev, eric.dumazet, sbrivio, dsahern
In-Reply-To: <20190626100528.218097-1-edumazet@google.com>
From: Eric Dumazet <edumazet@google.com>
Date: Wed, 26 Jun 2019 03:05:28 -0700
> syzbot reminded us that rt6_nh_dump_exceptions() needs to be called
> with rcu_read_lock()
>
> net/ipv6/route.c:1593 suspicious rcu_dereference_check() usage!
>
> other info that might help us debug this:
...
> Fixes: 1e47b4837f3b ("ipv6: Dump route exceptions if requested")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Stefano Brivio <sbrivio@redhat.com>
> Cc: David Ahern <dsahern@gmail.com>
Applied.
^ permalink raw reply
* Re: [PATCH net] ipv4: fix suspicious RCU usage in fib_dump_info_fnhe()
From: David Miller @ 2019-06-26 16:08 UTC (permalink / raw)
To: edumazet; +Cc: netdev, eric.dumazet, sbrivio, dsahern, syzkaller
In-Reply-To: <20190626100450.217106-1-edumazet@google.com>
From: Eric Dumazet <edumazet@google.com>
Date: Wed, 26 Jun 2019 03:04:50 -0700
> sysbot reported that we lack appropriate rcu_read_lock()
> protection in fib_dump_info_fnhe()
>
> net/ipv4/route.c:2875 suspicious rcu_dereference_check() usage!
>
> other info that might help us debug this:
...
> Fixes: ee28906fd7a1 ("ipv4: Dump route exceptions if requested")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Stefano Brivio <sbrivio@redhat.com>
> Cc: David Ahern <dsahern@gmail.com>
> Reported-by: syzbot <syzkaller@googlegroups.com>
Applied, we can adjust the 'err' initialization or whatever as a followup.
^ permalink raw reply
* Re: [PATCH net-next 13/18] ionic: Add initial ethtool support
From: Shannon Nelson @ 2019-06-26 16:07 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: netdev
In-Reply-To: <20190625165412.0e1206ce@cakuba.netronome.com>
On 6/25/19 4:54 PM, Jakub Kicinski wrote:
> On Thu, 20 Jun 2019 13:24:19 -0700, Shannon Nelson wrote:
>> + running = test_bit(LIF_UP, lif->state);
>> + if (running)
>> + ionic_stop(netdev);
>> +
>> + lif->ntxq_descs = ring->tx_pending;
>> + lif->nrxq_descs = ring->rx_pending;
>> +
>> + if (running)
>> + ionic_open(netdev);
>> + clear_bit(LIF_QUEUE_RESET, lif->state);
>> + running = test_bit(LIF_UP, lif->state);
>> + if (running)
>> + ionic_stop(netdev);
>> +
>> + lif->nxqs = ch->combined_count;
>> +
>> + if (running)
>> + ionic_open(netdev);
>> + clear_bit(LIF_QUEUE_RESET, lif->state);
> I think we'd rather see the drivers allocate/reserve the resources
> first, and then perform the configuration once they are as sure as
> possible it will succeed :( I'm not sure it's a hard requirement,
> but I think certainly it'd be nice in new drivers.
I think I know what you mean, but I suspect it depends upon which
resources. I think the point of the range checking already being done
covers what the driver is pretty sure it can handle, as early on it went
through some sizing work to figure out the max queues, interrupts,
filters, etc.
If we're looking at memory resources, then it may be a little harder:
should we try to allocate a whole new set of buffers before dropping
what we have, straining memory resources even more, or do we try to
extend or contract what we currently have, a little more complex
depending on layout?
Interesting...
sln
^ permalink raw reply
* [PATCH] team: Always enable vlan tx offload
From: YueHaibing @ 2019-06-26 16:03 UTC (permalink / raw)
To: davem, sdf, jianbol, jiri, mirq-linux, willemb, sdf, jiri,
j.vosburgh, vfalico, andy
Cc: linux-kernel, netdev, YueHaibing
In-Reply-To: <20190624135007.GA17673@nanopsycho>
We should rather have vlan_tci filled all the way down
to the transmitting netdevice and let it do the hw/sw
vlan implementation.
Suggested-by: Jiri Pirko <jiri@resnulli.us>
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
drivers/net/team/team.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
index b48006e7fa2f..a8bb25341bed 100644
--- a/drivers/net/team/team.c
+++ b/drivers/net/team/team.c
@@ -2128,12 +2128,12 @@ static void team_setup(struct net_device *dev)
dev->features |= NETIF_F_NETNS_LOCAL;
dev->hw_features = TEAM_VLAN_FEATURES |
- NETIF_F_HW_VLAN_CTAG_TX |
NETIF_F_HW_VLAN_CTAG_RX |
NETIF_F_HW_VLAN_CTAG_FILTER;
dev->hw_features |= NETIF_F_GSO_ENCAP_ALL | NETIF_F_GSO_UDP_L4;
dev->features |= dev->hw_features;
+ dev->features |= NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_STAG_TX;
}
static int team_newlink(struct net *src_net, struct net_device *dev,
--
2.20.1
^ permalink raw reply related
* Re: [PATCH net-next] can: dev: call netif_carrier_off() in register_candev()
From: David Miller @ 2019-06-26 16:03 UTC (permalink / raw)
To: rasmus.villemoes
Cc: willemdebruijn.kernel, wg, mkl, Rasmus.Villemoes, linux-can,
netdev, linux-kernel
In-Reply-To: <ff8160d4-3357-9b4f-1840-bbe46195da5a@prevas.dk>
From: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
Date: Wed, 26 Jun 2019 09:31:39 +0000
> Perhaps I've misunderstood when to use the net-next prefix - is that
> only for things that should be applied directly to the net-next
> tree?
Yes, it is.
^ 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