* Re: [PATCH v2 3/6] qedi: Add QLogic FastLinQ offload iSCSI driver framework.
From: Hannes Reinecke @ 2016-11-15 7:21 UTC (permalink / raw)
To: Rangankar, Manish, Martin K. Petersen, lduncan@suse.com,
Chris Leech
Cc: linux-scsi@vger.kernel.org, netdev@vger.kernel.org,
Dept-Eng QLogic Storage Upstream, Mintz, Yuval
In-Reply-To: <D450A21F.3537A%manish.rangankar@cavium.com>
On 11/15/2016 07:14 AM, Rangankar, Manish wrote:
> Hi Hannes,
>
> Please find the response below,
>
> On 11/11/16 10:13 PM, "Hannes Reinecke" <hare@suse.de> wrote:
>
[ .. ]
>> Please use the irq-affinity rework from Christoph here; that'll save you
>> the additional msix vectors allocation.
>
> The existing qed* driver(s) and common module (qed) framework is built on
> top of the older pci_enable_msix_*() API. The new framework requires
> re-work on the existing qed common module API. That would need
> co-ordination among other dependent drivers (e.g.: qede network driver,
> which is already in the tree). We would prefer to add this as a follow on
> (to the initial submission) effort, with additional testing done and
> submission co-ordinated across protocol drivers.
>
Ok, fair enough.
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
hare@suse.de +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
^ permalink raw reply
* RE: [PATCH net-next v5] cadence: Add LSO support.
From: Rafal Ozieblo @ 2016-11-15 7:07 UTC (permalink / raw)
To: David Miller
Cc: nicolas.ferre@atmel.com, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20161114.123038.1075174354580074536.davem@davemloft.net>
> > > If UFO is in use it should not silently disable UDP checksums.
> > >
> > > If you cannot support UFO with proper checksumming, then you cannot enable support for that feature.
> >
> > According Cadence Gigabit Ethernet MAC documentation:
> >
> > "Hardware will not calculate the UDP checksum or modify the UDP
> > checksum field. Therefore software must set a value of zero in the
> > checksum field in the UDP header (in the first payload buffer) to indicate to the receiver that the UDP datagram does not include a checksum."
> >
> > It is hardware requirement.
>
> I do not doubt that it is a hardware restriction.
>
> But I am saying that you cannot enable this feature under Linux if this is how it operates on your hardware.
Would it be good to enable UFO conditionally with some internal define? Ex.:
+#ifdef MACB_ENABLE_UFO
+#define MACB_NETIF_LSO (NETIF_F_TSO | NETIF_F_UFO)
+#else
+#define MACB_NETIF_LSO (NETIF_F_TSO)
+#endif
I could add precise comment here that ufo is possible only without checksum.
Or maybe I could enable it from module_params or device-tree (like: drivers/net/ethernet/neterion/s2io.c).
^ permalink raw reply
* Re: linux-next: net->netns_ids is used after calling idr_destroy for it
From: Andrei Vagin @ 2016-11-15 6:39 UTC (permalink / raw)
To: Nicolas Dichtel, Linux Kernel Network Developers
In-Reply-To: <CANaxB-x7FgxpAwpa-RgOvUptrCRUmAE6NFm3As+LuXUE=fHJyA@mail.gmail.com>
On Mon, Nov 14, 2016 at 10:23 PM, Andrei Vagin <avagin@gmail.com> wrote:
> Hi Nicolas,
>
> cleanup_net() calls idr_destroy(net->netns_ids) for network namespaces
> and then it calls unregister_netdevice_many() which calls
> idr_alloc(net0>netns_ids). It looks wrong, doesn't it?
Here is a report from kmemleak detector:
unreferenced object 0xffff91badb543950 (size 2096):
comm "kworker/u4:0", pid 6, jiffies 4295152553 (age 28.418s)
hex dump (first 32 bytes):
00 00 00 00 00 00 00 00 00 cb 5f df ba 91 ff ff .........._.....
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:
[<ffffffffb1865bea>] kmemleak_alloc+0x4a/0xa0
[<ffffffffb1243b38>] kmem_cache_alloc+0x128/0x280
[<ffffffffb142f5ab>] idr_layer_alloc+0x2b/0x90
[<ffffffffb142f9cd>] idr_get_empty_slot+0x34d/0x370
[<ffffffffb142fa4e>] idr_alloc+0x5e/0x110
[<ffffffffb170ac3d>] __peernet2id_alloc+0x6d/0x90
[<ffffffffb170bda5>] peernet2id_alloc+0x55/0xb0
[<ffffffffb1731216>] rtnl_fill_ifinfo+0xaa6/0x10a0
[<ffffffffb1733073>] rtmsg_ifinfo_build_skb+0x73/0xd0
[<ffffffffb17125d5>] rollback_registered_many+0x295/0x390
[<ffffffffb1712765>] unregister_netdevice_many+0x25/0x80
[<ffffffffb17138a5>] default_device_exit_batch+0x145/0x170
[<ffffffffb170ae52>] ops_exit_list.isra.4+0x52/0x60
[<ffffffffb170c17f>] cleanup_net+0x1bf/0x2a0
[<ffffffffb10b616f>] process_one_work+0x1ff/0x660
[<ffffffffb10b661e>] worker_thread+0x4e/0x480
>
> I compiled the kernel with the next patch:
> diff --git a/lib/idr.c b/lib/idr.c
> index 6098336..c0a3a32 100644
> --- a/lib/idr.c
> +++ b/lib/idr.c
> @@ -636,6 +636,8 @@ void idr_destroy(struct idr *idp)
> struct idr_layer *p = get_from_free_list(idp);
> kmem_cache_free(idr_layer_cache, p);
> }
> +
> + idp->top = 0xdeaddead;
> }
> EXPORT_SYMBOL(idr_destroy);
>
> and it crashed as expected:
>
> [ 306.974024] BUG: unable to handle kernel paging request at 00000000deade6bd
> [ 306.977724] IP: [<ffffffff8b445085>] _find_next_bit.part.0+0x15/0x70
> [ 306.978490] PGD 20dfa067 [ 306.978781] PUD 0
> [ 306.979043]
> [ 306.979230] Oops: 0000 [#1] SMP
> [ 306.979607] Modules linked in: macvlan tun bridge stp llc
> nf_conntrack_netlink udp_diag tcp_diag inet_diag netlink_diag
> af_packet_diag unix_diag binfmt_misc veth nf_conntrack_ipv4
> nf_defrag_ipv4 nf_conntrack_ipv6 nf_defrag_ipv6 xt_conntrack
> nf_conntrack nfnetlink ip6table_filter ip6_tables sunrpc ppdev
> crc32c_intel joydev virtio_balloon virtio_net i2c_piix4 parport_pc
> parport acpi_cpufreq tpm_tis tpm_tis_core tpm virtio_blk serio_raw
> virtio_pci ata_generic virtio_ring virtio pata_acpi
> [ 306.985236] CPU: 1 PID: 6 Comm: kworker/u4:0 Not tainted 4.9.0-rc5+ #91
> [ 306.986005] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
> BIOS 1.9.1-1.fc24 04/01/2014
> [ 306.987024] Workqueue: netns cleanup_net
> [ 306.987511] task: ffff8ca63cb5a540 task.stack: ffff9e3240340000
> [ 306.988207] RIP: 0010:[<ffffffff8b445085>] [<ffffffff8b445085>]
> _find_next_bit.part.0+0x15/0x70
> [ 306.989246] RSP: 0018:ffff9e3240343970 EFLAGS: 00010046
> [ 306.989871] RAX: ffffffffffffffff RBX: 0000000000000000 RCX: 0000000000000000
> [ 306.990713] RDX: 0000000000000000 RSI: 0000000000000100 RDI: 00000000deade6bd
> [ 306.991548] RBP: ffff9e3240343980 R08: ffffffffffffffff R09: ffffffffffffffff
> [ 306.992383] R10: 00000000f314d32d R11: 0000000000000000 R12: 00000000ffffffff
> [ 306.993277] R13: 00000000fffffff8 R14: 00000000deaddead R15: 0000000000000000
> [ 306.994117] FS: 0000000000000000(0000) GS:ffff8ca63fd00000(0000)
> knlGS:0000000000000000
> [ 306.995068] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 306.995744] CR2: 00000000deade6bd CR3: 0000000059aec000 CR4: 00000000000006e0
> [ 306.996586] DR0: 00000000000100a0 DR1: 0000000000000000 DR2: 0000000000000000
> [ 306.997423] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000600
> [ 306.998258] Stack:
> [ 306.998503] ffff9e3240343980 ffffffff8b44511d ffff9e32403439e0
> ffffffff8b42f819
> [ 306.999434] 0000000000000000 0208002000000007 ffff8ca6289d80c0
> ffff9e32403439f8
> [ 307.000365] 0000000000000000 000000007fffffff ffff8ca61f09b200
> ffff8ca6289d80c0
> [ 307.001296] Call Trace:
> [ 307.001594] [<ffffffff8b44511d>] ? find_next_zero_bit+0x1d/0x20
> [ 307.002307] [<ffffffff8b42f819>] idr_get_empty_slot+0x189/0x370
> [ 307.003012] [<ffffffff8b42fa5e>] idr_alloc+0x5e/0x110
> [ 307.003631] [<ffffffff8b70bd88>] ? peernet2id_alloc+0x38/0xb0
> [ 307.004321] [<ffffffff8b70ac3d>] __peernet2id_alloc+0x6d/0x90
> [ 307.005003] [<ffffffff8b70bda5>] peernet2id_alloc+0x55/0xb0
> [ 307.005673] [<ffffffff8b731216>] rtnl_fill_ifinfo+0xaa6/0x10a0
> [ 307.006368] [<ffffffff8b112458>] ? rcu_read_lock_sched_held+0x58/0x60
> [ 307.007136] [<ffffffff8b6ffe2b>] ? __alloc_skb+0x9b/0x1e0
> [ 307.007780] [<ffffffff8b733073>] rtmsg_ifinfo_build_skb+0x73/0xd0
> [ 307.008509] [<ffffffff8b7125d5>] rollback_registered_many+0x295/0x390
> [ 307.009282] [<ffffffff8b712765>] unregister_netdevice_many+0x25/0x80
> [ 307.010047] [<ffffffff8b7138a5>] default_device_exit_batch+0x145/0x170
> [ 307.010825] [<ffffffff8b0e7b10>] ? finish_wait+0x70/0x70
> [ 307.011465] [<ffffffff8b70ae52>] ops_exit_list.isra.4+0x52/0x60
> [ 307.012175] [<ffffffff8b70c17f>] cleanup_net+0x1bf/0x2a0
> [ 307.012811] [<ffffffff8b0b616f>] process_one_work+0x1ff/0x660
> [ 307.013548] [<ffffffff8b0b60f4>] ? process_one_work+0x184/0x660
> [ 307.014259] [<ffffffff8b0b661e>] worker_thread+0x4e/0x480
> [ 307.014906] [<ffffffff8b0b65d0>] ? process_one_work+0x660/0x660
> [ 307.015617] [<ffffffff8b0bd2a4>] kthread+0xf4/0x110
> [ 307.016209] [<ffffffff8b0bd1b0>] ? kthread_park+0x60/0x60
> [ 307.016857] [<ffffffff8b872efa>] ret_from_fork+0x2a/0x40
^ permalink raw reply
* linux-next: net->netns_ids is used after calling idr_destroy for it
From: Andrei Vagin @ 2016-11-15 6:23 UTC (permalink / raw)
To: Nicolas Dichtel, Linux Kernel Network Developers
Hi Nicolas,
cleanup_net() calls idr_destroy(net->netns_ids) for network namespaces
and then it calls unregister_netdevice_many() which calls
idr_alloc(net0>netns_ids). It looks wrong, doesn't it?
I compiled the kernel with the next patch:
diff --git a/lib/idr.c b/lib/idr.c
index 6098336..c0a3a32 100644
--- a/lib/idr.c
+++ b/lib/idr.c
@@ -636,6 +636,8 @@ void idr_destroy(struct idr *idp)
struct idr_layer *p = get_from_free_list(idp);
kmem_cache_free(idr_layer_cache, p);
}
+
+ idp->top = 0xdeaddead;
}
EXPORT_SYMBOL(idr_destroy);
and it crashed as expected:
[ 306.974024] BUG: unable to handle kernel paging request at 00000000deade6bd
[ 306.977724] IP: [<ffffffff8b445085>] _find_next_bit.part.0+0x15/0x70
[ 306.978490] PGD 20dfa067 [ 306.978781] PUD 0
[ 306.979043]
[ 306.979230] Oops: 0000 [#1] SMP
[ 306.979607] Modules linked in: macvlan tun bridge stp llc
nf_conntrack_netlink udp_diag tcp_diag inet_diag netlink_diag
af_packet_diag unix_diag binfmt_misc veth nf_conntrack_ipv4
nf_defrag_ipv4 nf_conntrack_ipv6 nf_defrag_ipv6 xt_conntrack
nf_conntrack nfnetlink ip6table_filter ip6_tables sunrpc ppdev
crc32c_intel joydev virtio_balloon virtio_net i2c_piix4 parport_pc
parport acpi_cpufreq tpm_tis tpm_tis_core tpm virtio_blk serio_raw
virtio_pci ata_generic virtio_ring virtio pata_acpi
[ 306.985236] CPU: 1 PID: 6 Comm: kworker/u4:0 Not tainted 4.9.0-rc5+ #91
[ 306.986005] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
BIOS 1.9.1-1.fc24 04/01/2014
[ 306.987024] Workqueue: netns cleanup_net
[ 306.987511] task: ffff8ca63cb5a540 task.stack: ffff9e3240340000
[ 306.988207] RIP: 0010:[<ffffffff8b445085>] [<ffffffff8b445085>]
_find_next_bit.part.0+0x15/0x70
[ 306.989246] RSP: 0018:ffff9e3240343970 EFLAGS: 00010046
[ 306.989871] RAX: ffffffffffffffff RBX: 0000000000000000 RCX: 0000000000000000
[ 306.990713] RDX: 0000000000000000 RSI: 0000000000000100 RDI: 00000000deade6bd
[ 306.991548] RBP: ffff9e3240343980 R08: ffffffffffffffff R09: ffffffffffffffff
[ 306.992383] R10: 00000000f314d32d R11: 0000000000000000 R12: 00000000ffffffff
[ 306.993277] R13: 00000000fffffff8 R14: 00000000deaddead R15: 0000000000000000
[ 306.994117] FS: 0000000000000000(0000) GS:ffff8ca63fd00000(0000)
knlGS:0000000000000000
[ 306.995068] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 306.995744] CR2: 00000000deade6bd CR3: 0000000059aec000 CR4: 00000000000006e0
[ 306.996586] DR0: 00000000000100a0 DR1: 0000000000000000 DR2: 0000000000000000
[ 306.997423] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000600
[ 306.998258] Stack:
[ 306.998503] ffff9e3240343980 ffffffff8b44511d ffff9e32403439e0
ffffffff8b42f819
[ 306.999434] 0000000000000000 0208002000000007 ffff8ca6289d80c0
ffff9e32403439f8
[ 307.000365] 0000000000000000 000000007fffffff ffff8ca61f09b200
ffff8ca6289d80c0
[ 307.001296] Call Trace:
[ 307.001594] [<ffffffff8b44511d>] ? find_next_zero_bit+0x1d/0x20
[ 307.002307] [<ffffffff8b42f819>] idr_get_empty_slot+0x189/0x370
[ 307.003012] [<ffffffff8b42fa5e>] idr_alloc+0x5e/0x110
[ 307.003631] [<ffffffff8b70bd88>] ? peernet2id_alloc+0x38/0xb0
[ 307.004321] [<ffffffff8b70ac3d>] __peernet2id_alloc+0x6d/0x90
[ 307.005003] [<ffffffff8b70bda5>] peernet2id_alloc+0x55/0xb0
[ 307.005673] [<ffffffff8b731216>] rtnl_fill_ifinfo+0xaa6/0x10a0
[ 307.006368] [<ffffffff8b112458>] ? rcu_read_lock_sched_held+0x58/0x60
[ 307.007136] [<ffffffff8b6ffe2b>] ? __alloc_skb+0x9b/0x1e0
[ 307.007780] [<ffffffff8b733073>] rtmsg_ifinfo_build_skb+0x73/0xd0
[ 307.008509] [<ffffffff8b7125d5>] rollback_registered_many+0x295/0x390
[ 307.009282] [<ffffffff8b712765>] unregister_netdevice_many+0x25/0x80
[ 307.010047] [<ffffffff8b7138a5>] default_device_exit_batch+0x145/0x170
[ 307.010825] [<ffffffff8b0e7b10>] ? finish_wait+0x70/0x70
[ 307.011465] [<ffffffff8b70ae52>] ops_exit_list.isra.4+0x52/0x60
[ 307.012175] [<ffffffff8b70c17f>] cleanup_net+0x1bf/0x2a0
[ 307.012811] [<ffffffff8b0b616f>] process_one_work+0x1ff/0x660
[ 307.013548] [<ffffffff8b0b60f4>] ? process_one_work+0x184/0x660
[ 307.014259] [<ffffffff8b0b661e>] worker_thread+0x4e/0x480
[ 307.014906] [<ffffffff8b0b65d0>] ? process_one_work+0x660/0x660
[ 307.015617] [<ffffffff8b0bd2a4>] kthread+0xf4/0x110
[ 307.016209] [<ffffffff8b0bd1b0>] ? kthread_park+0x60/0x60
[ 307.016857] [<ffffffff8b872efa>] ret_from_fork+0x2a/0x40
^ permalink raw reply related
* Re: [PATCH v2 3/6] qedi: Add QLogic FastLinQ offload iSCSI driver framework.
From: Rangankar, Manish @ 2016-11-15 6:14 UTC (permalink / raw)
To: Martin K. Petersen, lduncan@suse.com, Chris Leech,
Hannes Reinecke
Cc: linux-scsi@vger.kernel.org, netdev@vger.kernel.org,
Dept-Eng QLogic Storage Upstream, Mintz, Yuval
In-Reply-To: <d5abc303-1514-c521-390c-adfdd0e204af@suse.de>
Hi Hannes,
Please find the response below,
On 11/11/16 10:13 PM, "Hannes Reinecke" <hare@suse.de> wrote:
>On 11/08/2016 07:57 AM, Manish Rangankar wrote:
>> The QLogic FastLinQ Driver for iSCSI (qedi) is the iSCSI specific module
>> for 41000 Series Converged Network Adapters by QLogic.
>>
>> This patch consists of following changes:
>> - MAINTAINERS Makefile and Kconfig changes for qedi,
>> - PCI driver registration,
>> - iSCSI host level initialization,
>> - Debugfs and log level infrastructure.
>>
>> Signed-off-by: Nilesh Javali <nilesh.javali@cavium.com>
>> Signed-off-by: Adheer Chandravanshi <adheer.chandravanshi@qlogic.com>
>> Signed-off-by: Chad Dupuis <chad.dupuis@cavium.com>
>> Signed-off-by: Saurav Kashyap <saurav.kashyap@cavium.com>
>> Signed-off-by: Arun Easi <arun.easi@cavium.com>
>> Signed-off-by: Manish Rangankar <manish.rangankar@cavium.com>
>> ---
>> MAINTAINERS | 6 +
>> drivers/net/ethernet/qlogic/Kconfig | 12 -
>> drivers/scsi/Kconfig | 1 +
>> drivers/scsi/Makefile | 1 +
>> drivers/scsi/qedi/Kconfig | 10 +
>> drivers/scsi/qedi/Makefile | 5 +
>> drivers/scsi/qedi/qedi.h | 291 +++++++
>> drivers/scsi/qedi/qedi_dbg.c | 143 ++++
>> drivers/scsi/qedi/qedi_dbg.h | 144 ++++
>> drivers/scsi/qedi/qedi_debugfs.c | 244 ++++++
>> drivers/scsi/qedi/qedi_hsi.h | 52 ++
>> drivers/scsi/qedi/qedi_main.c | 1616
>>+++++++++++++++++++++++++++++++++++
>> drivers/scsi/qedi/qedi_sysfs.c | 52 ++
>> drivers/scsi/qedi/qedi_version.h | 14 +
>> 14 files changed, 2579 insertions(+), 12 deletions(-)
>> create mode 100644 drivers/scsi/qedi/Kconfig
>> create mode 100644 drivers/scsi/qedi/Makefile
>> create mode 100644 drivers/scsi/qedi/qedi.h
>> create mode 100644 drivers/scsi/qedi/qedi_dbg.c
>> create mode 100644 drivers/scsi/qedi/qedi_dbg.h
>> create mode 100644 drivers/scsi/qedi/qedi_debugfs.c
>> create mode 100644 drivers/scsi/qedi/qedi_hsi.h
>> create mode 100644 drivers/scsi/qedi/qedi_main.c
>> create mode 100644 drivers/scsi/qedi/qedi_sysfs.c
>> create mode 100644 drivers/scsi/qedi/qedi_version.h
>>
[...]
>>
>> +static enum qed_int_mode qedi_int_mode_to_enum(void)
>> +{
>> + switch (int_mode) {
>> + case 0: return QED_INT_MODE_MSIX;
>> + case 1: return QED_INT_MODE_INTA;
>> + case 2: return QED_INT_MODE_MSI;
>> + default:
>> + QEDI_ERR(NULL, "Unknown qede_int_mode=%08x; "
>> + "Defaulting to MSI-x\n", int_mode);
>> + return QED_INT_MODE_MSIX;
>> + }
>> +}
>Errm. A per-driver interrupt mode?
>How very curious.
>You surely want to make that per-HBA, right?
This was added for testing purpose, we will remove this code.
[...]
>> +static int qedi_request_msix_irq(struct qedi_ctx *qedi)
>> +{
>> + int i, rc, cpu;
>> +
>> + cpu = cpumask_first(cpu_online_mask);
>> + for (i = 0; i < MIN_NUM_CPUS_MSIX(qedi); i++) {
>> + rc = request_irq(qedi->int_info.msix[i].vector,
>> + qedi_msix_handler, 0, "qedi",
>> + &qedi->fp_array[i]);
>> +
>> + if (rc) {
>> + QEDI_WARN(&qedi->dbg_ctx, "request_irq failed.\n");
>> + qedi_sync_free_irqs(qedi);
>> + return rc;
>> + }
>> + qedi->int_info.used_cnt++;
>> + rc = irq_set_affinity_hint(qedi->int_info.msix[i].vector,
>> + get_cpu_mask(cpu));
>> + cpu = cpumask_next(cpu, cpu_online_mask);
>> + }
>> +
>> + return 0;
>> +}
>Please use the irq-affinity rework from Christoph here; that'll save you
>the additional msix vectors allocation.
The existing qed* driver(s) and common module (qed) framework is built on
top of the older pci_enable_msix_*() API. The new framework requires
re-work on the existing qed common module API. That would need
co-ordination among other dependent drivers (e.g.: qede network driver,
which is already in the tree). We would prefer to add this as a follow on
(to the initial submission) effort, with additional testing done and
submission co-ordinated across protocol drivers.
Thanks,
Manish
^ permalink raw reply
* [PATCH v5 5/5] wcn36xx: Don't use the destroyed hal_mutex
From: Bjorn Andersson @ 2016-11-15 6:06 UTC (permalink / raw)
To: Eugene Krasnikov, Kalle Valo
Cc: wcn36xx, linux-wireless, netdev, linux-kernel, linux-arm-msm,
Andy Gross
In-Reply-To: <1479190014-11297-1-git-send-email-bjorn.andersson@linaro.org>
ieee80211_unregister_hw() might invoke operations to stop the interface,
that uses the hal_mutex. So don't destroy it until after we're done
using it.
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
With this patch I can successfully (although with a SMD send timeout in the
shutdown path) start and stop the WCNSS PIL/remoteproc multiple times and the
wlan0 interface will come and go accordingly.
Will submit the necessary DT patches soon as well.
Changes since v4:
- New patch
drivers/net/wireless/ath/wcn36xx/main.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ath/wcn36xx/main.c b/drivers/net/wireless/ath/wcn36xx/main.c
index 96a9584edcbb..0002190c9041 100644
--- a/drivers/net/wireless/ath/wcn36xx/main.c
+++ b/drivers/net/wireless/ath/wcn36xx/main.c
@@ -1241,7 +1241,6 @@ static int wcn36xx_remove(struct platform_device *pdev)
wcn36xx_dbg(WCN36XX_DBG_MAC, "platform remove\n");
release_firmware(wcn->nv);
- mutex_destroy(&wcn->hal_mutex);
ieee80211_unregister_hw(hw);
@@ -1250,6 +1249,8 @@ static int wcn36xx_remove(struct platform_device *pdev)
iounmap(wcn->dxe_base);
iounmap(wcn->ccu_base);
+
+ mutex_destroy(&wcn->hal_mutex);
ieee80211_free_hw(hw);
return 0;
--
2.5.0
^ permalink raw reply related
* [PATCH v5 4/5] wcn36xx: Implement print_reg indication
From: Bjorn Andersson @ 2016-11-15 6:06 UTC (permalink / raw)
To: Eugene Krasnikov, Kalle Valo
Cc: wcn36xx, linux-wireless, netdev, linux-kernel, linux-arm-msm,
Andy Gross, Nicolas Dechesne
In-Reply-To: <1479190014-11297-1-git-send-email-bjorn.andersson@linaro.org>
Some firmware versions sends a "print register indication", handle this
by printing out the content.
Cc: Nicolas Dechesne <ndec@linaro.org>
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
Changes since v4:
- None
drivers/net/wireless/ath/wcn36xx/hal.h | 16 ++++++++++++++++
drivers/net/wireless/ath/wcn36xx/smd.c | 30 ++++++++++++++++++++++++++++++
2 files changed, 46 insertions(+)
diff --git a/drivers/net/wireless/ath/wcn36xx/hal.h b/drivers/net/wireless/ath/wcn36xx/hal.h
index 4f87ef1e1eb8..b765c647319d 100644
--- a/drivers/net/wireless/ath/wcn36xx/hal.h
+++ b/drivers/net/wireless/ath/wcn36xx/hal.h
@@ -350,6 +350,8 @@ enum wcn36xx_hal_host_msg_type {
WCN36XX_HAL_AVOID_FREQ_RANGE_IND = 233,
+ WCN36XX_HAL_PRINT_REG_INFO_IND = 259,
+
WCN36XX_HAL_MSG_MAX = WCN36XX_HAL_MSG_TYPE_MAX_ENUM_SIZE
};
@@ -4703,4 +4705,18 @@ struct stats_class_b_ind {
u32 rx_time_total;
};
+/* WCN36XX_HAL_PRINT_REG_INFO_IND */
+struct wcn36xx_hal_print_reg_info_ind {
+ struct wcn36xx_hal_msg_header header;
+
+ u32 count;
+ u32 scenario;
+ u32 reason;
+
+ struct {
+ u32 addr;
+ u32 value;
+ } regs[];
+} __packed;
+
#endif /* _HAL_H_ */
diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
index be5e5ea1e5c3..1c2966f7db7a 100644
--- a/drivers/net/wireless/ath/wcn36xx/smd.c
+++ b/drivers/net/wireless/ath/wcn36xx/smd.c
@@ -2109,6 +2109,30 @@ static int wcn36xx_smd_delete_sta_context_ind(struct wcn36xx *wcn,
return -ENOENT;
}
+static int wcn36xx_smd_print_reg_info_ind(struct wcn36xx *wcn,
+ void *buf,
+ size_t len)
+{
+ struct wcn36xx_hal_print_reg_info_ind *rsp = buf;
+ int i;
+
+ if (len < sizeof(*rsp)) {
+ wcn36xx_warn("Corrupted print reg info indication\n");
+ return -EIO;
+ }
+
+ wcn36xx_dbg(WCN36XX_DBG_HAL,
+ "reginfo indication, scenario: 0x%x reason: 0x%x\n",
+ rsp->scenario, rsp->reason);
+
+ for (i = 0; i < rsp->count; i++) {
+ wcn36xx_dbg(WCN36XX_DBG_HAL, "\t0x%x: 0x%x\n",
+ rsp->regs[i].addr, rsp->regs[i].value);
+ }
+
+ return 0;
+}
+
int wcn36xx_smd_update_cfg(struct wcn36xx *wcn, u32 cfg_id, u32 value)
{
struct wcn36xx_hal_update_cfg_req_msg msg_body, *body;
@@ -2237,6 +2261,7 @@ int wcn36xx_smd_rsp_process(struct qcom_smd_channel *channel,
case WCN36XX_HAL_OTA_TX_COMPL_IND:
case WCN36XX_HAL_MISSED_BEACON_IND:
case WCN36XX_HAL_DELETE_STA_CONTEXT_IND:
+ case WCN36XX_HAL_PRINT_REG_INFO_IND:
msg_ind = kmalloc(sizeof(*msg_ind) + len, GFP_ATOMIC);
if (!msg_ind) {
wcn36xx_err("Run out of memory while handling SMD_EVENT (%d)\n",
@@ -2296,6 +2321,11 @@ static void wcn36xx_ind_smd_work(struct work_struct *work)
hal_ind_msg->msg,
hal_ind_msg->msg_len);
break;
+ case WCN36XX_HAL_PRINT_REG_INFO_IND:
+ wcn36xx_smd_print_reg_info_ind(wcn,
+ hal_ind_msg->msg,
+ hal_ind_msg->msg_len);
+ break;
default:
wcn36xx_err("SMD_EVENT (%d) not supported\n",
msg_header->msg_type);
--
2.5.0
^ permalink raw reply related
* [PATCH v5 3/5] wcn36xx: Implement firmware assisted scan
From: Bjorn Andersson @ 2016-11-15 6:06 UTC (permalink / raw)
To: Eugene Krasnikov, Kalle Valo
Cc: wcn36xx-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-wireless-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-arm-msm-u79uwXL29TY76Z2rM5mHXA, Andy Gross
In-Reply-To: <1479190014-11297-1-git-send-email-bjorn.andersson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Using the software based channel scan mechanism from mac80211 keeps us
offline for 10-15 second, we should instead issue a start_scan/end_scan
on each channel reducing this time.
Signed-off-by: Bjorn Andersson <bjorn.andersson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
Changes since v4:
- None
drivers/net/wireless/ath/wcn36xx/main.c | 64 +++++++++++++++++++++++++-----
drivers/net/wireless/ath/wcn36xx/smd.c | 8 ++--
drivers/net/wireless/ath/wcn36xx/smd.h | 4 +-
drivers/net/wireless/ath/wcn36xx/txrx.c | 19 ++++++---
drivers/net/wireless/ath/wcn36xx/wcn36xx.h | 9 +++++
5 files changed, 81 insertions(+), 23 deletions(-)
diff --git a/drivers/net/wireless/ath/wcn36xx/main.c b/drivers/net/wireless/ath/wcn36xx/main.c
index 3c2522b07c90..96a9584edcbb 100644
--- a/drivers/net/wireless/ath/wcn36xx/main.c
+++ b/drivers/net/wireless/ath/wcn36xx/main.c
@@ -568,23 +568,59 @@ static int wcn36xx_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
return ret;
}
-static void wcn36xx_sw_scan_start(struct ieee80211_hw *hw,
- struct ieee80211_vif *vif,
- const u8 *mac_addr)
+static void wcn36xx_hw_scan_worker(struct work_struct *work)
{
- struct wcn36xx *wcn = hw->priv;
+ struct wcn36xx *wcn = container_of(work, struct wcn36xx, scan_work);
+ struct cfg80211_scan_request *req = wcn->scan_req;
+ u8 channels[WCN36XX_HAL_PNO_MAX_NETW_CHANNELS_EX];
+ struct cfg80211_scan_info scan_info = {};
+ int i;
+
+ wcn36xx_dbg(WCN36XX_DBG_MAC, "mac80211 scan %d channels worker\n", req->n_channels);
+
+ for (i = 0; i < req->n_channels; i++)
+ channels[i] = req->channels[i]->hw_value;
+
+ wcn36xx_smd_update_scan_params(wcn, channels, req->n_channels);
wcn36xx_smd_init_scan(wcn, HAL_SYS_MODE_SCAN);
- wcn36xx_smd_start_scan(wcn);
+ for (i = 0; i < req->n_channels; i++) {
+ wcn->scan_freq = req->channels[i]->center_freq;
+ wcn->scan_band = req->channels[i]->band;
+
+ wcn36xx_smd_start_scan(wcn, req->channels[i]->hw_value);
+ msleep(30);
+ wcn36xx_smd_end_scan(wcn, req->channels[i]->hw_value);
+
+ wcn->scan_freq = 0;
+ }
+ wcn36xx_smd_finish_scan(wcn, HAL_SYS_MODE_SCAN);
+
+ scan_info.aborted = false;
+ ieee80211_scan_completed(wcn->hw, &scan_info);
+
+ mutex_lock(&wcn->scan_lock);
+ wcn->scan_req = NULL;
+ mutex_unlock(&wcn->scan_lock);
}
-static void wcn36xx_sw_scan_complete(struct ieee80211_hw *hw,
- struct ieee80211_vif *vif)
+static int wcn36xx_hw_scan(struct ieee80211_hw *hw,
+ struct ieee80211_vif *vif,
+ struct ieee80211_scan_request *hw_req)
{
struct wcn36xx *wcn = hw->priv;
- wcn36xx_smd_end_scan(wcn);
- wcn36xx_smd_finish_scan(wcn, HAL_SYS_MODE_SCAN);
+ mutex_lock(&wcn->scan_lock);
+ if (wcn->scan_req) {
+ mutex_unlock(&wcn->scan_lock);
+ return -EBUSY;
+ }
+ wcn->scan_req = &hw_req->req;
+ mutex_unlock(&wcn->scan_lock);
+
+ schedule_work(&wcn->scan_work);
+
+ return 0;
}
static void wcn36xx_update_allowed_rates(struct ieee80211_sta *sta,
@@ -997,8 +1033,7 @@ static const struct ieee80211_ops wcn36xx_ops = {
.configure_filter = wcn36xx_configure_filter,
.tx = wcn36xx_tx,
.set_key = wcn36xx_set_key,
- .sw_scan_start = wcn36xx_sw_scan_start,
- .sw_scan_complete = wcn36xx_sw_scan_complete,
+ .hw_scan = wcn36xx_hw_scan,
.bss_info_changed = wcn36xx_bss_info_changed,
.set_rts_threshold = wcn36xx_set_rts_threshold,
.sta_add = wcn36xx_sta_add,
@@ -1023,6 +1058,7 @@ static int wcn36xx_init_ieee80211(struct wcn36xx *wcn)
ieee80211_hw_set(wcn->hw, SUPPORTS_PS);
ieee80211_hw_set(wcn->hw, SIGNAL_DBM);
ieee80211_hw_set(wcn->hw, HAS_RATE_CONTROL);
+ ieee80211_hw_set(wcn->hw, SINGLE_SCAN_ON_ALL_BANDS);
wcn->hw->wiphy->interface_modes = BIT(NL80211_IFTYPE_STATION) |
BIT(NL80211_IFTYPE_AP) |
@@ -1032,6 +1068,9 @@ static int wcn36xx_init_ieee80211(struct wcn36xx *wcn)
wcn->hw->wiphy->bands[NL80211_BAND_2GHZ] = &wcn_band_2ghz;
wcn->hw->wiphy->bands[NL80211_BAND_5GHZ] = &wcn_band_5ghz;
+ wcn->hw->wiphy->max_scan_ssids = WCN36XX_MAX_SCAN_SSIDS;
+ wcn->hw->wiphy->max_scan_ie_len = WCN36XX_MAX_SCAN_IE_LEN;
+
wcn->hw->wiphy->cipher_suites = cipher_suites;
wcn->hw->wiphy->n_cipher_suites = ARRAY_SIZE(cipher_suites);
@@ -1152,6 +1191,9 @@ static int wcn36xx_probe(struct platform_device *pdev)
wcn->hw = hw;
wcn->dev = &pdev->dev;
mutex_init(&wcn->hal_mutex);
+ mutex_init(&wcn->scan_lock);
+
+ INIT_WORK(&wcn->scan_work, wcn36xx_hw_scan_worker);
wcn->smd_channel = qcom_wcnss_open_channel(wcnss, "WLAN_CTRL", wcn36xx_smd_rsp_process);
if (IS_ERR(wcn->smd_channel)) {
diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
index af0260add841..be5e5ea1e5c3 100644
--- a/drivers/net/wireless/ath/wcn36xx/smd.c
+++ b/drivers/net/wireless/ath/wcn36xx/smd.c
@@ -522,7 +522,7 @@ int wcn36xx_smd_init_scan(struct wcn36xx *wcn, enum wcn36xx_hal_sys_mode mode)
return ret;
}
-int wcn36xx_smd_start_scan(struct wcn36xx *wcn)
+int wcn36xx_smd_start_scan(struct wcn36xx *wcn, u8 scan_channel)
{
struct wcn36xx_hal_start_scan_req_msg msg_body;
int ret = 0;
@@ -530,7 +530,7 @@ int wcn36xx_smd_start_scan(struct wcn36xx *wcn)
mutex_lock(&wcn->hal_mutex);
INIT_HAL_MSG(msg_body, WCN36XX_HAL_START_SCAN_REQ);
- msg_body.scan_channel = WCN36XX_HW_CHANNEL(wcn);
+ msg_body.scan_channel = scan_channel;
PREPARE_HAL_BUF(wcn->hal_buf, msg_body);
@@ -552,7 +552,7 @@ int wcn36xx_smd_start_scan(struct wcn36xx *wcn)
return ret;
}
-int wcn36xx_smd_end_scan(struct wcn36xx *wcn)
+int wcn36xx_smd_end_scan(struct wcn36xx *wcn, u8 scan_channel)
{
struct wcn36xx_hal_end_scan_req_msg msg_body;
int ret = 0;
@@ -560,7 +560,7 @@ int wcn36xx_smd_end_scan(struct wcn36xx *wcn)
mutex_lock(&wcn->hal_mutex);
INIT_HAL_MSG(msg_body, WCN36XX_HAL_END_SCAN_REQ);
- msg_body.scan_channel = WCN36XX_HW_CHANNEL(wcn);
+ msg_body.scan_channel = scan_channel;
PREPARE_HAL_BUF(wcn->hal_buf, msg_body);
diff --git a/drivers/net/wireless/ath/wcn36xx/smd.h b/drivers/net/wireless/ath/wcn36xx/smd.h
index 40d829563c2b..8892ccd67b14 100644
--- a/drivers/net/wireless/ath/wcn36xx/smd.h
+++ b/drivers/net/wireless/ath/wcn36xx/smd.h
@@ -60,8 +60,8 @@ int wcn36xx_smd_load_nv(struct wcn36xx *wcn);
int wcn36xx_smd_start(struct wcn36xx *wcn);
int wcn36xx_smd_stop(struct wcn36xx *wcn);
int wcn36xx_smd_init_scan(struct wcn36xx *wcn, enum wcn36xx_hal_sys_mode mode);
-int wcn36xx_smd_start_scan(struct wcn36xx *wcn);
-int wcn36xx_smd_end_scan(struct wcn36xx *wcn);
+int wcn36xx_smd_start_scan(struct wcn36xx *wcn, u8 scan_channel);
+int wcn36xx_smd_end_scan(struct wcn36xx *wcn, u8 scan_channel);
int wcn36xx_smd_finish_scan(struct wcn36xx *wcn,
enum wcn36xx_hal_sys_mode mode);
int wcn36xx_smd_update_scan_params(struct wcn36xx *wcn, u8 *channels, size_t channel_count);
diff --git a/drivers/net/wireless/ath/wcn36xx/txrx.c b/drivers/net/wireless/ath/wcn36xx/txrx.c
index 1f34c2e912d7..8c387a0a3c09 100644
--- a/drivers/net/wireless/ath/wcn36xx/txrx.c
+++ b/drivers/net/wireless/ath/wcn36xx/txrx.c
@@ -45,9 +45,20 @@ int wcn36xx_rx_skb(struct wcn36xx *wcn, struct sk_buff *skb)
skb_put(skb, bd->pdu.mpdu_header_off + bd->pdu.mpdu_len);
skb_pull(skb, bd->pdu.mpdu_header_off);
+ hdr = (struct ieee80211_hdr *) skb->data;
+ fc = __le16_to_cpu(hdr->frame_control);
+ sn = IEEE80211_SEQ_TO_SN(__le16_to_cpu(hdr->seq_ctrl));
+
+ /* When scanning associate beacons to this */
+ if (ieee80211_is_beacon(hdr->frame_control) && wcn->scan_freq) {
+ status.freq = wcn->scan_freq;
+ status.band = wcn->scan_band;
+ } else {
+ status.freq = WCN36XX_CENTER_FREQ(wcn);
+ status.band = WCN36XX_BAND(wcn);
+ }
+
status.mactime = 10;
- status.freq = WCN36XX_CENTER_FREQ(wcn);
- status.band = WCN36XX_BAND(wcn);
status.signal = -get_rssi0(bd);
status.antenna = 1;
status.rate_idx = 1;
@@ -61,10 +72,6 @@ int wcn36xx_rx_skb(struct wcn36xx *wcn, struct sk_buff *skb)
memcpy(IEEE80211_SKB_RXCB(skb), &status, sizeof(status));
- hdr = (struct ieee80211_hdr *) skb->data;
- fc = __le16_to_cpu(hdr->frame_control);
- sn = IEEE80211_SEQ_TO_SN(__le16_to_cpu(hdr->seq_ctrl));
-
if (ieee80211_is_beacon(hdr->frame_control)) {
wcn36xx_dbg(WCN36XX_DBG_BEACON, "beacon skb %p len %d fc %04x sn %d\n",
skb, skb->len, fc, sn);
diff --git a/drivers/net/wireless/ath/wcn36xx/wcn36xx.h b/drivers/net/wireless/ath/wcn36xx/wcn36xx.h
index 68cc06cf9bc0..35a6590c3ee5 100644
--- a/drivers/net/wireless/ath/wcn36xx/wcn36xx.h
+++ b/drivers/net/wireless/ath/wcn36xx/wcn36xx.h
@@ -35,6 +35,9 @@
/* How many frames until we start a-mpdu TX session */
#define WCN36XX_AMPDU_START_THRESH 20
+#define WCN36XX_MAX_SCAN_SSIDS 9
+#define WCN36XX_MAX_SCAN_IE_LEN 500
+
extern unsigned int wcn36xx_dbg_mask;
enum wcn36xx_debug_mask {
@@ -212,6 +215,12 @@ struct wcn36xx {
spinlock_t hal_ind_lock;
struct list_head hal_ind_queue;
+ struct work_struct scan_work;
+ struct cfg80211_scan_request *scan_req;
+ int scan_freq;
+ int scan_band;
+ struct mutex scan_lock;
+
/* DXE channels */
struct wcn36xx_dxe_ch dxe_tx_l_ch; /* TX low */
struct wcn36xx_dxe_ch dxe_tx_h_ch; /* TX high */
--
2.5.0
^ permalink raw reply related
* [PATCH v5 2/5] wcn36xx: Transition driver to SMD client
From: Bjorn Andersson @ 2016-11-15 6:06 UTC (permalink / raw)
To: Eugene Krasnikov, Kalle Valo
Cc: wcn36xx, linux-wireless, netdev, linux-kernel, linux-arm-msm,
Andy Gross
In-Reply-To: <1479190014-11297-1-git-send-email-bjorn.andersson@linaro.org>
The wcn36xx wifi driver follows the life cycle of the WLAN_CTRL SMD
channel, as such it should be a SMD client. This patch makes this
transition, now that we have the necessary frameworks available.
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
Changes since v4:
- Added Kconfig dependency to handle dependencies compiled as modules
drivers/net/wireless/ath/wcn36xx/Kconfig | 2 +
drivers/net/wireless/ath/wcn36xx/dxe.c | 16 +++---
drivers/net/wireless/ath/wcn36xx/main.c | 79 ++++++++++++++++++++----------
drivers/net/wireless/ath/wcn36xx/smd.c | 31 +++++-------
drivers/net/wireless/ath/wcn36xx/smd.h | 5 ++
drivers/net/wireless/ath/wcn36xx/wcn36xx.h | 21 +++-----
6 files changed, 88 insertions(+), 66 deletions(-)
diff --git a/drivers/net/wireless/ath/wcn36xx/Kconfig b/drivers/net/wireless/ath/wcn36xx/Kconfig
index 591ebaea8265..4b83e87f0b94 100644
--- a/drivers/net/wireless/ath/wcn36xx/Kconfig
+++ b/drivers/net/wireless/ath/wcn36xx/Kconfig
@@ -1,6 +1,8 @@
config WCN36XX
tristate "Qualcomm Atheros WCN3660/3680 support"
depends on MAC80211 && HAS_DMA
+ depends on QCOM_WCNSS_CTRL || QCOM_WCNSS_CTRL=n
+ depends on QCOM_SMD || QCOM_SMD=n
---help---
This module adds support for wireless adapters based on
Qualcomm Atheros WCN3660 and WCN3680 mobile chipsets.
diff --git a/drivers/net/wireless/ath/wcn36xx/dxe.c b/drivers/net/wireless/ath/wcn36xx/dxe.c
index 231fd022f0f5..87dfdaf9044c 100644
--- a/drivers/net/wireless/ath/wcn36xx/dxe.c
+++ b/drivers/net/wireless/ath/wcn36xx/dxe.c
@@ -23,6 +23,7 @@
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
#include <linux/interrupt.h>
+#include <linux/soc/qcom/smem_state.h>
#include "wcn36xx.h"
#include "txrx.h"
@@ -151,9 +152,12 @@ int wcn36xx_dxe_alloc_ctl_blks(struct wcn36xx *wcn)
goto out_err;
/* Initialize SMSM state Clear TX Enable RING EMPTY STATE */
- ret = wcn->ctrl_ops->smsm_change_state(
- WCN36XX_SMSM_WLAN_TX_ENABLE,
- WCN36XX_SMSM_WLAN_TX_RINGS_EMPTY);
+ ret = qcom_smem_state_update_bits(wcn->tx_enable_state,
+ WCN36XX_SMSM_WLAN_TX_ENABLE |
+ WCN36XX_SMSM_WLAN_TX_RINGS_EMPTY,
+ WCN36XX_SMSM_WLAN_TX_RINGS_EMPTY);
+ if (ret)
+ goto out_err;
return 0;
@@ -678,9 +682,9 @@ int wcn36xx_dxe_tx_frame(struct wcn36xx *wcn,
* notify chip about new frame through SMSM bus.
*/
if (is_low && vif_priv->pw_state == WCN36XX_BMPS) {
- wcn->ctrl_ops->smsm_change_state(
- 0,
- WCN36XX_SMSM_WLAN_TX_ENABLE);
+ qcom_smem_state_update_bits(wcn->tx_rings_empty_state,
+ WCN36XX_SMSM_WLAN_TX_ENABLE,
+ WCN36XX_SMSM_WLAN_TX_ENABLE);
} else {
/* indicate End Of Packet and generate interrupt on descriptor
* done.
diff --git a/drivers/net/wireless/ath/wcn36xx/main.c b/drivers/net/wireless/ath/wcn36xx/main.c
index e1d59da2ad20..3c2522b07c90 100644
--- a/drivers/net/wireless/ath/wcn36xx/main.c
+++ b/drivers/net/wireless/ath/wcn36xx/main.c
@@ -21,6 +21,10 @@
#include <linux/platform_device.h>
#include <linux/of_address.h>
#include <linux/of_device.h>
+#include <linux/of_irq.h>
+#include <linux/soc/qcom/smd.h>
+#include <linux/soc/qcom/smem_state.h>
+#include <linux/soc/qcom/wcnss_ctrl.h>
#include "wcn36xx.h"
unsigned int wcn36xx_dbg_mask;
@@ -1058,8 +1062,7 @@ static int wcn36xx_platform_get_resources(struct wcn36xx *wcn,
int ret;
/* Set TX IRQ */
- res = platform_get_resource_byname(pdev, IORESOURCE_IRQ,
- "wcnss_wlantx_irq");
+ res = platform_get_resource_byname(pdev, IORESOURCE_IRQ, "tx");
if (!res) {
wcn36xx_err("failed to get tx_irq\n");
return -ENOENT;
@@ -1067,14 +1070,29 @@ static int wcn36xx_platform_get_resources(struct wcn36xx *wcn,
wcn->tx_irq = res->start;
/* Set RX IRQ */
- res = platform_get_resource_byname(pdev, IORESOURCE_IRQ,
- "wcnss_wlanrx_irq");
+ res = platform_get_resource_byname(pdev, IORESOURCE_IRQ, "rx");
if (!res) {
wcn36xx_err("failed to get rx_irq\n");
return -ENOENT;
}
wcn->rx_irq = res->start;
+ /* Acquire SMSM tx enable handle */
+ wcn->tx_enable_state = qcom_smem_state_get(&pdev->dev,
+ "tx-enable", &wcn->tx_enable_state_bit);
+ if (IS_ERR(wcn->tx_enable_state)) {
+ wcn36xx_err("failed to get tx-enable state\n");
+ return PTR_ERR(wcn->tx_enable_state);
+ }
+
+ /* Acquire SMSM tx rings empty handle */
+ wcn->tx_rings_empty_state = qcom_smem_state_get(&pdev->dev,
+ "tx-rings-empty", &wcn->tx_rings_empty_state_bit);
+ if (IS_ERR(wcn->tx_rings_empty_state)) {
+ wcn36xx_err("failed to get tx-rings-empty state\n");
+ return PTR_ERR(wcn->tx_rings_empty_state);
+ }
+
mmio_node = of_parse_phandle(pdev->dev.parent->of_node, "qcom,mmio", 0);
if (!mmio_node) {
wcn36xx_err("failed to acquire qcom,mmio reference\n");
@@ -1115,11 +1133,14 @@ static int wcn36xx_probe(struct platform_device *pdev)
{
struct ieee80211_hw *hw;
struct wcn36xx *wcn;
+ void *wcnss;
int ret;
- u8 addr[ETH_ALEN];
+ const u8 *addr;
wcn36xx_dbg(WCN36XX_DBG_MAC, "platform probe\n");
+ wcnss = dev_get_drvdata(pdev->dev.parent);
+
hw = ieee80211_alloc_hw(sizeof(struct wcn36xx), &wcn36xx_ops);
if (!hw) {
wcn36xx_err("failed to alloc hw\n");
@@ -1130,11 +1151,23 @@ static int wcn36xx_probe(struct platform_device *pdev)
wcn = hw->priv;
wcn->hw = hw;
wcn->dev = &pdev->dev;
- wcn->ctrl_ops = pdev->dev.platform_data;
-
mutex_init(&wcn->hal_mutex);
- if (!wcn->ctrl_ops->get_hw_mac(addr)) {
+ wcn->smd_channel = qcom_wcnss_open_channel(wcnss, "WLAN_CTRL", wcn36xx_smd_rsp_process);
+ if (IS_ERR(wcn->smd_channel)) {
+ wcn36xx_err("failed to open WLAN_CTRL channel\n");
+ ret = PTR_ERR(wcn->smd_channel);
+ goto out_wq;
+ }
+
+ qcom_smd_set_drvdata(wcn->smd_channel, hw);
+
+ addr = of_get_property(pdev->dev.of_node, "local-mac-address", &ret);
+ if (addr && ret != ETH_ALEN) {
+ wcn36xx_err("invalid local-mac-address\n");
+ ret = -EINVAL;
+ goto out_wq;
+ } else if (addr) {
wcn36xx_info("mac address: %pM\n", addr);
SET_IEEE80211_PERM_ADDR(wcn->hw, addr);
}
@@ -1158,6 +1191,7 @@ static int wcn36xx_probe(struct platform_device *pdev)
out_err:
return ret;
}
+
static int wcn36xx_remove(struct platform_device *pdev)
{
struct ieee80211_hw *hw = platform_get_drvdata(pdev);
@@ -1168,42 +1202,33 @@ static int wcn36xx_remove(struct platform_device *pdev)
mutex_destroy(&wcn->hal_mutex);
ieee80211_unregister_hw(hw);
+
+ qcom_smem_state_put(wcn->tx_enable_state);
+ qcom_smem_state_put(wcn->tx_rings_empty_state);
+
iounmap(wcn->dxe_base);
iounmap(wcn->ccu_base);
ieee80211_free_hw(hw);
return 0;
}
-static const struct platform_device_id wcn36xx_platform_id_table[] = {
- {
- .name = "wcn36xx",
- .driver_data = 0
- },
+
+static const struct of_device_id wcn36xx_of_match[] = {
+ { .compatible = "qcom,wcnss-wlan" },
{}
};
-MODULE_DEVICE_TABLE(platform, wcn36xx_platform_id_table);
+MODULE_DEVICE_TABLE(of, wcn36xx_of_match);
static struct platform_driver wcn36xx_driver = {
.probe = wcn36xx_probe,
.remove = wcn36xx_remove,
.driver = {
.name = "wcn36xx",
+ .of_match_table = wcn36xx_of_match,
},
- .id_table = wcn36xx_platform_id_table,
};
-static int __init wcn36xx_init(void)
-{
- platform_driver_register(&wcn36xx_driver);
- return 0;
-}
-module_init(wcn36xx_init);
-
-static void __exit wcn36xx_exit(void)
-{
- platform_driver_unregister(&wcn36xx_driver);
-}
-module_exit(wcn36xx_exit);
+module_platform_driver(wcn36xx_driver);
MODULE_LICENSE("Dual BSD/GPL");
MODULE_AUTHOR("Eugene Krasnikov k.eugene.e@gmail.com");
diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
index a443992320f2..af0260add841 100644
--- a/drivers/net/wireless/ath/wcn36xx/smd.c
+++ b/drivers/net/wireless/ath/wcn36xx/smd.c
@@ -19,6 +19,7 @@
#include <linux/etherdevice.h>
#include <linux/firmware.h>
#include <linux/bitops.h>
+#include <linux/soc/qcom/smd.h>
#include "smd.h"
struct wcn36xx_cfg_val {
@@ -253,7 +254,7 @@ static int wcn36xx_smd_send_and_wait(struct wcn36xx *wcn, size_t len)
init_completion(&wcn->hal_rsp_compl);
start = jiffies;
- ret = wcn->ctrl_ops->tx(wcn->hal_buf, len);
+ ret = qcom_smd_send(wcn->smd_channel, wcn->hal_buf, len);
if (ret) {
wcn36xx_err("HAL TX failed\n");
goto out;
@@ -2180,9 +2181,12 @@ int wcn36xx_smd_set_mc_list(struct wcn36xx *wcn,
return ret;
}
-static void wcn36xx_smd_rsp_process(struct wcn36xx *wcn, void *buf, size_t len)
+int wcn36xx_smd_rsp_process(struct qcom_smd_channel *channel,
+ const void *buf, size_t len)
{
- struct wcn36xx_hal_msg_header *msg_header = buf;
+ const struct wcn36xx_hal_msg_header *msg_header = buf;
+ struct ieee80211_hw *hw = qcom_smd_get_drvdata(channel);
+ struct wcn36xx *wcn = hw->priv;
struct wcn36xx_hal_ind_msg *msg_ind;
wcn36xx_dbg_dump(WCN36XX_DBG_SMD_DUMP, "SMD <<< ", buf, len);
@@ -2233,15 +2237,11 @@ static void wcn36xx_smd_rsp_process(struct wcn36xx *wcn, void *buf, size_t len)
case WCN36XX_HAL_OTA_TX_COMPL_IND:
case WCN36XX_HAL_MISSED_BEACON_IND:
case WCN36XX_HAL_DELETE_STA_CONTEXT_IND:
- msg_ind = kmalloc(sizeof(*msg_ind) + len, GFP_KERNEL);
+ msg_ind = kmalloc(sizeof(*msg_ind) + len, GFP_ATOMIC);
if (!msg_ind) {
- /*
- * FIXME: Do something smarter then just
- * printing an error.
- */
wcn36xx_err("Run out of memory while handling SMD_EVENT (%d)\n",
msg_header->msg_type);
- break;
+ return -ENOMEM;
}
msg_ind->msg_len = len;
@@ -2257,6 +2257,8 @@ static void wcn36xx_smd_rsp_process(struct wcn36xx *wcn, void *buf, size_t len)
wcn36xx_err("SMD_EVENT (%d) not supported\n",
msg_header->msg_type);
}
+
+ return 0;
}
static void wcn36xx_ind_smd_work(struct work_struct *work)
{
@@ -2315,22 +2317,13 @@ int wcn36xx_smd_open(struct wcn36xx *wcn)
INIT_LIST_HEAD(&wcn->hal_ind_queue);
spin_lock_init(&wcn->hal_ind_lock);
- ret = wcn->ctrl_ops->open(wcn, wcn36xx_smd_rsp_process);
- if (ret) {
- wcn36xx_err("failed to open control channel\n");
- goto free_wq;
- }
-
- return ret;
+ return 0;
-free_wq:
- destroy_workqueue(wcn->hal_ind_wq);
out:
return ret;
}
void wcn36xx_smd_close(struct wcn36xx *wcn)
{
- wcn->ctrl_ops->close();
destroy_workqueue(wcn->hal_ind_wq);
}
diff --git a/drivers/net/wireless/ath/wcn36xx/smd.h b/drivers/net/wireless/ath/wcn36xx/smd.h
index df80cbbd9d1b..40d829563c2b 100644
--- a/drivers/net/wireless/ath/wcn36xx/smd.h
+++ b/drivers/net/wireless/ath/wcn36xx/smd.h
@@ -51,6 +51,7 @@ struct wcn36xx_hal_ind_msg {
};
struct wcn36xx;
+struct qcom_smd_channel;
int wcn36xx_smd_open(struct wcn36xx *wcn);
void wcn36xx_smd_close(struct wcn36xx *wcn);
@@ -127,6 +128,10 @@ int wcn36xx_smd_del_ba(struct wcn36xx *wcn, u16 tid, u8 sta_index);
int wcn36xx_smd_trigger_ba(struct wcn36xx *wcn, u8 sta_index);
int wcn36xx_smd_update_cfg(struct wcn36xx *wcn, u32 cfg_id, u32 value);
+
+int wcn36xx_smd_rsp_process(struct qcom_smd_channel *channel,
+ const void *buf, size_t len);
+
int wcn36xx_smd_set_mc_list(struct wcn36xx *wcn,
struct ieee80211_vif *vif,
struct wcn36xx_hal_rcv_flt_mc_addr_list_type *fp);
diff --git a/drivers/net/wireless/ath/wcn36xx/wcn36xx.h b/drivers/net/wireless/ath/wcn36xx/wcn36xx.h
index 22242d18e1fe..68cc06cf9bc0 100644
--- a/drivers/net/wireless/ath/wcn36xx/wcn36xx.h
+++ b/drivers/net/wireless/ath/wcn36xx/wcn36xx.h
@@ -103,19 +103,6 @@ struct nv_data {
u8 table;
};
-/* Interface for platform control path
- *
- * @open: hook must be called when wcn36xx wants to open control channel.
- * @tx: sends a buffer.
- */
-struct wcn36xx_platform_ctrl_ops {
- int (*open)(void *drv_priv, void *rsp_cb);
- void (*close)(void);
- int (*tx)(char *buf, size_t len);
- int (*get_hw_mac)(u8 *addr);
- int (*smsm_change_state)(u32 clear_mask, u32 set_mask);
-};
-
/**
* struct wcn36xx_vif - holds VIF related fields
*
@@ -205,7 +192,13 @@ struct wcn36xx {
void __iomem *ccu_base;
void __iomem *dxe_base;
- struct wcn36xx_platform_ctrl_ops *ctrl_ops;
+ struct qcom_smd_channel *smd_channel;
+
+ struct qcom_smem_state *tx_enable_state;
+ unsigned tx_enable_state_bit;
+ struct qcom_smem_state *tx_rings_empty_state;
+ unsigned tx_rings_empty_state_bit;
+
/*
* smd_buf must be protected with smd_mutex to garantee
* that all messages are sent one after another
--
2.5.0
^ permalink raw reply related
* [PATCH v5 1/5] soc: qcom: smem_state: Fix include for ERR_PTR()
From: Bjorn Andersson @ 2016-11-15 6:06 UTC (permalink / raw)
To: Eugene Krasnikov, Kalle Valo
Cc: Andy Gross, wcn36xx, linux-wireless, netdev, linux-kernel,
linux-arm-msm
The correct include file for getting errno constants and ERR_PTR() is
linux/err.h, rather than linux/errno.h, so fix the include.
Fixes: e8b123e60084 ("soc: qcom: smem_state: Add stubs for disabled smem_state")
Acked-by: Andy Gross <andy.gross@linaro.org>
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
Kalle, please merge this patch through your tree.
Changes since v4:
- New patch
include/linux/soc/qcom/smem_state.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/soc/qcom/smem_state.h b/include/linux/soc/qcom/smem_state.h
index 7b88697929e9..b8478ee7a71f 100644
--- a/include/linux/soc/qcom/smem_state.h
+++ b/include/linux/soc/qcom/smem_state.h
@@ -1,7 +1,7 @@
#ifndef __QCOM_SMEM_STATE__
#define __QCOM_SMEM_STATE__
-#include <linux/errno.h>
+#include <linux/err.h>
struct device_node;
struct qcom_smem_state;
--
2.5.0
^ permalink raw reply related
* Re: [PATCH net] gro_cells: mark napi struct as not busy poll candidates
From: Cong Wang @ 2016-11-15 5:21 UTC (permalink / raw)
To: Eric Dumazet
Cc: Eric W. Biederman, David Miller, Paul E. McKenney,
Rolf Neugebauer, Linux Kernel Network Developers, Justin Cormack,
Ian Campbell, Eric Dumazet
In-Reply-To: <1479169722.8455.108.camel@edumazet-glaptop3.roam.corp.google.com>
On Mon, Nov 14, 2016 at 4:28 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> Rolf Neugebauer reported very long delays at netns dismantle.
>
> Eric W. Biederman was kind enough to look at this problem
> and noticed synchronize_net() occurring from netif_napi_del() that was
> added in linux-4.5
>
> Busy polling makes no sense for tunnels NAPI.
> If busy poll is used for sessions over tunnels, the poller will need to
> poll the physical device queue anyway.
>
> netif_tx_napi_add() could be used here, but function name is misleading,
> and renaming it is not stable material, so set NAPI_STATE_NO_BUSY_POLL
> bit directly.
>
> This will avoid inserting gro_cells napi structures in napi_hash[]
> and avoid the problematic synchronize_net() (per possible cpu) that
> Rolf reported.
>
> Fixes: 93d05d4a320c ("net: provide generic busy polling to all NAPI drivers")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reported-by: Rolf Neugebauer <rolf.neugebauer@docker.com>
> Reported-by: Eric W. Biederman <ebiederm@xmission.com>
Acked-by: Cong Wang <xiyou.wangcong@gmail.com>
^ permalink raw reply
* Disable all network protocols on an interface?
From: Ed Swierk @ 2016-11-15 4:41 UTC (permalink / raw)
To: linux-netdev, linux-kernel; +Cc: Ed Swierk
I have a Linux kernel 4.4 system hosting a number of kvm VMs. Physical interface eth0 connects to an 802.1Q trunk port on an external switch. Each VM has a virtual interface (e1000 or virtio-net) connected to the physical NIC through a macvtap interface and a VLAN interface; traffic between the external switch and the host is tagged with a per-VM tag. The only logic is demultiplexing incoming traffic by VLAN tag and stripping the tag, and adding the tag for outgoing traffic. Other than that, the eth0-VM datapath is a dumb pipe.
eth0 is assigned an IP address for host applications to send and receive untagged packets. For example, here's the setup with 2 VMs.
+- (untagged) 192.168.0.2
eth0 -+- (tag 1) --- eth0.1 --- macvtap1 --- VM1
+- (tag 2) --- eth0.2 --- macvtap2 --- VM2
Various iptables rules filter the untagged packets received for host applications. The last rule in the INPUT chain logs incoming packets that don't match earlier rules:
-A INPUT -m limit --limit 10/min -j LOG --log-prefix FilterInput
This all works, but I see occasional FilterInput messages for traffic received on eth0.1 and eth0.2: so far, only DHCP packets with destination MAC address ff:ff:ff:ff:ff:ff.
FilterInput IN=eth0.1 OUT= MAC=ff:ff:ff:ff:ff:ff:00:01:02:03:04:05:08:00 SRC=0.0.0.0 DST=255.255.255.255 LEN=328 TOS=0x10 PREC=0x00 TTL=128 ID=0 PROTO=UDP SPT=68 DPT=67 LEN=308
Even though these are IP packets, I naively expect packets received on the VLAN interface lacking IP address to be either consumed by the attached macvtap or dropped before they trigger an iptables filter INPUT rule. It's a bit alarming to see packets destined for a VM to be processed at all by the host IP stack.
Digging through the code, I find that the core packet receive function __netif_receive_skb_core() first gives master devices like bridges and macvlans/macvtaps a chance to consume the packet; otherwise the packet gets handled by all installed protocols like IPv4. The packet gets pretty far down the IP receive process before it's discovered that there's nowhere to route it to, and no local sockets to deliver it to. The iptables INPUT chain is invoked well before that happens. (As far as I can tell, there's no explicit check in the IP receive code whether a local interface has an IP address.)
The macvlan's rx_handler definitively consumes or drops unicast packets, depending on the destination MAC address. But for broadcast packets, it passes the packet to the attached VM interface and also tells the core receive function to continue processing it. Presumably this is to allow a macvlan to attach to one or more VMs as well as have a local IP address.
The logic in the bridge driver is a bit different: it consumes all packets from the slave interface. This makes sense as only the bridge master interface can be assigned a local IP address.
However in my application, I'm setting up the macvtap interfaces in passthrough mode, which precludes assigning a local IP address, just like a bridge slave. So it stands to reason that for a macvlan in passthrough mode, its rx_handler should consume or drop all packets, and not allow broadcast packets to also be handled locally.
This one-line change seems to do the trick:
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -411,7 +411,7 @@ static rx_handler_result_t macvlan_handle_frame(struct sk_buff **pskb)
rx_handler_result_t handle_res;
port = macvlan_port_get_rcu(skb->dev);
- if (is_multicast_ether_addr(eth->h_dest)) {
+ if (is_multicast_ether_addr(eth->h_dest) && !port->passthru) {
skb = ip_check_defrag(dev_net(skb->dev), skb, IP_DEFRAG_MACVLAN);
if (!skb)
return RX_HANDLER_CONSUMED;
Well, mostly. I still see FilterInput log messages in the brief window between creating the VLAN interface and attaching the macvtap to it, since there's no rx_handler to consume them. Hooking the VLAN interface to a bridge rather than a macvtap suppresses local IP processing on the slave but enables it on the bridge master interface. Apparently any non-slave interface can handle IP traffic to some extent, even if it doesn't have an IP address.
I worry that allowing any IP processing at all on eth0-VM traffic is a potential security hole, and I'm one configuration typo away from letting VM's traffic leak into another VM or a host application, and vice versa. And logging those FilterInput messages for non-local traffic just looks like sloppy security.
Is there some way to stop all local protocols from handling packets received on an interface--a protocol-agnostic equivalent of net.ipv6.conf.INTF.disable_ipv6? Would it be reasonable to implement one?
--Ed
^ permalink raw reply
* Re: [PATCH 1/3] tuntap: rx batching
From: Michael S. Tsirkin @ 2016-11-15 3:41 UTC (permalink / raw)
To: Jason Wang; +Cc: John Fastabend, netdev, linux-kernel
In-Reply-To: <39c36d36-9029-5d1f-496f-6ff404c3b77a@redhat.com>
On Tue, Nov 15, 2016 at 11:14:48AM +0800, Jason Wang wrote:
>
>
> On 2016年11月12日 00:20, Michael S. Tsirkin wrote:
> > On Fri, Nov 11, 2016 at 12:28:38PM +0800, Jason Wang wrote:
> > >
> > > On 2016年11月11日 12:17, John Fastabend wrote:
> > > > On 16-11-10 07:31 PM, Michael S. Tsirkin wrote:
> > > > > > On Fri, Nov 11, 2016 at 10:07:44AM +0800, Jason Wang wrote:
> > > > > > > >
> > > > > > > > On 2016年11月10日 00:38, Michael S. Tsirkin wrote:
> > > > > > > > > > On Wed, Nov 09, 2016 at 03:38:31PM +0800, Jason Wang wrote:
> > > > > > > > > > > > Backlog were used for tuntap rx, but it can only process 1 packet at
> > > > > > > > > > > > one time since it was scheduled during sendmsg() synchronously in
> > > > > > > > > > > > process context. This lead bad cache utilization so this patch tries
> > > > > > > > > > > > to do some batching before call rx NAPI. This is done through:
> > > > > > > > > > > >
> > > > > > > > > > > > - accept MSG_MORE as a hint from sendmsg() caller, if it was set,
> > > > > > > > > > > > batch the packet temporarily in a linked list and submit them all
> > > > > > > > > > > > once MSG_MORE were cleared.
> > > > > > > > > > > > - implement a tuntap specific NAPI handler for processing this kind of
> > > > > > > > > > > > possible batching. (This could be done by extending backlog to
> > > > > > > > > > > > support skb like, but using a tun specific one looks cleaner and
> > > > > > > > > > > > easier for future extension).
> > > > > > > > > > > >
> > > > > > > > > > > > Signed-off-by: Jason Wang<jasowang@redhat.com>
> > > > > > > > > > So why do we need an extra queue?
> > > > > > > > The idea was borrowed from backlog to allow some kind of bulking and avoid
> > > > > > > > spinlock on each dequeuing.
> > > > > > > >
> > > > > > > > > > This is not what hardware devices do.
> > > > > > > > > > How about adding the packet to queue unconditionally, deferring
> > > > > > > > > > signalling until we get sendmsg without MSG_MORE?
> > > > > > > > Then you need touch spinlock when dequeuing each packet.
> > > > Random thought, I have a cmpxchg ring I am using for the qdisc work that
> > > > could possibly replace the spinlock implementation. I haven't figured
> > > > out the resizing API yet because I did not need it but I assume it could
> > > > help here and let you dequeue multiple skbs in one operation.
> > > >
> > > > I can post the latest version if useful or an older version is
> > > > somewhere on patchworks as well.
> > > >
> > > > .John
> > > >
> > > >
> > > Look useful here, and I can compare the performance if you post.
> > >
> > > A question is can we extend the skb_array to support that?
> > >
> > > Thanks
> > I'd like to start with simple patch adding napi with one queue, then add
> > optimization patches on top.
>
> The point is tun is using backlog who uses two queues (process_queue and
> input_pkt_queue).
>
> How about something like:
>
> 1) NAPI support with skb_array
I would start with just write queue linked list. It all runs on a single
CPU normally, so the nice reductions of cache line bounces due to skb
array should never materialize.
While we are at it, limiting the size of the queue might
be a good idea. Kind of like TUNSETSNDBUF but 1. actually
working where instead of tracking packets within net stack
we make sndbuf track the internal buffer
> 2) MSG_MORE support
> 3) other optimizations on top
>
> ?
>
> >
> > One issue that comes to mind is that write queue limits
> > are byte based, they do not count packets unlike tun rx queue.
>
> I'm not sure I get the issue, write queue is not exported and only used for
> batching. We probably need an internal limit in tun to avoid OOM attacker
> from guest.
>
> Thanks
^ permalink raw reply
* Re: [PATCH 2/3] vhost: better detection of available buffers
From: Michael S. Tsirkin @ 2016-11-15 3:28 UTC (permalink / raw)
To: Jason Wang; +Cc: netdev, linux-kernel
In-Reply-To: <6f650bf0-1e2b-1c35-db34-5024383a6892@redhat.com>
On Tue, Nov 15, 2016 at 11:16:59AM +0800, Jason Wang wrote:
>
>
> On 2016年11月12日 00:20, Michael S. Tsirkin wrote:
> > On Fri, Nov 11, 2016 at 12:18:50PM +0800, Jason Wang wrote:
> > >
> > > On 2016年11月11日 11:41, Michael S. Tsirkin wrote:
> > > > On Fri, Nov 11, 2016 at 10:18:37AM +0800, Jason Wang wrote:
> > > > > >
> > > > > > On 2016年11月10日 03:57, Michael S. Tsirkin wrote:
> > > > > > > > On Wed, Nov 09, 2016 at 03:38:32PM +0800, Jason Wang wrote:
> > > > > > > > > > We should use vq->last_avail_idx instead of vq->avail_idx in the
> > > > > > > > > > checking of vhost_vq_avail_empty() since latter is the cached avail
> > > > > > > > > > index from guest but we want to know if there's pending available
> > > > > > > > > > buffers in the virtqueue.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Jason Wang<jasowang@redhat.com>
> > > > > > > > I'm not sure why is this patch here. Is it related to
> > > > > > > > batching somehow?
> > > > > > Yes, we need to know whether or not there's still buffers left in the
> > > > > > virtqueue, so need to check last_avail_idx. Otherwise, we're checking if
> > > > > > guest has submitted new buffers.
> > > > > >
> > > > > > > >
> > > > > > > > > > ---
> > > > > > > > > > drivers/vhost/vhost.c | 2 +-
> > > > > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > > > > >
> > > > > > > > > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > > > > > > > > > index c6f2d89..fdf4cdf 100644
> > > > > > > > > > --- a/drivers/vhost/vhost.c
> > > > > > > > > > +++ b/drivers/vhost/vhost.c
> > > > > > > > > > @@ -2230,7 +2230,7 @@ bool vhost_vq_avail_empty(struct vhost_dev *dev, struct vhost_virtqueue *vq)
> > > > > > > > > > if (r)
> > > > > > > > > > return false;
> > > > > > > > > > - return vhost16_to_cpu(vq, avail_idx) == vq->avail_idx;
> > > > > > > > > > + return vhost16_to_cpu(vq, avail_idx) == vq->last_avail_idx;
> > > > > > > > > > }
> > > > > > > > > > EXPORT_SYMBOL_GPL(vhost_vq_avail_empty);
> > > > > > > > That might be OK for TX but it's probably wrong for RX
> > > > > > > > where the fact that used != avail does not mean
> > > > > > > > we have enough space to store the packet.
> > > > > > Right, but it's no harm since it was just a hint, handle_rx() can handle
> > > > > > this situation.
> > > > Means busy polling will cause useless load on the CPU though.
> > > >
> > > Right, but,it's not easy to have 100% correct hint here. Needs more thought.
> > What's wrong with what we have? It polls until value changes.
> >
>
> But as you said, this does not mean (in mergeable cases) we have enough
> space to store the packet.
Absolutely but it checks once and then only re-checks after value
changes again.
--
MST
^ permalink raw reply
* Re: Virtio_net support vxlan encapsulation package TSO offload discuss
From: Jason Wang @ 2016-11-15 3:27 UTC (permalink / raw)
To: Zhangming (James, Euler), netdev@vger.kernel.org
Cc: Michael S. Tsirkin, Vlad Yasevic, Amnon Ilan
In-Reply-To: <DBCD2614ECF3FF4087A2C27CA80E34DD5401BE13@SZXEMA501-MBX.china.huawei.com>
On 2016年11月10日 14:19, Zhangming (James, Euler) wrote:
> On 2016年11月09日 15:14, Jason Wang wrote:
>> On 2016年11月08日 19:58, Zhangming (James, Euler) wrote:
>>> On 2016年11月08日 19:17, Jason Wang wrote:
>>>
>>>> On 2016年11月08日 19:13, Jason Wang wrote:
>>>>> Cc Michael
>>>>>
>>>>> On 2016年11月08日 16:34, Zhangming (James, Euler) wrote:
>>>>>> In container scenario, OVS is installed in the Virtual machine, and
>>>>>> all the containers connected to the OVS will communicated through
>>>>>> VXLAN encapsulation.
>>>>>>
>>>>>> By now, virtio_net does not support TSO offload for VXLAN
>>>>>> encapsulated TSO package. In this condition, the performance is not
>>>>>> good, sender is bottleneck
>>>>>>
>>>>>> I googled this scenario, but I didn’t find any information. Will
>>>>>> virtio_net support VXLAN encapsulation package TSO offload later?
>>>>>>
>>>>> Yes and for both sender and receiver.
>>>>>
>>>>>> My idea is virtio_net open encapsulated TSO offload, and transport
>>>>>> encapsulation info to TUN, TUN will parse the info and build skb
>>>>>> with encapsulation info.
>>>>>>
>>>>>> OVS or kernel on the host should be modified to support this. Using
>>>>>> this method, the TCP performance aremore than 2x as before.
>>>>>>
>>>>>> Any advice and suggestions for this idea or new idea will be
>>>>>> greatly appreciated!
>>>>>>
>>>>>> Best regards,
>>>>>>
>>>>>> James zhang
>>>>>>
>>>>> Sounds very good. And we may also need features bits
>>>>> (VIRTIO_NET_F_GUEST|HOST_GSO_X) for this.
>>>>>
>>>>> This is in fact one of items in networking todo list. (See
>>>>> http://www.linux-kvm.org/page/NetworkingTodo). While at it, we'd
>>>>> better support not only VXLAN but also other tunnels.
>>>> Cc Vlad who is working on extending virtio-net headers.
>>>>
>>>>> We can start with the spec work, or if you've already had some bits
>>>>> you can post them as RFC for early review.
>>>>>
>>>>> Thanks
>>> Below is my demo code
>>> Virtio_net.c
>>> static int virtnet_probe(struct virtio_device *vdev), add belows codes:
>>> if (virtio_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF) || // avoid gso segment, it should be negotiation later, because in the demo I reuse num_buffers.
>>> virtio_has_feature(vdev, VIRTIO_F_VERSION_1)) {
>>> dev->hw_enc_features |= NETIF_F_TSO;
>>> dev->hw_enc_features |= NETIF_F_ALL_CSUM;
>>> dev->hw_enc_features |= NETIF_F_GSO_UDP_TUNNEL;
>>> dev->hw_enc_features |= NETIF_F_GSO_UDP_TUNNEL_CSUM;
>>> dev->hw_enc_features |= NETIF_F_GSO_TUNNEL_REMCSUM;
>>>
>>> dev->features |= NETIF_F_GSO_UDP_TUNNEL;
>>> dev->features |= NETIF_F_GSO_UDP_TUNNEL_CSUM;
>>> dev->features |= NETIF_F_GSO_TUNNEL_REMCSUM;
>>> }
>>>
>>> static int xmit_skb(struct send_queue *sq, struct sk_buff *skb), add
>>> below to pieces of codes
>>>
>>> if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_TUNNEL)
>>> hdr->hdr.gso_type |= VIRTIO_NET_HDR_GSO_TUNNEL;
>>> if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_TUNNEL_CSUM)
>>> hdr->hdr.gso_type |= VIRTIO_NET_HDR_GSO_TUNNEL_CSUM;
>>> if (skb_shinfo(skb)->gso_type & SKB_GSO_TUNNEL_REMCSUM)
>>> hdr->hdr.gso_type |=
>>> VIRTIO_NET_HDR_GSO_TUNNEL_REMCSUM;
>>>
>>> if (skb->encapsulation && skb_is_gso(skb)) {
>>> inner_mac_len = skb_inner_network_header(skb) - skb_inner_mac_header(skb);
>>> tnl_len = skb_inner_mac_header(skb) - skb_mac_header(skb);
>>> if ( !(inner_mac_len >> DATA_LEN_SHIFT) && !(tnl_len >> DATA_LEN_SHIFT) ) {
>>> hdr->hdr.flags |= VIRTIO_NET_HDR_F_ENCAPSULATION;
>>> hdr->num_buffers = (__virtio16)((inner_mac_len << DATA_LEN_SHIFT) | tnl_len); //we reuse num_buffers for simple , we should add extend member for later.
>>> } else
>>> hdr->num_buffers = 0;
>>> }
>>>
>>> Tun.c
>>> if (memcpy_fromiovecend((void *)&hdr, iv, offset, tun->vnet_hdr_sz)) //read header with negotiation length
>>> return -EFAULT;
>>>
>>> if (hdr.gso_type & VIRTIO_NET_HDR_GSO_TUNNEL) //set tunnel gso info
>>> skb_shinfo(skb)->gso_type |= SKB_GSO_UDP_TUNNEL;
>>> if (hdr.gso_type & VIRTIO_NET_HDR_GSO_TUNNEL_CSUM)
>>> skb_shinfo(skb)->gso_type |= SKB_GSO_UDP_TUNNEL_CSUM;
>>> if (hdr.gso_type & VIRTIO_NET_HDR_GSO_TUNNEL_REMCSUM)
>>> skb_shinfo(skb)->gso_type |=
>>> SKB_GSO_TUNNEL_REMCSUM;
>>>
>>> if (hdr.flags & VIRTIO_NET_HDR_F_ENCAPSULATION) { //read tunnel info from header and set to built skb.
>>> tnl_len = tun16_to_cpu(tun, hdr.num_buffers) & TUN_TNL_LEN_MASK;
>>> payload_mac_len = tun16_to_cpu(tun, hdr.num_buffers) >> TUN_DATA_LEN_SHIFT;
>>> mac_len = skb_network_header(skb) - skb_mac_header(skb);
>>> skb_set_inner_mac_header(skb, tnl_len - mac_len);
>>> skb_set_inner_network_header(skb, tnl_len + payload_mac_len - mac_len);
>>> skb->encapsulation = 1;
>>> }
>>>
>>>
>> Something like this, and you probably need do something more:
>>
>> - use net-next.git to generate the patch (for the latest code)
>> - add feature negotiation
>> - tun/macvtap/qemu patches for this, you can start with tun/macvtap patches
>> - support for all other SKB_GSO_* types which is not supported
>> - use a new field instead of num_buffers
>> - a virtio spec patch to describe the support for encapsulation offload
>>
>> Thanks
> Thank you for your advice, I will start it right now.
>
> Thanks
Cool, one more question: while at it, I think you may want to add
support for dpdk too?
Thanks
^ permalink raw reply
* Re: [PATCH 2/3] vhost: better detection of available buffers
From: Jason Wang @ 2016-11-15 3:16 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, linux-kernel
In-Reply-To: <20161111182023-mutt-send-email-mst@kernel.org>
On 2016年11月12日 00:20, Michael S. Tsirkin wrote:
> On Fri, Nov 11, 2016 at 12:18:50PM +0800, Jason Wang wrote:
>>
>> On 2016年11月11日 11:41, Michael S. Tsirkin wrote:
>>> On Fri, Nov 11, 2016 at 10:18:37AM +0800, Jason Wang wrote:
>>>>>
>>>>> On 2016年11月10日 03:57, Michael S. Tsirkin wrote:
>>>>>>> On Wed, Nov 09, 2016 at 03:38:32PM +0800, Jason Wang wrote:
>>>>>>>>> We should use vq->last_avail_idx instead of vq->avail_idx in the
>>>>>>>>> checking of vhost_vq_avail_empty() since latter is the cached avail
>>>>>>>>> index from guest but we want to know if there's pending available
>>>>>>>>> buffers in the virtqueue.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Jason Wang<jasowang@redhat.com>
>>>>>>> I'm not sure why is this patch here. Is it related to
>>>>>>> batching somehow?
>>>>> Yes, we need to know whether or not there's still buffers left in the
>>>>> virtqueue, so need to check last_avail_idx. Otherwise, we're checking if
>>>>> guest has submitted new buffers.
>>>>>
>>>>>>>
>>>>>>>>> ---
>>>>>>>>> drivers/vhost/vhost.c | 2 +-
>>>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>>>>>>>>> index c6f2d89..fdf4cdf 100644
>>>>>>>>> --- a/drivers/vhost/vhost.c
>>>>>>>>> +++ b/drivers/vhost/vhost.c
>>>>>>>>> @@ -2230,7 +2230,7 @@ bool vhost_vq_avail_empty(struct vhost_dev *dev, struct vhost_virtqueue *vq)
>>>>>>>>> if (r)
>>>>>>>>> return false;
>>>>>>>>> - return vhost16_to_cpu(vq, avail_idx) == vq->avail_idx;
>>>>>>>>> + return vhost16_to_cpu(vq, avail_idx) == vq->last_avail_idx;
>>>>>>>>> }
>>>>>>>>> EXPORT_SYMBOL_GPL(vhost_vq_avail_empty);
>>>>>>> That might be OK for TX but it's probably wrong for RX
>>>>>>> where the fact that used != avail does not mean
>>>>>>> we have enough space to store the packet.
>>>>> Right, but it's no harm since it was just a hint, handle_rx() can handle
>>>>> this situation.
>>> Means busy polling will cause useless load on the CPU though.
>>>
>> Right, but,it's not easy to have 100% correct hint here. Needs more thought.
> What's wrong with what we have? It polls until value changes.
>
But as you said, this does not mean (in mergeable cases) we have enough
space to store the packet.
^ permalink raw reply
* Re: [PATCH 1/3] tuntap: rx batching
From: Jason Wang @ 2016-11-15 3:14 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: John Fastabend, netdev, linux-kernel
In-Reply-To: <20161111181754-mutt-send-email-mst@kernel.org>
On 2016年11月12日 00:20, Michael S. Tsirkin wrote:
> On Fri, Nov 11, 2016 at 12:28:38PM +0800, Jason Wang wrote:
>>
>> On 2016年11月11日 12:17, John Fastabend wrote:
>>> On 16-11-10 07:31 PM, Michael S. Tsirkin wrote:
>>>>> On Fri, Nov 11, 2016 at 10:07:44AM +0800, Jason Wang wrote:
>>>>>>>
>>>>>>> On 2016年11月10日 00:38, Michael S. Tsirkin wrote:
>>>>>>>>> On Wed, Nov 09, 2016 at 03:38:31PM +0800, Jason Wang wrote:
>>>>>>>>>>> Backlog were used for tuntap rx, but it can only process 1 packet at
>>>>>>>>>>> one time since it was scheduled during sendmsg() synchronously in
>>>>>>>>>>> process context. This lead bad cache utilization so this patch tries
>>>>>>>>>>> to do some batching before call rx NAPI. This is done through:
>>>>>>>>>>>
>>>>>>>>>>> - accept MSG_MORE as a hint from sendmsg() caller, if it was set,
>>>>>>>>>>> batch the packet temporarily in a linked list and submit them all
>>>>>>>>>>> once MSG_MORE were cleared.
>>>>>>>>>>> - implement a tuntap specific NAPI handler for processing this kind of
>>>>>>>>>>> possible batching. (This could be done by extending backlog to
>>>>>>>>>>> support skb like, but using a tun specific one looks cleaner and
>>>>>>>>>>> easier for future extension).
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Jason Wang<jasowang@redhat.com>
>>>>>>>>> So why do we need an extra queue?
>>>>>>> The idea was borrowed from backlog to allow some kind of bulking and avoid
>>>>>>> spinlock on each dequeuing.
>>>>>>>
>>>>>>>>> This is not what hardware devices do.
>>>>>>>>> How about adding the packet to queue unconditionally, deferring
>>>>>>>>> signalling until we get sendmsg without MSG_MORE?
>>>>>>> Then you need touch spinlock when dequeuing each packet.
>>> Random thought, I have a cmpxchg ring I am using for the qdisc work that
>>> could possibly replace the spinlock implementation. I haven't figured
>>> out the resizing API yet because I did not need it but I assume it could
>>> help here and let you dequeue multiple skbs in one operation.
>>>
>>> I can post the latest version if useful or an older version is
>>> somewhere on patchworks as well.
>>>
>>> .John
>>>
>>>
>> Look useful here, and I can compare the performance if you post.
>>
>> A question is can we extend the skb_array to support that?
>>
>> Thanks
> I'd like to start with simple patch adding napi with one queue, then add
> optimization patches on top.
The point is tun is using backlog who uses two queues (process_queue and
input_pkt_queue).
How about something like:
1) NAPI support with skb_array
2) MSG_MORE support
3) other optimizations on top
?
>
> One issue that comes to mind is that write queue limits
> are byte based, they do not count packets unlike tun rx queue.
I'm not sure I get the issue, write queue is not exported and only used
for batching. We probably need an internal limit in tun to avoid OOM
attacker from guest.
Thanks
^ permalink raw reply
* Re: [PATCH net][v2] bpf: fix range arithmetic for bpf map access
From: Alexei Starovoitov @ 2016-11-15 3:10 UTC (permalink / raw)
To: Josef Bacik; +Cc: jannh, ast, daniel, davem, netdev
In-Reply-To: <1479156336-6211-1-git-send-email-jbacik@fb.com>
On Mon, Nov 14, 2016 at 03:45:36PM -0500, Josef Bacik wrote:
> I made some invalid assumptions with BPF_AND and BPF_MOD that could result in
> invalid accesses to bpf map entries. Fix this up by doing a few things
>
> 1) Kill BPF_MOD support. This doesn't actually get used by the compiler in real
> life and just adds extra complexity.
>
> 2) Fix the logic for BPF_AND, don't allow AND of negative numbers and set the
> minimum value to 0 for positive AND's.
>
> 3) Don't do operations on the ranges if they are set to the limits, as they are
> by definition undefined, and allowing arithmetic operations on those values
> could make them appear valid when they really aren't.
>
> This fixes the testcase provided by Jann as well as a few other theoretical
> problems.
>
> Reported-by: Jann Horn <jannh@google.com>
> Signed-off-by: Josef Bacik <jbacik@fb.com>
lgtm.
Acked-by: Alexei Starovoitov <ast@kernel.org>
Jann, could you please double check the logic.
Thanks!
^ permalink raw reply
* Re: [PATCH v2 net-next 1/5] bpf: Refactor cgroups code in prep for new type
From: Alexei Starovoitov @ 2016-11-15 2:23 UTC (permalink / raw)
To: David Ahern, Thomas Graf, Daniel Mack
Cc: David Miller, netdev, daniel, maheshb
In-Reply-To: <65b46edc-02bd-415c-4b1d-af36928d22da@cumulusnetworks.com>
On 11/13/16 7:51 PM, David Ahern wrote:
> On 10/31/16 11:49 AM, Thomas Graf wrote:
>> On 10/31/16 at 06:16pm, Daniel Mack wrote:
>>> On 10/31/2016 06:05 PM, David Ahern wrote:
>>>> On 10/31/16 11:00 AM, Daniel Mack wrote:
>>>>> Yeah, I'm confused too. I changed that name in my v7 from
>>>>> BPF_PROG_TYPE_CGROUP_SOCK to BPF_PROG_TYPE_CGROUP_SKB on David's
>>>>> (Ahern) request. Why is it now renamed again?
>>>>
>>>> Thomas pushed back on adding another program type in favor of using
>>>> subtypes. So this makes the program type generic to CGROUP and patch
>>>> 2 in this v2 set added Mickaël's subtype patch with the socket
>>>> mangling done that way in patch 3.
>>>>
>>>
>>> Fine for me. I can change it around again.
>>
>> I would like to hear from Daniel B and Alexei as well. We need to
>> decide whether to use subtypes consistently and treat prog types as
>> something more high level or whether to bluntly introduce a new prog
>> type for every distinct set of verifier limits. I will change lwt_bpf
>> as well accordingly.
>>
>
> Alexei / Daniel - any comments/preferences on subtypes vs program types?
looks like in this particular case it's better to use different program
types, since they all serve different purpose and context is different.
Daniel Mack's programs can stay BPF_PROG_TYPE_CGROUP_SKB and operate on
skb, whereas your ifindex changing programs can be
BPF_PROG_TYPE_CGROUP_SOCK.
regarding DanielM's patches.. Dave's comment regarding skb->sk vs sk
made us think hard about it. We looked into tunnels and at the end
realized that skb->sk won't fly, since the program will be called
multiple times on tx for every ip_output. And since we were not sure
that using 'sk' will fix it, we had to do a bunch of tests with
different tunnels and analyze the code to make sure it's all good.
We'll repost soon.
^ permalink raw reply
* [PATCH net-next] tcp: allow to enable the repair mode for non-listening sockets
From: Andrei Vagin @ 2016-11-15 2:15 UTC (permalink / raw)
To: David S. Miller
Cc: linux-kernel, netdev, Alexey Kuznetsov, James Morris,
Hideaki YOSHIFUJI, Patrick McHardy, criu, Andrei Vagin
The repair mode is used to get and restore sequence numbers and
data from queues. It used to checkpoint/restore connections.
Currently the repair mode can be enabled for sockets in the established
and closed states, but for other states we have to dump the same socket
properties, so lets allow to enable repair mode for these sockets.
The repair mode reveals nothing more for sockets in other states.
Signed-off-by: Andrei Vagin <avagin@openvz.org>
---
net/ipv4/tcp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 3251fe7..a2a3a8c 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -2302,7 +2302,7 @@ EXPORT_SYMBOL(tcp_disconnect);
static inline bool tcp_can_repair_sock(const struct sock *sk)
{
return ns_capable(sock_net(sk)->user_ns, CAP_NET_ADMIN) &&
- ((1 << sk->sk_state) & (TCPF_CLOSE | TCPF_ESTABLISHED));
+ (sk->sk_state != TCP_LISTEN);
}
static int tcp_repair_set_window(struct tcp_sock *tp, char __user *optbuf, int len)
--
2.7.4
^ permalink raw reply related
* Re: [PATCH v2 net-next 2/6] bpf: Add percpu LRU list
From: Alexei Starovoitov @ 2016-11-15 1:51 UTC (permalink / raw)
To: Martin KaFai Lau
Cc: netdev, David Miller, Alexei Starovoitov, Daniel Borkmann,
Kernel Team
In-Reply-To: <1478890511-1346984-3-git-send-email-kafai@fb.com>
On Fri, Nov 11, 2016 at 10:55:07AM -0800, Martin KaFai Lau wrote:
> Instead of having a common LRU list, this patch allows a
> percpu LRU list which can be selected by specifying a map
> attribute. The map attribute will be added in the later
> patch.
>
> While the common use case for LRU is #reads >> #updates,
> percpu LRU list allows bpf prog to absorb unusual #updates
> under pathological case (e.g. external traffic facing machine which
> could be under attack).
>
> Each percpu LRU is isolated from each other. The LRU nodes (including
> free nodes) cannot be moved across different LRU Lists.
>
> Here are the update performance comparison between
> common LRU list and percpu LRU list (the test code is
> at the last patch):
>
> [root@kerneltest003.31.prn1 ~]# for i in 1 4 8; do echo -n "$i cpus: "; \
> ./map_perf_test 16 $i | awk '{r += $3}END{print r " updates"}'; done
> 1 cpus: 2934082 updates
> 4 cpus: 7391434 updates
> 8 cpus: 6500576 updates
>
> [root@kerneltest003.31.prn1 ~]# for i in 1 4 8; do echo -n "$i cpus: "; \
> ./map_perf_test 32 $i | awk '{r += $3}END{printr " updates"}'; done
> 1 cpus: 2896553 updates
> 4 cpus: 9766395 updates
> 8 cpus: 17460553 updates
>
> Signed-off-by: Martin KaFai Lau <kafai@fb.com>
Acked-by: Alexei Starovoitov <ast@kernel.org>
^ permalink raw reply
* Re: [PATCH v2 net-next 1/6] bpf: LRU List
From: Alexei Starovoitov @ 2016-11-15 1:50 UTC (permalink / raw)
To: Martin KaFai Lau
Cc: netdev, David Miller, Alexei Starovoitov, Daniel Borkmann,
Kernel Team
In-Reply-To: <1478890511-1346984-2-git-send-email-kafai@fb.com>
On Fri, Nov 11, 2016 at 10:55:06AM -0800, Martin KaFai Lau wrote:
> Introduce bpf_lru_list which will provide LRU capability to
> the bpf_htab in the later patch.
>
> * General Thoughts:
> 1. Target use case. Read is more often than update.
> (i.e. bpf_lookup_elem() is more often than bpf_update_elem()).
> If bpf_prog does a bpf_lookup_elem() first and then an in-place
> update, it still counts as a read operation to the LRU list concern.
> 2. It may be useful to think of it as a LRU cache
> 3. Optimize the read case
> 3.1 No lock in read case
> 3.2 The LRU maintenance is only done during bpf_update_elem()
> 4. If there is a percpu LRU list, it will lose the system-wise LRU
> property. A completely isolated percpu LRU list has the best
> performance but the memory utilization is not ideal considering
> the work load may be imbalance.
> 5. Hence, this patch starts the LRU implementation with a global LRU
> list with batched operations before accessing the global LRU list.
> As a LRU cache, #read >> #update/#insert operations, it will work well.
> 6. There is a local list (for each cpu) which is named
> 'struct bpf_lru_locallist'. This local list is not used to sort
> the LRU property. Instead, the local list is to batch enough
> operations before acquiring the lock of the global LRU list. More
> details on this later.
> 7. In the later patch, it allows a percpu LRU list by specifying a
> map-attribute for scalability reason and for use cases that need to
> prepare for the worst (and pathological) case like DoS attack.
> The percpu LRU list is completely isolated from each other and the
> LRU nodes (including free nodes) cannot be moved across the list. The
> following description is for the global LRU list but mostly applicable
> to the percpu LRU list also.
>
> * Global LRU List:
> 1. It has three sub-lists: active-list, inactive-list and free-list.
> 2. The two list idea, active and inactive, is borrowed from the
> page cache.
> 3. All nodes are pre-allocated and all sit at the free-list (of the
> global LRU list) at the beginning. The pre-allocation reasoning
> is similar to the existing BPF_MAP_TYPE_HASH. However,
> opting-out prealloc (BPF_F_NO_PREALLOC) is not supported in
> the LRU map.
>
> * Active/Inactive List (of the global LRU list):
> 1. The active list, as its name says it, maintains the active set of
> the nodes. We can think of it as the working set or more frequently
> accessed nodes. The access frequency is approximated by a ref-bit.
> The ref-bit is set during the bpf_lookup_elem().
> 2. The inactive list, as its name also says it, maintains a less
> active set of nodes. They are the candidates to be removed
> from the bpf_htab when we are running out of free nodes.
> 3. The ordering of these two lists is acting as a rough clock.
> The tail of the inactive list is the older nodes and
> should be released first if the bpf_htab needs free element.
>
> * Rotating the Active/Inactive List (of the global LRU list):
> 1. It is the basic operation to maintain the LRU property of
> the global list.
> 2. The active list is only rotated when the inactive list is running
> low. This idea is similar to the current page cache.
> Inactive running low is currently defined as
> "# of inactive < # of active".
> 3. The active list rotation always starts from the tail. It moves
> node without ref-bit set to the head of the inactive list.
> It moves node with ref-bit set back to the head of the active
> list and then clears its ref-bit.
> 4. The inactive rotation is pretty simply.
> It walks the inactive list and moves the nodes back to the head of
> active list if its ref-bit is set. The ref-bit is cleared after moving
> to the active list.
> If the node does not have ref-bit set, it just leave it as it is
> because it is already in the inactive list.
>
> * Shrinking the Inactive List (of the global LRU list):
> 1. Shrinking is the operation to get free nodes when the bpf_htab is
> full.
> 2. It usually only shrinks the inactive list to get free nodes.
> 3. During shrinking, it will walk the inactive list from the tail,
> delete the nodes without ref-bit set from bpf_htab.
> 4. If no free node found after step (3), it will forcefully get
> one node from the tail of inactive or active list. Forcefully is
> in the sense that it ignores the ref-bit.
>
> * Local List:
> 1. Each CPU has a 'struct bpf_lru_locallist'. The purpose is to
> batch enough operations before acquiring the lock of the
> global LRU.
> 2. A local list has two sub-lists, free-list and pending-list.
> 3. During bpf_update_elem(), it will try to get from the free-list
> of (the current CPU local list).
> 4. If the local free-list is empty, it will acquire from the
> global LRU list. The global LRU list can either satisfy it
> by its global free-list or by shrinking the global inactive
> list. Since we have acquired the global LRU list lock,
> it will try to get at most LOCAL_FREE_TARGET elements
> to the local free list.
> 5. When a new element is added to the bpf_htab, it will
> first sit at the pending-list (of the local list) first.
> The pending-list will be flushed to the global LRU list
> when it needs to acquire free nodes from the global list
> next time.
>
> * Lock Consideration:
> The LRU list has a lock (lru_lock). Each bucket of htab has a
> lock (buck_lock). If both locks need to be acquired together,
> the lock order is always lru_lock -> buck_lock and this only
> happens in the bpf_lru_list.c logic.
>
> In hashtab.c, both locks are not acquired together (i.e. one
> lock is always released first before acquiring another lock).
>
> Signed-off-by: Martin KaFai Lau <kafai@fb.com>
thanks for detailed commit log.
I think it's worth adding it to bpf_lru_list.c as design documentation.
Acked-by: Alexei Starovoitov <ast@kernel.org>
^ permalink raw reply
* Re: [PATCH v2 net-next 6/6] bpf: Add tests for the LRU bpf_htab
From: Alexei Starovoitov @ 2016-11-15 1:43 UTC (permalink / raw)
To: Martin KaFai Lau
Cc: netdev, David Miller, Alexei Starovoitov, Daniel Borkmann,
Kernel Team
In-Reply-To: <1478890511-1346984-7-git-send-email-kafai@fb.com>
On Fri, Nov 11, 2016 at 10:55:11AM -0800, Martin KaFai Lau wrote:
> This patch has some unit tests and a test_lru_dist.
>
> The test_lru_dist reads in the numeric keys from a file.
> The files used here are generated by a modified fio-genzipf tool
> originated from the fio test suit. The sample data file can be
> found here: https://github.com/iamkafai/bpf-lru
>
> The zipf.* data files have 100k numeric keys and the key is also
> ranged from 1 to 100k.
>
> The test_lru_dist outputs the number of unique keys (nr_unique).
> F.e. The following means, 61239 of them is unique out of 100k keys.
> nr_misses means it cannot be found in the LRU map, so nr_misses
> must be >= nr_unique. test_lru_dist also simulates a perfect LRU
> map as a comparison:
>
> [root@arch-fb-vm1 ~]# ~/devshare/fb-kernel/linux/samples/bpf/test_lru_dist \
> /root/zipf.100k.a1_01.out 4000 1
> ...
> test_parallel_lru_dist (map_type:9 map_flags:0x0):
> task:0 BPF LRU: nr_unique:23093(/100000) nr_misses:31603(/100000)
> task:0 Perfect LRU: nr_unique:23093(/100000 nr_misses:34328(/100000)
> ....
> test_parallel_lru_dist (map_type:9 map_flags:0x2):
> task:0 BPF LRU: nr_unique:23093(/100000) nr_misses:31710(/100000)
> task:0 Perfect LRU: nr_unique:23093(/100000 nr_misses:34328(/100000)
>
> [root@arch-fb-vm1 ~]# ~/devshare/fb-kernel/linux/samples/bpf/test_lru_dist \
> /root/zipf.100k.a0_01.out 40000 1
> ...
> test_parallel_lru_dist (map_type:9 map_flags:0x0):
> task:0 BPF LRU: nr_unique:61239(/100000) nr_misses:67054(/100000)
> task:0 Perfect LRU: nr_unique:61239(/100000 nr_misses:66993(/100000)
> ...
> test_parallel_lru_dist (map_type:9 map_flags:0x2):
> task:0 BPF LRU: nr_unique:61239(/100000) nr_misses:67068(/100000)
> task:0 Perfect LRU: nr_unique:61239(/100000 nr_misses:66993(/100000)
>
> LRU map has also been added to map_perf_test:
> /* Global LRU */
> [root@kerneltest003.31.prn1 ~]# for i in 1 4 8; do echo -n "$i cpus: "; \
> ./map_perf_test 16 $i | awk '{r += $3}END{print r " updates"}'; done
> 1 cpus: 2934082 updates
> 4 cpus: 7391434 updates
> 8 cpus: 6500576 updates
>
> /* Percpu LRU */
> [root@kerneltest003.31.prn1 ~]# for i in 1 4 8; do echo -n "$i cpus: "; \
> ./map_perf_test 32 $i | awk '{r += $3}END{print r " updates"}'; done
> 1 cpus: 2896553 updates
> 4 cpus: 9766395 updates
> 8 cpus: 17460553 updates
>
> Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> ---
> samples/bpf/Makefile | 2 +
> samples/bpf/map_perf_test_kern.c | 39 ++
> samples/bpf/map_perf_test_user.c | 32 ++
> samples/bpf/test_lru_dist.c | 538 ++++++++++++++++++++++++++
> tools/testing/selftests/bpf/Makefile | 6 +-
> tools/testing/selftests/bpf/test_lru_map.c | 583 +++++++++++++++++++++++++++++
> 6 files changed, 1197 insertions(+), 3 deletions(-)
> create mode 100644 samples/bpf/test_lru_dist.c
> create mode 100644 tools/testing/selftests/bpf/test_lru_map.c
I guess test_lru_dist stayed in samples/bpf because it needs external
file and the output also not suitable for simple pass/fail and
needs to be deciphered by humans.
Thanks for the other two tests.
In the future we need to consider moving map_perf_test into selftests.
Acked-by: Alexei Starovoitov <ast@kernel.org>
^ permalink raw reply
* Re: [PATCH v2 net-next 5/6] bpf: Add BPF_MAP_TYPE_LRU_PERCPU_HASH
From: Alexei Starovoitov @ 2016-11-15 1:34 UTC (permalink / raw)
To: Martin KaFai Lau
Cc: netdev, David Miller, Alexei Starovoitov, Daniel Borkmann,
Kernel Team
In-Reply-To: <1478890511-1346984-6-git-send-email-kafai@fb.com>
On Fri, Nov 11, 2016 at 10:55:10AM -0800, Martin KaFai Lau wrote:
> Provide a LRU version of the existing BPF_MAP_TYPE_PERCPU_HASH
>
> Signed-off-by: Martin KaFai Lau <kafai@fb.com>
...
> + /* For LRU, we need to alloc before taking bucket's
> + * spinlock because LRU's elem alloc may need
> + * to remove older elem from htab and this removal
> + * operation will need a bucket lock.
> + */
> + if (map_flags != BPF_EXIST) {
> + l_new = prealloc_lru_pop(htab, key, hash);
> + if (!l_new)
> + return -ENOMEM;
> + }
> +
> + /* bpf_map_update_elem() can be called in_irq() */
> + raw_spin_lock_irqsave(&b->lock, flags);
> +
> + l_old = lookup_elem_raw(head, hash, key, key_size);
> +
> + ret = check_flags(htab, l_old, map_flags);
> + if (ret)
> + goto err;
> +
> + if (l_old) {
> + bpf_lru_node_set_ref(&l_old->lru_node);
> +
> + /* per-cpu hash map can update value in-place */
> + pcpu_copy_value(htab, htab_elem_get_ptr(l_old, key_size),
> + value, onallcpus);
> + } else {
> + pcpu_copy_value(htab, htab_elem_get_ptr(l_new, key_size),
> + value, onallcpus);
> + hlist_add_head_rcu(&l_new->hash_node, head);
> + l_new = NULL;
> + }
> + ret = 0;
> +err:
> + raw_spin_unlock_irqrestore(&b->lock, flags);
> + if (l_new)
> + bpf_lru_push_free(&htab->lru, &l_new->lru_node);
> + return ret;
> +}
definitely tricky code, but all looks correct.
Acked-by: Alexei Starovoitov <ast@kernel.org>
^ permalink raw reply
* Re: [PATCH net 2/2] r8152: rx descriptor check
From: Francois Romieu @ 2016-11-15 1:10 UTC (permalink / raw)
To: Hayes Wang
Cc: netdev@vger.kernel.org, nic_swsd, linux-kernel@vger.kernel.org,
linux-usb@vger.kernel.org, mlord@pobox.com
In-Reply-To: <0835B3720019904CB8F7AA43166CEEB20104EAF8@RTITMBSV03.realtek.com.tw>
Hayes Wang <hayeswang@realtek.com> :
> Francois Romieu [mailto:romieu@fr.zoreil.com]
> > Sent: Friday, November 11, 2016 8:13 PM
> [...]
> > Invalid packet size corrupted receive descriptors in Realtek's device
> > reminds of CVE-2009-4537.
>
> Do you mean that the driver would get a packet exceed the size
> which is set to RxMaxSize ?
If it was possible to get it wrong once, it should be possible to
get it wrong twice, especially if some part of the hardware design
is recycled. I don't mean anything else.
I won't speculate about some cache consistency issue or some badly
aborted dma transaction to explain the memory corruption.
--
Ueimor
^ 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