* RE: [PATCH 2/7] iommu: add api to get iommu_domain of a device
From: Bhushan Bharat-R65777 @ 2013-10-04 9:54 UTC (permalink / raw)
To: Alex Williamson
Cc: agraf@suse.de, Wood Scott-B07421, linux-pci@vger.kernel.org,
joro@8bytes.org, linux-kernel@vger.kernel.org,
iommu@lists.linux-foundation.org, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <1380127533.3030.319.camel@ul30vt.home>
DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogbGludXgtcGNpLW93bmVy
QHZnZXIua2VybmVsLm9yZyBbbWFpbHRvOmxpbnV4LXBjaS1vd25lckB2Z2VyLmtlcm5lbC5vcmdd
DQo+IE9uIEJlaGFsZiBPZiBBbGV4IFdpbGxpYW1zb24NCj4gU2VudDogV2VkbmVzZGF5LCBTZXB0
ZW1iZXIgMjUsIDIwMTMgMTA6MTYgUE0NCj4gVG86IEJodXNoYW4gQmhhcmF0LVI2NTc3Nw0KPiBD
Yzogam9yb0A4Ynl0ZXMub3JnOyBiZW5oQGtlcm5lbC5jcmFzaGluZy5vcmc7IGdhbGFrQGtlcm5l
bC5jcmFzaGluZy5vcmc7IGxpbnV4LQ0KPiBrZXJuZWxAdmdlci5rZXJuZWwub3JnOyBsaW51eHBw
Yy1kZXZAbGlzdHMub3psYWJzLm9yZzsgbGludXgtDQo+IHBjaUB2Z2VyLmtlcm5lbC5vcmc7IGFn
cmFmQHN1c2UuZGU7IFdvb2QgU2NvdHQtQjA3NDIxOyBpb21tdUBsaXN0cy5saW51eC0NCj4gZm91
bmRhdGlvbi5vcmc7IEJodXNoYW4gQmhhcmF0LVI2NTc3Nw0KPiBTdWJqZWN0OiBSZTogW1BBVENI
IDIvN10gaW9tbXU6IGFkZCBhcGkgdG8gZ2V0IGlvbW11X2RvbWFpbiBvZiBhIGRldmljZQ0KPiAN
Cj4gT24gVGh1LCAyMDEzLTA5LTE5IGF0IDEyOjU5ICswNTMwLCBCaGFyYXQgQmh1c2hhbiB3cm90
ZToNCj4gPiBUaGlzIGFwaSByZXR1cm4gdGhlIGlvbW11IGRvbWFpbiB0byB3aGljaCB0aGUgZGV2
aWNlIGlzIGF0dGFjaGVkLg0KPiA+IFRoZSBpb21tdV9kb21haW4gaXMgcmVxdWlyZWQgZm9yIG1h
a2luZyBBUEkgY2FsbHMgcmVsYXRlZCB0byBpb21tdS4NCj4gPiBGb2xsb3cgdXAgcGF0Y2hlcyB3
aGljaCB1c2UgdGhpcyBBUEkgdG8ga25vdyBpb21tdSBtYXBpbmcuDQo+ID4NCj4gPiBTaWduZWQt
b2ZmLWJ5OiBCaGFyYXQgQmh1c2hhbiA8YmhhcmF0LmJodXNoYW5AZnJlZXNjYWxlLmNvbT4NCj4g
PiAtLS0NCj4gPiAgZHJpdmVycy9pb21tdS9pb21tdS5jIHwgICAxMCArKysrKysrKysrDQo+ID4g
IGluY2x1ZGUvbGludXgvaW9tbXUuaCB8ICAgIDcgKysrKysrKw0KPiA+ICAyIGZpbGVzIGNoYW5n
ZWQsIDE3IGluc2VydGlvbnMoKyksIDAgZGVsZXRpb25zKC0pDQo+ID4NCj4gPiBkaWZmIC0tZ2l0
IGEvZHJpdmVycy9pb21tdS9pb21tdS5jIGIvZHJpdmVycy9pb21tdS9pb21tdS5jIGluZGV4DQo+
ID4gZmJlOWNhNy4uNmFjNWY1MCAxMDA2NDQNCj4gPiAtLS0gYS9kcml2ZXJzL2lvbW11L2lvbW11
LmMNCj4gPiArKysgYi9kcml2ZXJzL2lvbW11L2lvbW11LmMNCj4gPiBAQCAtNjk2LDYgKzY5Niwx
NiBAQCB2b2lkIGlvbW11X2RldGFjaF9kZXZpY2Uoc3RydWN0IGlvbW11X2RvbWFpbg0KPiA+ICpk
b21haW4sIHN0cnVjdCBkZXZpY2UgKmRldikgIH0NCj4gPiBFWFBPUlRfU1lNQk9MX0dQTChpb21t
dV9kZXRhY2hfZGV2aWNlKTsNCj4gPg0KPiA+ICtzdHJ1Y3QgaW9tbXVfZG9tYWluICppb21tdV9n
ZXRfZGV2X2RvbWFpbihzdHJ1Y3QgZGV2aWNlICpkZXYpIHsNCj4gPiArCXN0cnVjdCBpb21tdV9v
cHMgKm9wcyA9IGRldi0+YnVzLT5pb21tdV9vcHM7DQo+ID4gKw0KPiA+ICsJaWYgKHVubGlrZWx5
KG9wcyA9PSBOVUxMIHx8IG9wcy0+Z2V0X2Rldl9pb21tdV9kb21haW4gPT0gTlVMTCkpDQo+ID4g
KwkJcmV0dXJuIE5VTEw7DQo+ID4gKw0KPiA+ICsJcmV0dXJuIG9wcy0+Z2V0X2Rldl9pb21tdV9k
b21haW4oZGV2KTsgfQ0KPiA+ICtFWFBPUlRfU1lNQk9MX0dQTChpb21tdV9nZXRfZGV2X2RvbWFp
bik7DQo+IA0KPiBXaGF0IHByZXZlbnRzIHRoaXMgZnJvbSByYWNpbmcgaW9tbXVfZG9tYWluX2Zy
ZWUoKT8gIFRoZXJlJ3Mgbm8gcmVmZXJlbmNlcw0KPiBhY3F1aXJlZCwgc28gdGhlcmUncyBubyBy
ZWFzb24gZm9yIHRoZSBjYWxsZXIgdG8gYXNzdW1lIHRoZSBwb2ludGVyIGlzIHZhbGlkLg0KDQpT
b3JyeSBmb3IgbGF0ZSBxdWVyeSwgc29tZWhvdyB0aGlzIGVtYWlsIHdlbnQgaW50byBhIGZvbGRl
ciBhbmQgZXNjYXBlZDsNCg0KSnVzdCB0byBiZSBzdXJlLCB0aGVyZSBpcyBub3QgbG9jayBhdCBn
ZW5lcmljICJzdHJ1Y3QgaW9tbXVfZG9tYWluIiwgYnV0IElQIHNwZWNpZmljIHN0cnVjdHVyZSAo
bGluayBGU0wgZG9tYWluKSBsaW5rZWQgaW4gaW9tbXVfZG9tYWluLT5wcml2IGhhdmUgYSBsb2Nr
LCBzbyB3ZSBuZWVkIHRvIGVuc3VyZSB0aGlzIHJhY2UgaW4gRlNMIGlvbW11IGNvZGUgKHNheSBk
cml2ZXJzL2lvbW11L2ZzbF9wYW11X2RvbWFpbi5jKSwgcmlnaHQ/DQoNClRoYW5rcw0KLUJoYXJh
dA0KDQo+IA0KPiA+ICAvKg0KPiA+ICAgKiBJT01NVSBncm91cHMgYXJlIHJlYWxseSB0aGUgbmF0
cnVhbCB3b3JraW5nIHVuaXQgb2YgdGhlIElPTU1VLCBidXQNCj4gPiAgICogdGhlIElPTU1VIEFQ
SSB3b3JrcyBvbiBkb21haW5zIGFuZCBkZXZpY2VzLiAgQnJpZGdlIHRoYXQgZ2FwIGJ5DQo+ID4g
ZGlmZiAtLWdpdCBhL2luY2x1ZGUvbGludXgvaW9tbXUuaCBiL2luY2x1ZGUvbGludXgvaW9tbXUu
aCBpbmRleA0KPiA+IDdlYTMxOWUuLmZhMDQ2YmQgMTAwNjQ0DQo+ID4gLS0tIGEvaW5jbHVkZS9s
aW51eC9pb21tdS5oDQo+ID4gKysrIGIvaW5jbHVkZS9saW51eC9pb21tdS5oDQo+ID4gQEAgLTEy
Nyw2ICsxMjcsNyBAQCBzdHJ1Y3QgaW9tbXVfb3BzIHsNCj4gPiAgCWludCAoKmRvbWFpbl9zZXRf
d2luZG93cykoc3RydWN0IGlvbW11X2RvbWFpbiAqZG9tYWluLCB1MzIgd19jb3VudCk7DQo+ID4g
IAkvKiBHZXQgdGhlIG51bWVyIG9mIHdpbmRvdyBwZXIgZG9tYWluICovDQo+ID4gIAl1MzIgKCpk
b21haW5fZ2V0X3dpbmRvd3MpKHN0cnVjdCBpb21tdV9kb21haW4gKmRvbWFpbik7DQo+ID4gKwlz
dHJ1Y3QgaW9tbXVfZG9tYWluICooKmdldF9kZXZfaW9tbXVfZG9tYWluKShzdHJ1Y3QgZGV2aWNl
ICpkZXYpOw0KPiA+DQo+ID4gIAl1bnNpZ25lZCBsb25nIHBnc2l6ZV9iaXRtYXA7DQo+ID4gIH07
DQo+ID4gQEAgLTE5MCw2ICsxOTEsNyBAQCBleHRlcm4gaW50IGlvbW11X2RvbWFpbl93aW5kb3df
ZW5hYmxlKHN0cnVjdCBpb21tdV9kb21haW4NCj4gKmRvbWFpbiwgdTMyIHduZF9uciwNCj4gPiAg
CQkJCSAgICAgIHBoeXNfYWRkcl90IG9mZnNldCwgdTY0IHNpemUsDQo+ID4gIAkJCQkgICAgICBp
bnQgcHJvdCk7DQo+ID4gIGV4dGVybiB2b2lkIGlvbW11X2RvbWFpbl93aW5kb3dfZGlzYWJsZShz
dHJ1Y3QgaW9tbXVfZG9tYWluICpkb21haW4sDQo+ID4gdTMyIHduZF9ucik7DQo+ID4gK2V4dGVy
biBzdHJ1Y3QgaW9tbXVfZG9tYWluICppb21tdV9nZXRfZGV2X2RvbWFpbihzdHJ1Y3QgZGV2aWNl
ICpkZXYpOw0KPiA+ICAvKioNCj4gPiAgICogcmVwb3J0X2lvbW11X2ZhdWx0KCkgLSByZXBvcnQg
YWJvdXQgYW4gSU9NTVUgZmF1bHQgdG8gdGhlIElPTU1VIGZyYW1ld29yaw0KPiA+ICAgKiBAZG9t
YWluOiB0aGUgaW9tbXUgZG9tYWluIHdoZXJlIHRoZSBmYXVsdCBoYXMgaGFwcGVuZWQgQEAgLTI4
NCw2DQo+ID4gKzI4NiwxMSBAQCBzdGF0aWMgaW5saW5lIHZvaWQgaW9tbXVfZG9tYWluX3dpbmRv
d19kaXNhYmxlKHN0cnVjdA0KPiA+IGlvbW11X2RvbWFpbiAqZG9tYWluLCAgeyAgfQ0KPiA+DQo+
ID4gK3N0YXRpYyBpbmxpbmUgc3RydWN0IGlvbW11X2RvbWFpbiAqaW9tbXVfZ2V0X2Rldl9kb21h
aW4oc3RydWN0IGRldmljZQ0KPiA+ICsqZGV2KSB7DQo+ID4gKwlyZXR1cm4gTlVMTDsNCj4gPiAr
fQ0KPiA+ICsNCj4gPiAgc3RhdGljIGlubGluZSBwaHlzX2FkZHJfdCBpb21tdV9pb3ZhX3RvX3Bo
eXMoc3RydWN0IGlvbW11X2RvbWFpbg0KPiA+ICpkb21haW4sIGRtYV9hZGRyX3QgaW92YSkgIHsN
Cj4gPiAgCXJldHVybiAwOw0KPiANCj4gDQo+IA0KPiAtLQ0KPiBUbyB1bnN1YnNjcmliZSBmcm9t
IHRoaXMgbGlzdDogc2VuZCB0aGUgbGluZSAidW5zdWJzY3JpYmUgbGludXgtcGNpIiBpbiB0aGUg
Ym9keQ0KPiBvZiBhIG1lc3NhZ2UgdG8gbWFqb3Jkb21vQHZnZXIua2VybmVsLm9yZyBNb3JlIG1h
am9yZG9tbyBpbmZvIGF0DQo+IGh0dHA6Ly92Z2VyLmtlcm5lbC5vcmcvbWFqb3Jkb21vLWluZm8u
aHRtbA0KDQo=
^ permalink raw reply
* Re: [PATCH RFC 00/77] Re-design MSI/MSI-X interrupts enablement pattern
From: Alexander Gordeev @ 2013-10-04 9:21 UTC (permalink / raw)
To: David Laight
Cc: linux-mips, VMware, Inc., linux-pci, linux-nvme, linux-ide,
Martin Schwidefsky, linux-s390, Andy King, linux-scsi, linux-rdma,
x86, Ingo Molnar, iss_storagedev, linuxppc-dev, linux-driver,
Ben Hutchings, Dan Williams, Jon Mason,
Solarflare linux maintainers, netdev, linux-kernel, Ralf Baechle,
e1000-devel, Tejun Heo, linux390, Bjorn Helgaas
In-Reply-To: <AE90C24D6B3A694183C094C60CF0A2F6026B7371@saturn3.aculab.com>
On Fri, Oct 04, 2013 at 09:31:49AM +0100, David Laight wrote:
> > Mmmm.. I am not sure I am getting it. Could you please rephrase?
>
> One possibility is for drivers than can use a lot of interrupts to
> request a minimum number initially and then request the additional
> ones much later on.
> That would make it less likely that none will be available for
> devices/drivers that need them but are initialised later.
It sounds as a whole new topic for me. Isn't it?
Anyway, what prevents the above from being done with pci_enable_msix(nvec1) -
pci_disable_msix() - pci_enable_msix(nvec2) where nvec1 < nvec2?
> David
--
Regards,
Alexander Gordeev
agordeev@redhat.com
^ permalink raw reply
* RE: [PATCH RFC 00/77] Re-design MSI/MSI-X interrupts enablement pattern
From: David Laight @ 2013-10-04 8:31 UTC (permalink / raw)
To: Alexander Gordeev, Ben Hutchings
Cc: linux-mips, VMware, Inc., linux-pci, linux-nvme, linux-ide,
Martin Schwidefsky, linux-s390, Andy King, linux-scsi,
e1000-devel, x86, Ingo Molnar, iss_storagedev, Tejun Heo,
Bjorn Helgaas, Dan Williams, Jon Mason,
Solarflare linux maintainers, netdev, linux-kernel, Ralf Baechle,
linux-rdma, linux-driver, linux390, linuxppc-dev
In-Reply-To: <20131004082920.GA4536@dhcp-26-207.brq.redhat.com>
> > It seems to me that a more useful interface would take a minimum and
> > maximum number of vectors from the driver. This wouldn't allow the
> > driver to specify that it could only accept, say, any even number =
within
> > a certain range, but you could still leave the current functions
> > available for any driver that needs that.
>=20
> Mmmm.. I am not sure I am getting it. Could you please rephrase?
One possibility is for drivers than can use a lot of interrupts to
request a minimum number initially and then request the additional
ones much later on.
That would make it less likely that none will be available for
devices/drivers that need them but are initialised later.
David
^ permalink raw reply
* Re: [PATCH RFC 00/77] Re-design MSI/MSI-X interrupts enablement pattern
From: Alexander Gordeev @ 2013-10-04 8:29 UTC (permalink / raw)
To: Ben Hutchings
Cc: linux-mips, VMware, Inc., linux-nvme, linux-ide, linux-s390,
Andy King, linux-scsi, linux-rdma, x86, Ingo Molnar, linux-pci,
iss_storagedev, linux-driver, Tejun Heo, Bjorn Helgaas,
Dan Williams, Jon Mason, Solarflare linux maintainers, netdev,
linux-kernel, Ralf Baechle, e1000-devel, Martin Schwidefsky,
linux390, linuxppc-dev
In-Reply-To: <1380840585.3419.50.camel@bwh-desktop.uk.level5networks.com>
On Thu, Oct 03, 2013 at 11:49:45PM +0100, Ben Hutchings wrote:
> On Wed, 2013-10-02 at 12:48 +0200, Alexander Gordeev wrote:
> > This update converts pci_enable_msix() and pci_enable_msi_block()
> > interfaces to canonical kernel functions and makes them return a
> > error code in case of failure or 0 in case of success.
> [...]
>
> I think this is fundamentally flawed: pci_msix_table_size() and
> pci_get_msi_cap() can only report the limits of the *device* (which the
> driver usually already knows), whereas MSI allocation can also be
> constrained due to *global* limits on the number of distinct IRQs.
Even the current implementation by no means addresses it. Although it
might seem a case for architectures to report the number of IRQs available
for a driver to retry, in fact they all just fail. The same applies to
*any* other type of resource involved: irq_desc's, CPU interrupt vector
space, msi_desc's etc. No platform cares about it and just bails out once
a constrain met (please correct me if I am wrong here). Given that Linux
has been doing well even on embedded I think we should not change it.
The only exception to the above is pSeries platform which takes advantage
of the current design (to implement MSI quota). There are indications we
can satisfy pSeries requirements, but the design proposed in this RFC
is not going to change drastically anyway. The start of the discusstion
is here: https://lkml.org/lkml/2013/9/5/293
> Currently pci_enable_msix() will report a positive value if it fails due
> to the global limit. Your patch 7 removes that. pci_enable_msi_block()
> unfortunately doesn't appear to do this.
pci_enable_msi_block() can do more than one MSI only on x86 (with IOMMU),
but it does not bother to return positive numbers, indeed.
> It seems to me that a more useful interface would take a minimum and
> maximum number of vectors from the driver. This wouldn't allow the
> driver to specify that it could only accept, say, any even number within
> a certain range, but you could still leave the current functions
> available for any driver that needs that.
Mmmm.. I am not sure I am getting it. Could you please rephrase?
> Ben.
--
Regards,
Alexander Gordeev
agordeev@redhat.com
^ permalink raw reply
* Re: [PATCH v3 net-next] fix unsafe set_memory_rw from softirq
From: Ingo Molnar @ 2013-10-04 7:51 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Heiko Carstens, Eric Dumazet, Paul Mackerras, H. Peter Anvin,
sparclinux, Nicolas Dichtel, linux-s390, Russell King, x86,
James Morris, Ingo Molnar, Alexey Kuznetsov, Paul E. McKenney,
Xi Wang, Matt Evans, Thomas Gleixner, linux-arm-kernel,
Stelian Nirlu, Nicolas Schichan, Hideaki YOSHIFUJI, netdev,
linux-kernel, David S. Miller, Mircea Gherzan, Daniel Borkmann,
Martin Schwidefsky, linux390, linuxppc-dev, Patrick McHardy
In-Reply-To: <1380853446-30537-1-git-send-email-ast@plumgrid.com>
* Alexei Starovoitov <ast@plumgrid.com> wrote:
> on x86 system with net.core.bpf_jit_enable = 1
>
> sudo tcpdump -i eth1 'tcp port 22'
>
> causes the warning:
> [ 56.766097] Possible unsafe locking scenario:
> [ 56.766097]
> [ 56.780146] CPU0
> [ 56.786807] ----
> [ 56.793188] lock(&(&vb->lock)->rlock);
> [ 56.799593] <Interrupt>
> [ 56.805889] lock(&(&vb->lock)->rlock);
> [ 56.812266]
> [ 56.812266] *** DEADLOCK ***
> [ 56.812266]
> [ 56.830670] 1 lock held by ksoftirqd/1/13:
> [ 56.836838] #0: (rcu_read_lock){.+.+..}, at: [<ffffffff8118f44c>] vm_unmap_aliases+0x8c/0x380
> [ 56.849757]
> [ 56.849757] stack backtrace:
> [ 56.862194] CPU: 1 PID: 13 Comm: ksoftirqd/1 Not tainted 3.12.0-rc3+ #45
> [ 56.868721] Hardware name: System manufacturer System Product Name/P8Z77 WS, BIOS 3007 07/26/2012
> [ 56.882004] ffffffff821944c0 ffff88080bbdb8c8 ffffffff8175a145 0000000000000007
> [ 56.895630] ffff88080bbd5f40 ffff88080bbdb928 ffffffff81755b14 0000000000000001
> [ 56.909313] ffff880800000001 ffff880800000000 ffffffff8101178f 0000000000000001
> [ 56.923006] Call Trace:
> [ 56.929532] [<ffffffff8175a145>] dump_stack+0x55/0x76
> [ 56.936067] [<ffffffff81755b14>] print_usage_bug+0x1f7/0x208
> [ 56.942445] [<ffffffff8101178f>] ? save_stack_trace+0x2f/0x50
> [ 56.948932] [<ffffffff810cc0a0>] ? check_usage_backwards+0x150/0x150
> [ 56.955470] [<ffffffff810ccb52>] mark_lock+0x282/0x2c0
> [ 56.961945] [<ffffffff810ccfed>] __lock_acquire+0x45d/0x1d50
> [ 56.968474] [<ffffffff810cce6e>] ? __lock_acquire+0x2de/0x1d50
> [ 56.975140] [<ffffffff81393bf5>] ? cpumask_next_and+0x55/0x90
> [ 56.981942] [<ffffffff810cef72>] lock_acquire+0x92/0x1d0
> [ 56.988745] [<ffffffff8118f52a>] ? vm_unmap_aliases+0x16a/0x380
> [ 56.995619] [<ffffffff817628f1>] _raw_spin_lock+0x41/0x50
> [ 57.002493] [<ffffffff8118f52a>] ? vm_unmap_aliases+0x16a/0x380
> [ 57.009447] [<ffffffff8118f52a>] vm_unmap_aliases+0x16a/0x380
> [ 57.016477] [<ffffffff8118f44c>] ? vm_unmap_aliases+0x8c/0x380
> [ 57.023607] [<ffffffff810436b0>] change_page_attr_set_clr+0xc0/0x460
> [ 57.030818] [<ffffffff810cfb8d>] ? trace_hardirqs_on+0xd/0x10
> [ 57.037896] [<ffffffff811a8330>] ? kmem_cache_free+0xb0/0x2b0
> [ 57.044789] [<ffffffff811b59c3>] ? free_object_rcu+0x93/0xa0
> [ 57.051720] [<ffffffff81043d9f>] set_memory_rw+0x2f/0x40
> [ 57.058727] [<ffffffff8104e17c>] bpf_jit_free+0x2c/0x40
> [ 57.065577] [<ffffffff81642cba>] sk_filter_release_rcu+0x1a/0x30
> [ 57.072338] [<ffffffff811108e2>] rcu_process_callbacks+0x202/0x7c0
> [ 57.078962] [<ffffffff81057f17>] __do_softirq+0xf7/0x3f0
> [ 57.085373] [<ffffffff81058245>] run_ksoftirqd+0x35/0x70
>
> cannot reuse jited filter memory, since it's readonly,
> so use original bpf insns memory to hold work_struct
>
> defer kfree of sk_filter until jit completed freeing
>
> tested on x86_64 and i386
>
> Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
> ---
> arch/arm/net/bpf_jit_32.c | 1 +
> arch/powerpc/net/bpf_jit_comp.c | 1 +
> arch/s390/net/bpf_jit_comp.c | 4 +++-
> arch/sparc/net/bpf_jit_comp.c | 1 +
> arch/x86/net/bpf_jit_comp.c | 20 +++++++++++++++-----
> include/linux/filter.h | 11 +++++++++--
> net/core/filter.c | 11 +++++++----
> 7 files changed, 37 insertions(+), 12 deletions(-)
>
> diff --git a/arch/arm/net/bpf_jit_32.c b/arch/arm/net/bpf_jit_32.c
> index f50d223..99b44e0 100644
> --- a/arch/arm/net/bpf_jit_32.c
> +++ b/arch/arm/net/bpf_jit_32.c
> @@ -930,4 +930,5 @@ void bpf_jit_free(struct sk_filter *fp)
> {
> if (fp->bpf_func != sk_run_filter)
> module_free(NULL, fp->bpf_func);
> + kfree(fp);
> }
> diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c
> index bf56e33..2345bdb 100644
> --- a/arch/powerpc/net/bpf_jit_comp.c
> +++ b/arch/powerpc/net/bpf_jit_comp.c
> @@ -691,4 +691,5 @@ void bpf_jit_free(struct sk_filter *fp)
> {
> if (fp->bpf_func != sk_run_filter)
> module_free(NULL, fp->bpf_func);
> + kfree(fp);
> }
> diff --git a/arch/s390/net/bpf_jit_comp.c b/arch/s390/net/bpf_jit_comp.c
> index 7092392..a5df511 100644
> --- a/arch/s390/net/bpf_jit_comp.c
> +++ b/arch/s390/net/bpf_jit_comp.c
> @@ -881,7 +881,9 @@ void bpf_jit_free(struct sk_filter *fp)
> struct bpf_binary_header *header = (void *)addr;
>
> if (fp->bpf_func == sk_run_filter)
> - return;
> + goto free_filter;
> set_memory_rw(addr, header->pages);
> module_free(NULL, header);
> +free_filter:
> + kfree(fp);
> }
> diff --git a/arch/sparc/net/bpf_jit_comp.c b/arch/sparc/net/bpf_jit_comp.c
> index 9c7be59..218b6b2 100644
> --- a/arch/sparc/net/bpf_jit_comp.c
> +++ b/arch/sparc/net/bpf_jit_comp.c
> @@ -808,4 +808,5 @@ void bpf_jit_free(struct sk_filter *fp)
> {
> if (fp->bpf_func != sk_run_filter)
> module_free(NULL, fp->bpf_func);
> + kfree(fp);
> }
> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index 79c216a..1396a0a 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -772,13 +772,23 @@ out:
> return;
> }
>
> +static void bpf_jit_free_deferred(struct work_struct *work)
> +{
> + struct sk_filter *fp = container_of((void *)work, struct sk_filter,
> + insns);
> + unsigned long addr = (unsigned long)fp->bpf_func & PAGE_MASK;
> + struct bpf_binary_header *header = (void *)addr;
> +
> + set_memory_rw(addr, header->pages);
> + module_free(NULL, header);
> + kfree(fp);
> +}
Using the data type suggestions I make further below, this could be
written in a simpler form, as:
struct sk_filter *fp = container_of(work, struct sk_filter, work);
Also, a question, why do you mask with PAGE_MASK here:
unsigned long addr = (unsigned long)fp->bpf_func & PAGE_MASK;
?
AFAICS bpf_func is the module_alloc() result - and module code is page
aligned. So ->bpf_func is always page aligned here. (The sk_run_filter
special case cannot happen here.)
> +
> void bpf_jit_free(struct sk_filter *fp)
> {
> if (fp->bpf_func != sk_run_filter) {
> - unsigned long addr = (unsigned long)fp->bpf_func & PAGE_MASK;
> - struct bpf_binary_header *header = (void *)addr;
> -
> - set_memory_rw(addr, header->pages);
> - module_free(NULL, header);
> + struct work_struct *work = (struct work_struct *)fp->insns;
> + INIT_WORK(work, bpf_jit_free_deferred);
Missing newline between local variables and statements.
> + schedule_work(work);
> }
> }
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index a6ac848..5d66cd9 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -25,15 +25,20 @@ struct sk_filter
> {
> atomic_t refcnt;
> unsigned int len; /* Number of filter blocks */
> + struct rcu_head rcu;
> unsigned int (*bpf_func)(const struct sk_buff *skb,
> const struct sock_filter *filter);
> - struct rcu_head rcu;
> + /* insns start right after bpf_func, so that sk_run_filter() fetches
> + * first insn from the same cache line that was used to call into
> + * sk_run_filter()
> + */
> struct sock_filter insns[0];
Please use the customary (multi-line) comment style:
/*
* Comment .....
* ...... goes here.
*/
specified in Documentation/CodingStyle.
> };
>
> static inline unsigned int sk_filter_len(const struct sk_filter *fp)
> {
> - return fp->len * sizeof(struct sock_filter) + sizeof(*fp);
> + return max(fp->len * sizeof(struct sock_filter),
> + sizeof(struct work_struct)) + sizeof(*fp);
So, "sizeof(struct work_struct)) + sizeof(*fp)" is a pattern that repeats
a couple of times. Might make sense to stick that into a helper of its own
- but in general this open coded overlay allocation method looks a bit
fragile and not very obvious in isolation.
So it could be done a bit cleaner, using an anonymous union:
/*
* These two overlay, the work struct is used during workqueue
* driven teardown, when the instructions are not used anymore:
*/
union {
struct sock_filter insns[0];
struct work_struct work;
};
And then all the sizeof() calculations become obvious and sk_filter_len()
could be eliminated - I've marked the conversions in the code further
below.
> extern int sk_filter(struct sock *sk, struct sk_buff *skb);
> @@ -67,11 +72,13 @@ static inline void bpf_jit_dump(unsigned int flen, unsigned int proglen,
> }
> #define SK_RUN_FILTER(FILTER, SKB) (*FILTER->bpf_func)(SKB, FILTER->insns)
> #else
> +#include <linux/slab.h>
Inlines in the middle of header files are generally frowned upon.
The standard pattern is to put them at the top, that way it's easier to
see the dependencies and there's also less .config dependent inclusion,
which makes header hell cleanup work easier.
> static inline void bpf_jit_compile(struct sk_filter *fp)
> {
> }
> static inline void bpf_jit_free(struct sk_filter *fp)
> {
> + kfree(fp);
> }
> #define SK_RUN_FILTER(FILTER, SKB) sk_run_filter(SKB, FILTER->insns)
> #endif
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 6438f29..ad5eaba 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -644,7 +644,6 @@ void sk_filter_release_rcu(struct rcu_head *rcu)
> struct sk_filter *fp = container_of(rcu, struct sk_filter, rcu);
>
> bpf_jit_free(fp);
> - kfree(fp);
> }
> EXPORT_SYMBOL(sk_filter_release_rcu);
>
> @@ -677,13 +676,15 @@ int sk_unattached_filter_create(struct sk_filter **pfp,
> {
> struct sk_filter *fp;
> unsigned int fsize = sizeof(struct sock_filter) * fprog->len;
> + unsigned int sk_fsize = max_t(u32, fsize, sizeof(struct work_struct))
> + + sizeof(*fp);
Using the structure definition I suggested, this could be replaced with
the more obvious:
unsigned int sk_fsize = max(fsize, sizeof(*fp));
> int err;
>
> /* Make sure new filter is there and in the right amounts. */
> if (fprog->filter == NULL)
> return -EINVAL;
>
> - fp = kmalloc(fsize + sizeof(*fp), GFP_KERNEL);
> + fp = kmalloc(sk_fsize, GFP_KERNEL);
> if (!fp)
> return -ENOMEM;
> memcpy(fp->insns, fprog->filter, fsize);
> @@ -723,6 +724,8 @@ int sk_attach_filter(struct sock_fprog *fprog, struct sock *sk)
> {
> struct sk_filter *fp, *old_fp;
> unsigned int fsize = sizeof(struct sock_filter) * fprog->len;
> + unsigned int sk_fsize = max_t(u32, fsize, sizeof(struct work_struct))
> + + sizeof(*fp);
This too could be written as:
unsigned int sk_fsize = max(fsize, sizeof(*fp));
> int err;
>
> if (sock_flag(sk, SOCK_FILTER_LOCKED))
> @@ -732,11 +735,11 @@ int sk_attach_filter(struct sock_fprog *fprog, struct sock *sk)
> if (fprog->filter == NULL)
> return -EINVAL;
>
> - fp = sock_kmalloc(sk, fsize+sizeof(*fp), GFP_KERNEL);
> + fp = sock_kmalloc(sk, sk_fsize, GFP_KERNEL);
> if (!fp)
> return -ENOMEM;
> if (copy_from_user(fp->insns, fprog->filter, fsize)) {
> - sock_kfree_s(sk, fp, fsize+sizeof(*fp));
> + sock_kfree_s(sk, fp, sk_fsize);
> return -EFAULT;
> }
A couple of questions/suggestions:
1)
I took a brief look at arch/x86/net/bpf_jit_comp.c while reviewing this
patch.
You need to split up bpf_jit_compile(), it's an obscenely large, ~600
lines long function. We don't do that in modern, maintainable kernel code.
2)
This 128 bytes extra padding:
/* Most of BPF filters are really small,
* but if some of them fill a page, allow at least
* 128 extra bytes to insert a random section of int3
*/
sz = round_up(proglen + sizeof(*header) + 128, PAGE_SIZE);
why is it done? It's not clear to me from the comment.
3)
It's nice code altogether! Are there any plans to generalize its
interfaces, to allow arbitrary bytecode to be used by other kernel
subsystems as well? In particular tracing filters could make use of it,
but it would also allow safe probe points.
Thanks,
Ingo
^ permalink raw reply
* Re: [PATCH RFC 03/77] PCI/MSI/s390: Fix single MSI only check
From: Martin Schwidefsky @ 2013-10-04 7:39 UTC (permalink / raw)
To: Alexander Gordeev
Cc: linux-mips, VMware, Inc., linux-nvme, linux-ide, stable,
linux-s390, Andy King, linux-scsi, linux-rdma, x86, Ingo Molnar,
linux-pci, iss_storagedev, linux-driver, Bjorn Helgaas,
Dan Williams, Jon Mason, Solarflare linux maintainers, netdev,
linux-kernel, Ralf Baechle, e1000-devel, Tejun Heo, linux390,
linuxppc-dev
In-Reply-To: <8c9811b13fd93e73641dab8e3bd1bd5b2dc37a61.1380703262.git.agordeev@redhat.com>
On Wed, 2 Oct 2013 12:48:19 +0200
Alexander Gordeev <agordeev@redhat.com> wrote:
> Multiple MSIs have never been supported on s390 architecture,
> but the platform code fails to report single MSI only.
>
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> ---
> arch/s390/pci/pci.c | 2 ++
> 1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
> index f17a834..c79c6e4 100644
> --- a/arch/s390/pci/pci.c
> +++ b/arch/s390/pci/pci.c
> @@ -427,6 +427,8 @@ int arch_setup_msi_irqs(struct pci_dev *pdev, int nvec, int type)
> pr_debug("%s: requesting %d MSI-X interrupts...", __func__, nvec);
> if (type != PCI_CAP_ID_MSIX && type != PCI_CAP_ID_MSI)
> return -EINVAL;
> + if (type == PCI_CAP_ID_MSI && nvec > 1)
> + return 1;
> msi_vecs = min(nvec, ZPCI_MSI_VEC_MAX);
> msi_vecs = min_t(unsigned int, msi_vecs, CONFIG_PCI_NR_MSI);
>
Acked-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.
^ permalink raw reply
* Re: [PATCH RFC 04/77] PCI/MSI/s390: Remove superfluous check of MSI type
From: Martin Schwidefsky @ 2013-10-04 7:40 UTC (permalink / raw)
To: Alexander Gordeev
Cc: linux-mips, VMware, Inc., linux-nvme, linux-ide, stable,
linux-s390, Andy King, linux-scsi, linux-rdma, x86, Ingo Molnar,
linux-pci, iss_storagedev, linux-driver, Bjorn Helgaas,
Dan Williams, Jon Mason, Solarflare linux maintainers, netdev,
linux-kernel, Ralf Baechle, e1000-devel, Tejun Heo, linux390,
linuxppc-dev
In-Reply-To: <bae65aa3e30dfd23bd5ed47add7310cfbb96243a.1380703262.git.agordeev@redhat.com>
On Wed, 2 Oct 2013 12:48:20 +0200
Alexander Gordeev <agordeev@redhat.com> wrote:
> arch_setup_msi_irqs() hook can only be called from the generic
> MSI code which ensures correct MSI type parameter.
>
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> ---
> arch/s390/pci/pci.c | 2 --
> 1 files changed, 0 insertions(+), 2 deletions(-)
>
> diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
> index c79c6e4..61a3c2c 100644
> --- a/arch/s390/pci/pci.c
> +++ b/arch/s390/pci/pci.c
> @@ -425,8 +425,6 @@ int arch_setup_msi_irqs(struct pci_dev *pdev, int nvec, int type)
> int rc;
>
> pr_debug("%s: requesting %d MSI-X interrupts...", __func__, nvec);
> - if (type != PCI_CAP_ID_MSIX && type != PCI_CAP_ID_MSI)
> - return -EINVAL;
> if (type == PCI_CAP_ID_MSI && nvec > 1)
> return 1;
> msi_vecs = min(nvec, ZPCI_MSI_VEC_MAX);
Acked-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.
^ permalink raw reply
* [PATCH v4 net-next] fix unsafe set_memory_rw from softirq
From: Alexei Starovoitov @ 2013-10-04 7:14 UTC (permalink / raw)
To: David S. Miller
Cc: linux-s390, netdev, Heiko Carstens, Eric Dumazet, Daniel Borkmann,
linuxppc-dev, linux-arm-kernel
on x86 system with net.core.bpf_jit_enable = 1
sudo tcpdump -i eth1 'tcp port 22'
causes the warning:
[ 56.766097] Possible unsafe locking scenario:
[ 56.766097]
[ 56.780146] CPU0
[ 56.786807] ----
[ 56.793188] lock(&(&vb->lock)->rlock);
[ 56.799593] <Interrupt>
[ 56.805889] lock(&(&vb->lock)->rlock);
[ 56.812266]
[ 56.812266] *** DEADLOCK ***
[ 56.812266]
[ 56.830670] 1 lock held by ksoftirqd/1/13:
[ 56.836838] #0: (rcu_read_lock){.+.+..}, at: [<ffffffff8118f44c>] vm_unmap_aliases+0x8c/0x380
[ 56.849757]
[ 56.849757] stack backtrace:
[ 56.862194] CPU: 1 PID: 13 Comm: ksoftirqd/1 Not tainted 3.12.0-rc3+ #45
[ 56.868721] Hardware name: System manufacturer System Product Name/P8Z77 WS, BIOS 3007 07/26/2012
[ 56.882004] ffffffff821944c0 ffff88080bbdb8c8 ffffffff8175a145 0000000000000007
[ 56.895630] ffff88080bbd5f40 ffff88080bbdb928 ffffffff81755b14 0000000000000001
[ 56.909313] ffff880800000001 ffff880800000000 ffffffff8101178f 0000000000000001
[ 56.923006] Call Trace:
[ 56.929532] [<ffffffff8175a145>] dump_stack+0x55/0x76
[ 56.936067] [<ffffffff81755b14>] print_usage_bug+0x1f7/0x208
[ 56.942445] [<ffffffff8101178f>] ? save_stack_trace+0x2f/0x50
[ 56.948932] [<ffffffff810cc0a0>] ? check_usage_backwards+0x150/0x150
[ 56.955470] [<ffffffff810ccb52>] mark_lock+0x282/0x2c0
[ 56.961945] [<ffffffff810ccfed>] __lock_acquire+0x45d/0x1d50
[ 56.968474] [<ffffffff810cce6e>] ? __lock_acquire+0x2de/0x1d50
[ 56.975140] [<ffffffff81393bf5>] ? cpumask_next_and+0x55/0x90
[ 56.981942] [<ffffffff810cef72>] lock_acquire+0x92/0x1d0
[ 56.988745] [<ffffffff8118f52a>] ? vm_unmap_aliases+0x16a/0x380
[ 56.995619] [<ffffffff817628f1>] _raw_spin_lock+0x41/0x50
[ 57.002493] [<ffffffff8118f52a>] ? vm_unmap_aliases+0x16a/0x380
[ 57.009447] [<ffffffff8118f52a>] vm_unmap_aliases+0x16a/0x380
[ 57.016477] [<ffffffff8118f44c>] ? vm_unmap_aliases+0x8c/0x380
[ 57.023607] [<ffffffff810436b0>] change_page_attr_set_clr+0xc0/0x460
[ 57.030818] [<ffffffff810cfb8d>] ? trace_hardirqs_on+0xd/0x10
[ 57.037896] [<ffffffff811a8330>] ? kmem_cache_free+0xb0/0x2b0
[ 57.044789] [<ffffffff811b59c3>] ? free_object_rcu+0x93/0xa0
[ 57.051720] [<ffffffff81043d9f>] set_memory_rw+0x2f/0x40
[ 57.058727] [<ffffffff8104e17c>] bpf_jit_free+0x2c/0x40
[ 57.065577] [<ffffffff81642cba>] sk_filter_release_rcu+0x1a/0x30
[ 57.072338] [<ffffffff811108e2>] rcu_process_callbacks+0x202/0x7c0
[ 57.078962] [<ffffffff81057f17>] __do_softirq+0xf7/0x3f0
[ 57.085373] [<ffffffff81058245>] run_ksoftirqd+0x35/0x70
cannot reuse jited filter memory, since it's readonly,
so use original bpf insns memory to hold work_struct
defer kfree of sk_filter until jit completed freeing
tested on x86_64 and i386
Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
---
arch/arm/net/bpf_jit_32.c | 1 +
arch/powerpc/net/bpf_jit_comp.c | 1 +
arch/s390/net/bpf_jit_comp.c | 4 +++-
arch/sparc/net/bpf_jit_comp.c | 1 +
arch/x86/net/bpf_jit_comp.c | 18 +++++++++++++-----
include/linux/filter.h | 15 +++++++++++----
include/net/sock.h | 6 ++----
net/core/filter.c | 8 ++++----
8 files changed, 36 insertions(+), 18 deletions(-)
diff --git a/arch/arm/net/bpf_jit_32.c b/arch/arm/net/bpf_jit_32.c
index f50d223..99b44e0 100644
--- a/arch/arm/net/bpf_jit_32.c
+++ b/arch/arm/net/bpf_jit_32.c
@@ -930,4 +930,5 @@ void bpf_jit_free(struct sk_filter *fp)
{
if (fp->bpf_func != sk_run_filter)
module_free(NULL, fp->bpf_func);
+ kfree(fp);
}
diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c
index bf56e33..2345bdb 100644
--- a/arch/powerpc/net/bpf_jit_comp.c
+++ b/arch/powerpc/net/bpf_jit_comp.c
@@ -691,4 +691,5 @@ void bpf_jit_free(struct sk_filter *fp)
{
if (fp->bpf_func != sk_run_filter)
module_free(NULL, fp->bpf_func);
+ kfree(fp);
}
diff --git a/arch/s390/net/bpf_jit_comp.c b/arch/s390/net/bpf_jit_comp.c
index 7092392..a5df511 100644
--- a/arch/s390/net/bpf_jit_comp.c
+++ b/arch/s390/net/bpf_jit_comp.c
@@ -881,7 +881,9 @@ void bpf_jit_free(struct sk_filter *fp)
struct bpf_binary_header *header = (void *)addr;
if (fp->bpf_func == sk_run_filter)
- return;
+ goto free_filter;
set_memory_rw(addr, header->pages);
module_free(NULL, header);
+free_filter:
+ kfree(fp);
}
diff --git a/arch/sparc/net/bpf_jit_comp.c b/arch/sparc/net/bpf_jit_comp.c
index 9c7be59..218b6b2 100644
--- a/arch/sparc/net/bpf_jit_comp.c
+++ b/arch/sparc/net/bpf_jit_comp.c
@@ -808,4 +808,5 @@ void bpf_jit_free(struct sk_filter *fp)
{
if (fp->bpf_func != sk_run_filter)
module_free(NULL, fp->bpf_func);
+ kfree(fp);
}
diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 79c216a..516593e 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -772,13 +772,21 @@ out:
return;
}
+static void bpf_jit_free_deferred(struct work_struct *work)
+{
+ struct sk_filter *fp = container_of(work, struct sk_filter, work);
+ unsigned long addr = (unsigned long)fp->bpf_func & PAGE_MASK;
+ struct bpf_binary_header *header = (void *)addr;
+
+ set_memory_rw(addr, header->pages);
+ module_free(NULL, header);
+ kfree(fp);
+}
+
void bpf_jit_free(struct sk_filter *fp)
{
if (fp->bpf_func != sk_run_filter) {
- unsigned long addr = (unsigned long)fp->bpf_func & PAGE_MASK;
- struct bpf_binary_header *header = (void *)addr;
-
- set_memory_rw(addr, header->pages);
- module_free(NULL, header);
+ INIT_WORK(&fp->work, bpf_jit_free_deferred);
+ schedule_work(&fp->work);
}
}
diff --git a/include/linux/filter.h b/include/linux/filter.h
index a6ac848..ff4e40c 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -6,6 +6,7 @@
#include <linux/atomic.h>
#include <linux/compat.h>
+#include <linux/workqueue.h>
#include <uapi/linux/filter.h>
#ifdef CONFIG_COMPAT
@@ -25,15 +26,19 @@ struct sk_filter
{
atomic_t refcnt;
unsigned int len; /* Number of filter blocks */
+ struct rcu_head rcu;
unsigned int (*bpf_func)(const struct sk_buff *skb,
const struct sock_filter *filter);
- struct rcu_head rcu;
- struct sock_filter insns[0];
+ union {
+ struct sock_filter insns[0];
+ struct work_struct work;
+ };
};
-static inline unsigned int sk_filter_len(const struct sk_filter *fp)
+static inline unsigned int sk_filter_size(unsigned int proglen)
{
- return fp->len * sizeof(struct sock_filter) + sizeof(*fp);
+ return max(sizeof(struct sk_filter),
+ offsetof(struct sk_filter, insns[proglen]));
}
extern int sk_filter(struct sock *sk, struct sk_buff *skb);
@@ -67,11 +72,13 @@ static inline void bpf_jit_dump(unsigned int flen, unsigned int proglen,
}
#define SK_RUN_FILTER(FILTER, SKB) (*FILTER->bpf_func)(SKB, FILTER->insns)
#else
+#include <linux/slab.h>
static inline void bpf_jit_compile(struct sk_filter *fp)
{
}
static inline void bpf_jit_free(struct sk_filter *fp)
{
+ kfree(fp);
}
#define SK_RUN_FILTER(FILTER, SKB) sk_run_filter(SKB, FILTER->insns)
#endif
diff --git a/include/net/sock.h b/include/net/sock.h
index e3bf213..1105357 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -1612,16 +1612,14 @@ static inline void sk_filter_release(struct sk_filter *fp)
static inline void sk_filter_uncharge(struct sock *sk, struct sk_filter *fp)
{
- unsigned int size = sk_filter_len(fp);
-
- atomic_sub(size, &sk->sk_omem_alloc);
+ atomic_sub(sk_filter_size(fp->len), &sk->sk_omem_alloc);
sk_filter_release(fp);
}
static inline void sk_filter_charge(struct sock *sk, struct sk_filter *fp)
{
atomic_inc(&fp->refcnt);
- atomic_add(sk_filter_len(fp), &sk->sk_omem_alloc);
+ atomic_add(sk_filter_size(fp->len), &sk->sk_omem_alloc);
}
/*
diff --git a/net/core/filter.c b/net/core/filter.c
index 6438f29..01b7808 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -644,7 +644,6 @@ void sk_filter_release_rcu(struct rcu_head *rcu)
struct sk_filter *fp = container_of(rcu, struct sk_filter, rcu);
bpf_jit_free(fp);
- kfree(fp);
}
EXPORT_SYMBOL(sk_filter_release_rcu);
@@ -683,7 +682,7 @@ int sk_unattached_filter_create(struct sk_filter **pfp,
if (fprog->filter == NULL)
return -EINVAL;
- fp = kmalloc(fsize + sizeof(*fp), GFP_KERNEL);
+ fp = kmalloc(sk_filter_size(fprog->len), GFP_KERNEL);
if (!fp)
return -ENOMEM;
memcpy(fp->insns, fprog->filter, fsize);
@@ -723,6 +722,7 @@ int sk_attach_filter(struct sock_fprog *fprog, struct sock *sk)
{
struct sk_filter *fp, *old_fp;
unsigned int fsize = sizeof(struct sock_filter) * fprog->len;
+ unsigned int sk_fsize = sk_filter_size(fprog->len);
int err;
if (sock_flag(sk, SOCK_FILTER_LOCKED))
@@ -732,11 +732,11 @@ int sk_attach_filter(struct sock_fprog *fprog, struct sock *sk)
if (fprog->filter == NULL)
return -EINVAL;
- fp = sock_kmalloc(sk, fsize+sizeof(*fp), GFP_KERNEL);
+ fp = sock_kmalloc(sk, sk_fsize, GFP_KERNEL);
if (!fp)
return -ENOMEM;
if (copy_from_user(fp->insns, fprog->filter, fsize)) {
- sock_kfree_s(sk, fp, fsize+sizeof(*fp));
+ sock_kfree_s(sk, fp, sk_fsize);
return -EFAULT;
}
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCH v3 net-next] fix unsafe set_memory_rw from softirq
From: Heiko Carstens @ 2013-10-04 6:47 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Eric Dumazet, Paul Mackerras, H. Peter Anvin, sparclinux,
Nicolas Dichtel, linux-s390, Russell King, x86, James Morris,
Ingo Molnar, Alexey Kuznetsov, Paul E. McKenney, Xi Wang,
Matt Evans, Thomas Gleixner, linux-arm-kernel, Stelian Nirlu,
Nicolas Schichan, Hideaki YOSHIFUJI, netdev, linux-kernel,
David S. Miller, Mircea Gherzan, Daniel Borkmann,
Martin Schwidefsky, linux390, linuxppc-dev, Patrick McHardy
In-Reply-To: <1380853446-30537-1-git-send-email-ast@plumgrid.com>
On Thu, Oct 03, 2013 at 07:24:06PM -0700, Alexei Starovoitov wrote:
> diff --git a/arch/s390/net/bpf_jit_comp.c b/arch/s390/net/bpf_jit_comp.c
> index 7092392..a5df511 100644
> --- a/arch/s390/net/bpf_jit_comp.c
> +++ b/arch/s390/net/bpf_jit_comp.c
> @@ -881,7 +881,9 @@ void bpf_jit_free(struct sk_filter *fp)
> struct bpf_binary_header *header = (void *)addr;
>
> if (fp->bpf_func == sk_run_filter)
> - return;
> + goto free_filter;
> set_memory_rw(addr, header->pages);
> module_free(NULL, header);
> +free_filter:
> + kfree(fp);
> }
For the s390 part:
Acked-by: Heiko Carstens <heiko.carstens@de.ibm.com>
^ permalink raw reply
* Re: [PATCH v3 net-next] fix unsafe set_memory_rw from softirq
From: Alexei Starovoitov @ 2013-10-04 6:30 UTC (permalink / raw)
To: Eric Dumazet
Cc: linux-s390, netdev, Eric Dumazet, Daniel Borkmann, linuxppc-dev,
David S. Miller, linux-arm-kernel, Alexei Starovoitov
In-Reply-To: <1380863798.3564.12.camel@edumazet-glaptop.roam.corp.google.com>
On Thu, Oct 3, 2013 at 10:16 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Thu, 2013-10-03 at 21:11 -0700, Alexei Starovoitov wrote:
>
> -static inline unsigned int sk_filter_len(const struct sk_filter *fp)
> +static inline unsigned int sk_filter_size(const struct sk_filter *fp,
> + unsigned int proglen)
> {
> - return fp->len * sizeof(struct sock_filter) + sizeof(*fp);
> + return max(sizeof(*fp),
> + offsetof(struct sk_filter, insns[proglen]));
> }
indeed that's cleaner.
Like this then:
-static inline unsigned int sk_filter_len(const struct sk_filter *fp)
+static inline unsigned int sk_filter_size(unsigned int proglen)
{
- return fp->len * sizeof(struct sock_filter) + sizeof(*fp);
+ return max(sizeof(struct sk_filter),
+ offsetof(struct sk_filter, insns[proglen]));
}
testing it... will send v4 shortly
^ permalink raw reply
* RE: [PATCH 1/7] powerpc: Add interface to get msi region information
From: Bhushan Bharat-R65777 @ 2013-10-04 5:19 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: agraf@suse.de, Wood Scott-B07421, joro@8bytes.org,
linux-kernel@vger.kernel.org, iommu@lists.linux-foundation.org,
alex.williamson@redhat.com, linux-pci@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <20130924235812.GD9302@google.com>
> -----Original Message-----
> From: linux-pci-owner@vger.kernel.org [mailto:linux-pci-owner@vger.kernel=
.org]
> On Behalf Of Bjorn Helgaas
> Sent: Wednesday, September 25, 2013 5:28 AM
> To: Bhushan Bharat-R65777
> Cc: alex.williamson@redhat.com; joro@8bytes.org; benh@kernel.crashing.org=
;
> galak@kernel.crashing.org; linux-kernel@vger.kernel.org; linuxppc-
> dev@lists.ozlabs.org; linux-pci@vger.kernel.org; agraf@suse.de; Wood Scot=
t-
> B07421; iommu@lists.linux-foundation.org; Bhushan Bharat-R65777
> Subject: Re: [PATCH 1/7] powerpc: Add interface to get msi region informa=
tion
>=20
> On Thu, Sep 19, 2013 at 12:59:17PM +0530, Bharat Bhushan wrote:
> > This patch adds interface to get following information
> > - Number of MSI regions (which is number of MSI banks for powerpc).
> > - Get the region address range: Physical page which have the
> > address/addresses used for generating MSI interrupt
> > and size of the page.
> >
> > These are required to create IOMMU (Freescale PAMU) mapping for
> > devices which are directly assigned using VFIO.
> >
> > Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
> > ---
> > arch/powerpc/include/asm/machdep.h | 8 +++++++
> > arch/powerpc/include/asm/pci.h | 2 +
> > arch/powerpc/kernel/msi.c | 18 ++++++++++++++++
> > arch/powerpc/sysdev/fsl_msi.c | 39 ++++++++++++++++++++++++++++=
+++++--
> > arch/powerpc/sysdev/fsl_msi.h | 11 ++++++++-
> > drivers/pci/msi.c | 26 ++++++++++++++++++++++++
> > include/linux/msi.h | 8 +++++++
> > include/linux/pci.h | 13 ++++++++++++
> > 8 files changed, 120 insertions(+), 5 deletions(-)
> >
> > ...
>=20
> > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c index
> > aca7578..6d85c15 100644
> > --- a/drivers/pci/msi.c
> > +++ b/drivers/pci/msi.c
> > @@ -30,6 +30,20 @@ static int pci_msi_enable =3D 1;
> >
> > /* Arch hooks */
> >
> > +#ifndef arch_msi_get_region_count
> > +int arch_msi_get_region_count(void)
> > +{
> > + return 0;
> > +}
> > +#endif
> > +
> > +#ifndef arch_msi_get_region
> > +int arch_msi_get_region(int region_num, struct msi_region *region) {
> > + return 0;
> > +}
> > +#endif
>=20
> This #define strategy is gone; see 4287d824 ("PCI: use weak functions for=
MSI
> arch-specific functions"). Please use the weak function strategy for you=
r new
> MSI region functions.
ok
>=20
> > +
> > #ifndef arch_msi_check_device
> > int arch_msi_check_device(struct pci_dev *dev, int nvec, int type) {
> > @@ -903,6 +917,18 @@ void pci_disable_msi(struct pci_dev *dev) }
> > EXPORT_SYMBOL(pci_disable_msi);
> >
> > +int msi_get_region_count(void)
> > +{
> > + return arch_msi_get_region_count();
> > +}
> > +EXPORT_SYMBOL(msi_get_region_count);
> > +
> > +int msi_get_region(int region_num, struct msi_region *region) {
> > + return arch_msi_get_region(region_num, region); }
> > +EXPORT_SYMBOL(msi_get_region);
>=20
> Please split these interface additions, i.e., the drivers/pci/msi.c,
> include/linux/msi.h, and include/linux/pci.h changes, into a separate pat=
ch.
ok
>=20
> I don't know enough about VFIO to understand why these new interfaces are
> needed. Is this the first VFIO IOMMU driver? I see vfio_iommu_spapr_tce=
.c and
> vfio_iommu_type1.c but I don't know if they're comparable to the Freescal=
e PAMU.
> Do other VFIO IOMMU implementations support MSI? If so, do they handle t=
he
> problem of mapping the MSI regions in a different way?
PAMU is an aperture type of IOMMU while other are paging type, So they are =
completely different from what PAMU is and handle that differently.
>=20
> > /**
> > * pci_msix_table_size - return the number of device's MSI-X table ent=
ries
> > * @dev: pointer to the pci_dev data structure of MSI-X device
> > function diff --git a/include/linux/msi.h b/include/linux/msi.h index
> > ee66f3a..ae32601 100644
> > --- a/include/linux/msi.h
> > +++ b/include/linux/msi.h
> > @@ -50,6 +50,12 @@ struct msi_desc {
> > struct kobject kobj;
> > };
> >
> > +struct msi_region {
> > + int region_num;
> > + dma_addr_t addr;
> > + size_t size;
> > +};
>=20
> This needs some sort of explanatory comment.
Ok
-Bharat
>=20
> > /*
> > * The arch hook for setup up msi irqs
> > */
> > @@ -58,5 +64,7 @@ void arch_teardown_msi_irq(unsigned int irq); int
> > arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type); void
> > arch_teardown_msi_irqs(struct pci_dev *dev); int
> > arch_msi_check_device(struct pci_dev* dev, int nvec, int type);
> > +int arch_msi_get_region_count(void);
> > +int arch_msi_get_region(int region_num, struct msi_region *region);
> >
> > #endif /* LINUX_MSI_H */
> > diff --git a/include/linux/pci.h b/include/linux/pci.h index
> > 186540d..2b26a59 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -1126,6 +1126,7 @@ struct msix_entry {
> > u16 entry; /* driver uses to specify entry, OS writes */
> > };
> >
> > +struct msi_region;
> >
> > #ifndef CONFIG_PCI_MSI
> > static inline int pci_enable_msi_block(struct pci_dev *dev, unsigned
> > int nvec) @@ -1168,6 +1169,16 @@ static inline int
> > pci_msi_enabled(void) {
> > return 0;
> > }
> > +
> > +static inline int msi_get_region_count(void) {
> > + return 0;
> > +}
> > +
> > +static inline int msi_get_region(int region_num, struct msi_region
> > +*region) {
> > + return 0;
> > +}
> > #else
> > int pci_enable_msi_block(struct pci_dev *dev, unsigned int nvec);
> > int pci_enable_msi_block_auto(struct pci_dev *dev, unsigned int
> > *maxvec); @@ -1180,6 +1191,8 @@ void pci_disable_msix(struct pci_dev
> > *dev); void msi_remove_pci_irq_vectors(struct pci_dev *dev); void
> > pci_restore_msi_state(struct pci_dev *dev); int
> > pci_msi_enabled(void);
> > +int msi_get_region_count(void);
> > +int msi_get_region(int region_num, struct msi_region *region);
> > #endif
> >
> > #ifdef CONFIG_PCIEPORTBUS
> > --
> > 1.7.0.4
> >
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-pci"
> > in the body of a message to majordomo@vger.kernel.org More majordomo
> > info at http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in t=
he body
> of a message to majordomo@vger.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v3 net-next] fix unsafe set_memory_rw from softirq
From: Eric Dumazet @ 2013-10-04 5:16 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: linux-s390, netdev, Eric Dumazet, Daniel Borkmann, linuxppc-dev,
David S. Miller, linux-arm-kernel
In-Reply-To: <1380859875-31025-1-git-send-email-ast@plumgrid.com>
On Thu, 2013-10-03 at 21:11 -0700, Alexei Starovoitov wrote:
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index a6ac848..5d66cd9 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -25,15 +25,20 @@ struct sk_filter
> {
> atomic_t refcnt;
> unsigned int len; /* Number of filter blocks */
> + struct rcu_head rcu;
> unsigned int (*bpf_func)(const struct sk_buff *skb,
> const struct sock_filter *filter);
> - struct rcu_head rcu;
> + /* insns start right after bpf_func, so that sk_run_filter() fetches
> + * first insn from the same cache line that was used to call into
> + * sk_run_filter()
> + */
> struct sock_filter insns[0];
> };
>
> static inline unsigned int sk_filter_len(const struct sk_filter *fp)
> {
> - return fp->len * sizeof(struct sock_filter) + sizeof(*fp);
> + return max(fp->len * sizeof(struct sock_filter),
> + sizeof(struct work_struct)) + sizeof(*fp);
> }
I would use for include/linux/filter.h this (untested) diff :
(Note the include <linux/workqueue.h>)
I also remove your comment about cache lines, since there is nothing
to align stuff on a cache line boundary.
diff --git a/include/linux/filter.h b/include/linux/filter.h
index a6ac848..281b05c 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -6,6 +6,7 @@
#include <linux/atomic.h>
#include <linux/compat.h>
+#include <linux/workqueue.h>
#include <uapi/linux/filter.h>
#ifdef CONFIG_COMPAT
@@ -25,15 +26,20 @@ struct sk_filter
{
atomic_t refcnt;
unsigned int len; /* Number of filter blocks */
+ struct rcu_head rcu;
unsigned int (*bpf_func)(const struct sk_buff *skb,
const struct sock_filter *filter);
- struct rcu_head rcu;
- struct sock_filter insns[0];
+ union {
+ struct work_struct work;
+ struct sock_filter insns[0];
+ };
};
-static inline unsigned int sk_filter_len(const struct sk_filter *fp)
+static inline unsigned int sk_filter_size(const struct sk_filter *fp,
+ unsigned int proglen)
{
- return fp->len * sizeof(struct sock_filter) + sizeof(*fp);
+ return max(sizeof(*fp),
+ offsetof(struct sk_filter, insns[proglen]));
}
extern int sk_filter(struct sock *sk, struct sk_buff *skb);
This way, you can use sk_filter_size(fp, fprog->len)
instead of doing the max() games in sk_attach_filter() and
sk_unattached_filter_create()
Other than that, I think your patch is fine.
^ permalink raw reply related
* Re: [PATCH RFC 06/77] PCI/MSI: Factor out pci_get_msi_cap() interface
From: Alexander Gordeev @ 2013-10-04 5:13 UTC (permalink / raw)
To: Ben Hutchings
Cc: linux-mips, VMware, Inc., linux-nvme, linux-ide, linux-s390,
Andy King, linux-scsi, linux-rdma, x86, Ingo Molnar, linux-pci,
iss_storagedev, linux-driver, Tejun Heo, Bjorn Helgaas,
Dan Williams, Jon Mason, Solarflare linux maintainers, netdev,
linux-kernel, Ralf Baechle, e1000-devel, Martin Schwidefsky,
linux390, linuxppc-dev
In-Reply-To: <1380837174.3419.21.camel@bwh-desktop.uk.level5networks.com>
On Thu, Oct 03, 2013 at 10:52:54PM +0100, Ben Hutchings wrote:
> On Wed, 2013-10-02 at 12:48 +0200, Alexander Gordeev wrote:
> > #ifndef CONFIG_PCI_MSI
> > +static inline int pci_get_msi_cap(struct pci_dev *dev)
> > +{
> > + return -1;
> [...]
>
> Shouldn't this also return -EINVAL?
Yep, all inliners here are better to return -EINVAL.
Will do unless someone speaks out against.
> Ben.
--
Regards,
Alexander Gordeev
agordeev@redhat.com
^ permalink raw reply
* Re: [PATCH v3] powerpc/kernel/sysfs: cleanup set up macros for PMC/non-PMC SPRs
From: Olof Johansson @ 2013-10-04 4:19 UTC (permalink / raw)
To: Madhavan Srinivasan; +Cc: linuxppc-dev
In-Reply-To: <1380792456-19128-1-git-send-email-maddy@linux.vnet.ibm.com>
On Thu, Oct 3, 2013 at 2:27 AM, Madhavan Srinivasan
<maddy@linux.vnet.ibm.com> wrote:
> Currently PMC (Performance Monitor Counter) setup macros are used
> for other SPRs. Since not all SPRs are PMC related, this patch
> modifies the exisiting macro and uses it to setup both PMC and
> non PMC SPRs accordingly.
>
> V3 changes:
>
> 1) No logic change, just renamed generic macro and removed #define for empty string
> 2) Changes in the comment to explain better.
>
> V2 changes:
>
> 1) Modified SYSFS_PMCSETUP to a generic macro with additional parameter
> 2) Added PMC and SPR macro to call the generic macro
> 3) Changes in the comment to explain better.
>
> Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
Acked-by: Olof Johansson <olof@lixom.net>
For the pa6t parts
-Olof
^ permalink raw reply
* [PATCH v3 net-next] fix unsafe set_memory_rw from softirq
From: Alexei Starovoitov @ 2013-10-04 4:11 UTC (permalink / raw)
To: David S. Miller
Cc: linux-s390, netdev, Eric Dumazet, Daniel Borkmann, linuxppc-dev,
linux-arm-kernel
on x86 system with net.core.bpf_jit_enable = 1
sudo tcpdump -i eth1 'tcp port 22'
causes the warning:
[ 56.766097] Possible unsafe locking scenario:
[ 56.766097]
[ 56.780146] CPU0
[ 56.786807] ----
[ 56.793188] lock(&(&vb->lock)->rlock);
[ 56.799593] <Interrupt>
[ 56.805889] lock(&(&vb->lock)->rlock);
[ 56.812266]
[ 56.812266] *** DEADLOCK ***
[ 56.812266]
[ 56.830670] 1 lock held by ksoftirqd/1/13:
[ 56.836838] #0: (rcu_read_lock){.+.+..}, at: [<ffffffff8118f44c>] vm_unmap_aliases+0x8c/0x380
[ 56.849757]
[ 56.849757] stack backtrace:
[ 56.862194] CPU: 1 PID: 13 Comm: ksoftirqd/1 Not tainted 3.12.0-rc3+ #45
[ 56.868721] Hardware name: System manufacturer System Product Name/P8Z77 WS, BIOS 3007 07/26/2012
[ 56.882004] ffffffff821944c0 ffff88080bbdb8c8 ffffffff8175a145 0000000000000007
[ 56.895630] ffff88080bbd5f40 ffff88080bbdb928 ffffffff81755b14 0000000000000001
[ 56.909313] ffff880800000001 ffff880800000000 ffffffff8101178f 0000000000000001
[ 56.923006] Call Trace:
[ 56.929532] [<ffffffff8175a145>] dump_stack+0x55/0x76
[ 56.936067] [<ffffffff81755b14>] print_usage_bug+0x1f7/0x208
[ 56.942445] [<ffffffff8101178f>] ? save_stack_trace+0x2f/0x50
[ 56.948932] [<ffffffff810cc0a0>] ? check_usage_backwards+0x150/0x150
[ 56.955470] [<ffffffff810ccb52>] mark_lock+0x282/0x2c0
[ 56.961945] [<ffffffff810ccfed>] __lock_acquire+0x45d/0x1d50
[ 56.968474] [<ffffffff810cce6e>] ? __lock_acquire+0x2de/0x1d50
[ 56.975140] [<ffffffff81393bf5>] ? cpumask_next_and+0x55/0x90
[ 56.981942] [<ffffffff810cef72>] lock_acquire+0x92/0x1d0
[ 56.988745] [<ffffffff8118f52a>] ? vm_unmap_aliases+0x16a/0x380
[ 56.995619] [<ffffffff817628f1>] _raw_spin_lock+0x41/0x50
[ 57.002493] [<ffffffff8118f52a>] ? vm_unmap_aliases+0x16a/0x380
[ 57.009447] [<ffffffff8118f52a>] vm_unmap_aliases+0x16a/0x380
[ 57.016477] [<ffffffff8118f44c>] ? vm_unmap_aliases+0x8c/0x380
[ 57.023607] [<ffffffff810436b0>] change_page_attr_set_clr+0xc0/0x460
[ 57.030818] [<ffffffff810cfb8d>] ? trace_hardirqs_on+0xd/0x10
[ 57.037896] [<ffffffff811a8330>] ? kmem_cache_free+0xb0/0x2b0
[ 57.044789] [<ffffffff811b59c3>] ? free_object_rcu+0x93/0xa0
[ 57.051720] [<ffffffff81043d9f>] set_memory_rw+0x2f/0x40
[ 57.058727] [<ffffffff8104e17c>] bpf_jit_free+0x2c/0x40
[ 57.065577] [<ffffffff81642cba>] sk_filter_release_rcu+0x1a/0x30
[ 57.072338] [<ffffffff811108e2>] rcu_process_callbacks+0x202/0x7c0
[ 57.078962] [<ffffffff81057f17>] __do_softirq+0xf7/0x3f0
[ 57.085373] [<ffffffff81058245>] run_ksoftirqd+0x35/0x70
cannot reuse jited filter memory, since it's readonly,
so use original bpf insns memory to hold work_struct
defer kfree of sk_filter until jit completed freeing
tested on x86_64 and i386
Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
---
arch/arm/net/bpf_jit_32.c | 1 +
arch/powerpc/net/bpf_jit_comp.c | 1 +
arch/s390/net/bpf_jit_comp.c | 4 +++-
arch/sparc/net/bpf_jit_comp.c | 1 +
arch/x86/net/bpf_jit_comp.c | 20 +++++++++++++++-----
include/linux/filter.h | 11 +++++++++--
net/core/filter.c | 11 +++++++----
7 files changed, 37 insertions(+), 12 deletions(-)
diff --git a/arch/arm/net/bpf_jit_32.c b/arch/arm/net/bpf_jit_32.c
index f50d223..99b44e0 100644
--- a/arch/arm/net/bpf_jit_32.c
+++ b/arch/arm/net/bpf_jit_32.c
@@ -930,4 +930,5 @@ void bpf_jit_free(struct sk_filter *fp)
{
if (fp->bpf_func != sk_run_filter)
module_free(NULL, fp->bpf_func);
+ kfree(fp);
}
diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c
index bf56e33..2345bdb 100644
--- a/arch/powerpc/net/bpf_jit_comp.c
+++ b/arch/powerpc/net/bpf_jit_comp.c
@@ -691,4 +691,5 @@ void bpf_jit_free(struct sk_filter *fp)
{
if (fp->bpf_func != sk_run_filter)
module_free(NULL, fp->bpf_func);
+ kfree(fp);
}
diff --git a/arch/s390/net/bpf_jit_comp.c b/arch/s390/net/bpf_jit_comp.c
index 7092392..a5df511 100644
--- a/arch/s390/net/bpf_jit_comp.c
+++ b/arch/s390/net/bpf_jit_comp.c
@@ -881,7 +881,9 @@ void bpf_jit_free(struct sk_filter *fp)
struct bpf_binary_header *header = (void *)addr;
if (fp->bpf_func == sk_run_filter)
- return;
+ goto free_filter;
set_memory_rw(addr, header->pages);
module_free(NULL, header);
+free_filter:
+ kfree(fp);
}
diff --git a/arch/sparc/net/bpf_jit_comp.c b/arch/sparc/net/bpf_jit_comp.c
index 9c7be59..218b6b2 100644
--- a/arch/sparc/net/bpf_jit_comp.c
+++ b/arch/sparc/net/bpf_jit_comp.c
@@ -808,4 +808,5 @@ void bpf_jit_free(struct sk_filter *fp)
{
if (fp->bpf_func != sk_run_filter)
module_free(NULL, fp->bpf_func);
+ kfree(fp);
}
diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 79c216a..1396a0a 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -772,13 +772,23 @@ out:
return;
}
+static void bpf_jit_free_deferred(struct work_struct *work)
+{
+ struct sk_filter *fp = container_of((void *)work, struct sk_filter,
+ insns);
+ unsigned long addr = (unsigned long)fp->bpf_func & PAGE_MASK;
+ struct bpf_binary_header *header = (void *)addr;
+
+ set_memory_rw(addr, header->pages);
+ module_free(NULL, header);
+ kfree(fp);
+}
+
void bpf_jit_free(struct sk_filter *fp)
{
if (fp->bpf_func != sk_run_filter) {
- unsigned long addr = (unsigned long)fp->bpf_func & PAGE_MASK;
- struct bpf_binary_header *header = (void *)addr;
-
- set_memory_rw(addr, header->pages);
- module_free(NULL, header);
+ struct work_struct *work = (struct work_struct *)fp->insns;
+ INIT_WORK(work, bpf_jit_free_deferred);
+ schedule_work(work);
}
}
diff --git a/include/linux/filter.h b/include/linux/filter.h
index a6ac848..5d66cd9 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -25,15 +25,20 @@ struct sk_filter
{
atomic_t refcnt;
unsigned int len; /* Number of filter blocks */
+ struct rcu_head rcu;
unsigned int (*bpf_func)(const struct sk_buff *skb,
const struct sock_filter *filter);
- struct rcu_head rcu;
+ /* insns start right after bpf_func, so that sk_run_filter() fetches
+ * first insn from the same cache line that was used to call into
+ * sk_run_filter()
+ */
struct sock_filter insns[0];
};
static inline unsigned int sk_filter_len(const struct sk_filter *fp)
{
- return fp->len * sizeof(struct sock_filter) + sizeof(*fp);
+ return max(fp->len * sizeof(struct sock_filter),
+ sizeof(struct work_struct)) + sizeof(*fp);
}
extern int sk_filter(struct sock *sk, struct sk_buff *skb);
@@ -67,11 +72,13 @@ static inline void bpf_jit_dump(unsigned int flen, unsigned int proglen,
}
#define SK_RUN_FILTER(FILTER, SKB) (*FILTER->bpf_func)(SKB, FILTER->insns)
#else
+#include <linux/slab.h>
static inline void bpf_jit_compile(struct sk_filter *fp)
{
}
static inline void bpf_jit_free(struct sk_filter *fp)
{
+ kfree(fp);
}
#define SK_RUN_FILTER(FILTER, SKB) sk_run_filter(SKB, FILTER->insns)
#endif
diff --git a/net/core/filter.c b/net/core/filter.c
index 6438f29..ad5eaba 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -644,7 +644,6 @@ void sk_filter_release_rcu(struct rcu_head *rcu)
struct sk_filter *fp = container_of(rcu, struct sk_filter, rcu);
bpf_jit_free(fp);
- kfree(fp);
}
EXPORT_SYMBOL(sk_filter_release_rcu);
@@ -677,13 +676,15 @@ int sk_unattached_filter_create(struct sk_filter **pfp,
{
struct sk_filter *fp;
unsigned int fsize = sizeof(struct sock_filter) * fprog->len;
+ unsigned int sk_fsize = max_t(u32, fsize, sizeof(struct work_struct))
+ + sizeof(*fp);
int err;
/* Make sure new filter is there and in the right amounts. */
if (fprog->filter == NULL)
return -EINVAL;
- fp = kmalloc(fsize + sizeof(*fp), GFP_KERNEL);
+ fp = kmalloc(sk_fsize, GFP_KERNEL);
if (!fp)
return -ENOMEM;
memcpy(fp->insns, fprog->filter, fsize);
@@ -723,6 +724,8 @@ int sk_attach_filter(struct sock_fprog *fprog, struct sock *sk)
{
struct sk_filter *fp, *old_fp;
unsigned int fsize = sizeof(struct sock_filter) * fprog->len;
+ unsigned int sk_fsize = max_t(u32, fsize, sizeof(struct work_struct))
+ + sizeof(*fp);
int err;
if (sock_flag(sk, SOCK_FILTER_LOCKED))
@@ -732,11 +735,11 @@ int sk_attach_filter(struct sock_fprog *fprog, struct sock *sk)
if (fprog->filter == NULL)
return -EINVAL;
- fp = sock_kmalloc(sk, fsize+sizeof(*fp), GFP_KERNEL);
+ fp = sock_kmalloc(sk, sk_fsize, GFP_KERNEL);
if (!fp)
return -ENOMEM;
if (copy_from_user(fp->insns, fprog->filter, fsize)) {
- sock_kfree_s(sk, fp, fsize+sizeof(*fp));
+ sock_kfree_s(sk, fp, sk_fsize);
return -EFAULT;
}
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCH 1/2][v7] powerpc/mpc85xx:Add initial device tree support of T104x
From: Timur Tabi @ 2013-10-04 3:16 UTC (permalink / raw)
To: Prabhakar Kushwaha
Cc: Scott Wood, Varun Sethi, linuxppc-dev@lists.ozlabs.org,
Poonam Aggrwal, Priyanka Jain
In-Reply-To: <524A4664.7080008@freescale.com>
On Mon, Sep 30, 2013 at 10:49 PM, Prabhakar Kushwaha
<prabhakar@freescale.com> wrote:
>
> do we have any scripts which check for whitespace as checkpatch never give
> any warning/error.
> it is a very silly mistake which I am doing continuously :(
Use a proper code editor. I've never had this problem, so I'm
guessing your editor is not good enough.
I use Slickedit, but there are others that work just as well.
^ permalink raw reply
* [PATCH v3 net-next] fix unsafe set_memory_rw from softirq
From: Alexei Starovoitov @ 2013-10-04 2:24 UTC (permalink / raw)
To: David S. Miller
Cc: Heiko Carstens, Eric Dumazet, Paul Mackerras, H. Peter Anvin,
sparclinux, Nicolas Dichtel, Alexei Starovoitov, linux-s390,
Russell King, x86, James Morris, Ingo Molnar, Alexey Kuznetsov,
Paul E. McKenney, Xi Wang, Matt Evans, Thomas Gleixner,
linux-arm-kernel, Stelian Nirlu, Nicolas Schichan,
Hideaki YOSHIFUJI, netdev, linux-kernel, Mircea Gherzan,
Daniel Borkmann, Martin Schwidefsky, linux390, linuxppc-dev,
Patrick McHardy
on x86 system with net.core.bpf_jit_enable = 1
sudo tcpdump -i eth1 'tcp port 22'
causes the warning:
[ 56.766097] Possible unsafe locking scenario:
[ 56.766097]
[ 56.780146] CPU0
[ 56.786807] ----
[ 56.793188] lock(&(&vb->lock)->rlock);
[ 56.799593] <Interrupt>
[ 56.805889] lock(&(&vb->lock)->rlock);
[ 56.812266]
[ 56.812266] *** DEADLOCK ***
[ 56.812266]
[ 56.830670] 1 lock held by ksoftirqd/1/13:
[ 56.836838] #0: (rcu_read_lock){.+.+..}, at: [<ffffffff8118f44c>] vm_unmap_aliases+0x8c/0x380
[ 56.849757]
[ 56.849757] stack backtrace:
[ 56.862194] CPU: 1 PID: 13 Comm: ksoftirqd/1 Not tainted 3.12.0-rc3+ #45
[ 56.868721] Hardware name: System manufacturer System Product Name/P8Z77 WS, BIOS 3007 07/26/2012
[ 56.882004] ffffffff821944c0 ffff88080bbdb8c8 ffffffff8175a145 0000000000000007
[ 56.895630] ffff88080bbd5f40 ffff88080bbdb928 ffffffff81755b14 0000000000000001
[ 56.909313] ffff880800000001 ffff880800000000 ffffffff8101178f 0000000000000001
[ 56.923006] Call Trace:
[ 56.929532] [<ffffffff8175a145>] dump_stack+0x55/0x76
[ 56.936067] [<ffffffff81755b14>] print_usage_bug+0x1f7/0x208
[ 56.942445] [<ffffffff8101178f>] ? save_stack_trace+0x2f/0x50
[ 56.948932] [<ffffffff810cc0a0>] ? check_usage_backwards+0x150/0x150
[ 56.955470] [<ffffffff810ccb52>] mark_lock+0x282/0x2c0
[ 56.961945] [<ffffffff810ccfed>] __lock_acquire+0x45d/0x1d50
[ 56.968474] [<ffffffff810cce6e>] ? __lock_acquire+0x2de/0x1d50
[ 56.975140] [<ffffffff81393bf5>] ? cpumask_next_and+0x55/0x90
[ 56.981942] [<ffffffff810cef72>] lock_acquire+0x92/0x1d0
[ 56.988745] [<ffffffff8118f52a>] ? vm_unmap_aliases+0x16a/0x380
[ 56.995619] [<ffffffff817628f1>] _raw_spin_lock+0x41/0x50
[ 57.002493] [<ffffffff8118f52a>] ? vm_unmap_aliases+0x16a/0x380
[ 57.009447] [<ffffffff8118f52a>] vm_unmap_aliases+0x16a/0x380
[ 57.016477] [<ffffffff8118f44c>] ? vm_unmap_aliases+0x8c/0x380
[ 57.023607] [<ffffffff810436b0>] change_page_attr_set_clr+0xc0/0x460
[ 57.030818] [<ffffffff810cfb8d>] ? trace_hardirqs_on+0xd/0x10
[ 57.037896] [<ffffffff811a8330>] ? kmem_cache_free+0xb0/0x2b0
[ 57.044789] [<ffffffff811b59c3>] ? free_object_rcu+0x93/0xa0
[ 57.051720] [<ffffffff81043d9f>] set_memory_rw+0x2f/0x40
[ 57.058727] [<ffffffff8104e17c>] bpf_jit_free+0x2c/0x40
[ 57.065577] [<ffffffff81642cba>] sk_filter_release_rcu+0x1a/0x30
[ 57.072338] [<ffffffff811108e2>] rcu_process_callbacks+0x202/0x7c0
[ 57.078962] [<ffffffff81057f17>] __do_softirq+0xf7/0x3f0
[ 57.085373] [<ffffffff81058245>] run_ksoftirqd+0x35/0x70
cannot reuse jited filter memory, since it's readonly,
so use original bpf insns memory to hold work_struct
defer kfree of sk_filter until jit completed freeing
tested on x86_64 and i386
Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
---
arch/arm/net/bpf_jit_32.c | 1 +
arch/powerpc/net/bpf_jit_comp.c | 1 +
arch/s390/net/bpf_jit_comp.c | 4 +++-
arch/sparc/net/bpf_jit_comp.c | 1 +
arch/x86/net/bpf_jit_comp.c | 20 +++++++++++++++-----
include/linux/filter.h | 11 +++++++++--
net/core/filter.c | 11 +++++++----
7 files changed, 37 insertions(+), 12 deletions(-)
diff --git a/arch/arm/net/bpf_jit_32.c b/arch/arm/net/bpf_jit_32.c
index f50d223..99b44e0 100644
--- a/arch/arm/net/bpf_jit_32.c
+++ b/arch/arm/net/bpf_jit_32.c
@@ -930,4 +930,5 @@ void bpf_jit_free(struct sk_filter *fp)
{
if (fp->bpf_func != sk_run_filter)
module_free(NULL, fp->bpf_func);
+ kfree(fp);
}
diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c
index bf56e33..2345bdb 100644
--- a/arch/powerpc/net/bpf_jit_comp.c
+++ b/arch/powerpc/net/bpf_jit_comp.c
@@ -691,4 +691,5 @@ void bpf_jit_free(struct sk_filter *fp)
{
if (fp->bpf_func != sk_run_filter)
module_free(NULL, fp->bpf_func);
+ kfree(fp);
}
diff --git a/arch/s390/net/bpf_jit_comp.c b/arch/s390/net/bpf_jit_comp.c
index 7092392..a5df511 100644
--- a/arch/s390/net/bpf_jit_comp.c
+++ b/arch/s390/net/bpf_jit_comp.c
@@ -881,7 +881,9 @@ void bpf_jit_free(struct sk_filter *fp)
struct bpf_binary_header *header = (void *)addr;
if (fp->bpf_func == sk_run_filter)
- return;
+ goto free_filter;
set_memory_rw(addr, header->pages);
module_free(NULL, header);
+free_filter:
+ kfree(fp);
}
diff --git a/arch/sparc/net/bpf_jit_comp.c b/arch/sparc/net/bpf_jit_comp.c
index 9c7be59..218b6b2 100644
--- a/arch/sparc/net/bpf_jit_comp.c
+++ b/arch/sparc/net/bpf_jit_comp.c
@@ -808,4 +808,5 @@ void bpf_jit_free(struct sk_filter *fp)
{
if (fp->bpf_func != sk_run_filter)
module_free(NULL, fp->bpf_func);
+ kfree(fp);
}
diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 79c216a..1396a0a 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -772,13 +772,23 @@ out:
return;
}
+static void bpf_jit_free_deferred(struct work_struct *work)
+{
+ struct sk_filter *fp = container_of((void *)work, struct sk_filter,
+ insns);
+ unsigned long addr = (unsigned long)fp->bpf_func & PAGE_MASK;
+ struct bpf_binary_header *header = (void *)addr;
+
+ set_memory_rw(addr, header->pages);
+ module_free(NULL, header);
+ kfree(fp);
+}
+
void bpf_jit_free(struct sk_filter *fp)
{
if (fp->bpf_func != sk_run_filter) {
- unsigned long addr = (unsigned long)fp->bpf_func & PAGE_MASK;
- struct bpf_binary_header *header = (void *)addr;
-
- set_memory_rw(addr, header->pages);
- module_free(NULL, header);
+ struct work_struct *work = (struct work_struct *)fp->insns;
+ INIT_WORK(work, bpf_jit_free_deferred);
+ schedule_work(work);
}
}
diff --git a/include/linux/filter.h b/include/linux/filter.h
index a6ac848..5d66cd9 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -25,15 +25,20 @@ struct sk_filter
{
atomic_t refcnt;
unsigned int len; /* Number of filter blocks */
+ struct rcu_head rcu;
unsigned int (*bpf_func)(const struct sk_buff *skb,
const struct sock_filter *filter);
- struct rcu_head rcu;
+ /* insns start right after bpf_func, so that sk_run_filter() fetches
+ * first insn from the same cache line that was used to call into
+ * sk_run_filter()
+ */
struct sock_filter insns[0];
};
static inline unsigned int sk_filter_len(const struct sk_filter *fp)
{
- return fp->len * sizeof(struct sock_filter) + sizeof(*fp);
+ return max(fp->len * sizeof(struct sock_filter),
+ sizeof(struct work_struct)) + sizeof(*fp);
}
extern int sk_filter(struct sock *sk, struct sk_buff *skb);
@@ -67,11 +72,13 @@ static inline void bpf_jit_dump(unsigned int flen, unsigned int proglen,
}
#define SK_RUN_FILTER(FILTER, SKB) (*FILTER->bpf_func)(SKB, FILTER->insns)
#else
+#include <linux/slab.h>
static inline void bpf_jit_compile(struct sk_filter *fp)
{
}
static inline void bpf_jit_free(struct sk_filter *fp)
{
+ kfree(fp);
}
#define SK_RUN_FILTER(FILTER, SKB) sk_run_filter(SKB, FILTER->insns)
#endif
diff --git a/net/core/filter.c b/net/core/filter.c
index 6438f29..ad5eaba 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -644,7 +644,6 @@ void sk_filter_release_rcu(struct rcu_head *rcu)
struct sk_filter *fp = container_of(rcu, struct sk_filter, rcu);
bpf_jit_free(fp);
- kfree(fp);
}
EXPORT_SYMBOL(sk_filter_release_rcu);
@@ -677,13 +676,15 @@ int sk_unattached_filter_create(struct sk_filter **pfp,
{
struct sk_filter *fp;
unsigned int fsize = sizeof(struct sock_filter) * fprog->len;
+ unsigned int sk_fsize = max_t(u32, fsize, sizeof(struct work_struct))
+ + sizeof(*fp);
int err;
/* Make sure new filter is there and in the right amounts. */
if (fprog->filter == NULL)
return -EINVAL;
- fp = kmalloc(fsize + sizeof(*fp), GFP_KERNEL);
+ fp = kmalloc(sk_fsize, GFP_KERNEL);
if (!fp)
return -ENOMEM;
memcpy(fp->insns, fprog->filter, fsize);
@@ -723,6 +724,8 @@ int sk_attach_filter(struct sock_fprog *fprog, struct sock *sk)
{
struct sk_filter *fp, *old_fp;
unsigned int fsize = sizeof(struct sock_filter) * fprog->len;
+ unsigned int sk_fsize = max_t(u32, fsize, sizeof(struct work_struct))
+ + sizeof(*fp);
int err;
if (sock_flag(sk, SOCK_FILTER_LOCKED))
@@ -732,11 +735,11 @@ int sk_attach_filter(struct sock_fprog *fprog, struct sock *sk)
if (fprog->filter == NULL)
return -EINVAL;
- fp = sock_kmalloc(sk, fsize+sizeof(*fp), GFP_KERNEL);
+ fp = sock_kmalloc(sk, sk_fsize, GFP_KERNEL);
if (!fp)
return -ENOMEM;
if (copy_from_user(fp->insns, fprog->filter, fsize)) {
- sock_kfree_s(sk, fp, fsize+sizeof(*fp));
+ sock_kfree_s(sk, fp, sk_fsize);
return -EFAULT;
}
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCH RFC 01/77] PCI/MSI: Fix return value when populate_msi_sysfs() failed
From: Jon Mason @ 2013-10-04 0:59 UTC (permalink / raw)
To: Ben Hutchings
Cc: linux-mips, VMware, Inc., linux-nvme, linux-ide, stable,
linux-s390, Andy King, linux-scsi, linux-rdma, x86,
Alexander Gordeev, linux-pci, iss_storagedev, linux-driver,
Tejun Heo, Bjorn Helgaas, Dan Williams, Ingo Molnar,
Solarflare linux maintainers, netdev, linux-kernel, Ralf Baechle,
e1000-devel, Martin Schwidefsky, linux390, linuxppc-dev
In-Reply-To: <1380836781.3419.17.camel@bwh-desktop.uk.level5networks.com>
On Thu, Oct 03, 2013 at 10:46:21PM +0100, Ben Hutchings wrote:
> On Wed, 2013-10-02 at 17:39 -0700, Jon Mason wrote:
> > On Wed, Oct 02, 2013 at 12:48:17PM +0200, Alexander Gordeev wrote:
> > > Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> >
> > Since you are changing the behavior of the msix_capability_init
> > function on populate_msi_sysfs error, a comment describing why in this
> > commit would be nice.
> [...]
>
> This function was already treating that error as fatal, and freeing the
> MSIs. The change in behaviour is that it now returns the error code in
> this case, rather than 0. This is obviously correct and properly
> described by the one-line summary.
If someone dumb, like me, is looking at this commit and trying to
figure out what is happening, having ANY commit message is good. "Fix
the return value" doesn't tell me anything. Documenting what issue(s)
would've been seen had the error case been encountered and what will
now been seen would be very nice.
>
> Ben.
>
> --
> Ben Hutchings, Staff Engineer, Solarflare
> Not speaking for my employer; that's the marketing department's job.
> They asked us to note that Solarflare product names are trademarked.
>
^ permalink raw reply
* Re: [PATCH RFC 00/77] Re-design MSI/MSI-X interrupts enablement pattern
From: Ben Hutchings @ 2013-10-03 22:49 UTC (permalink / raw)
To: Alexander Gordeev
Cc: linux-mips, VMware, Inc., linux-nvme, linux-ide, linux-s390,
Andy King, linux-scsi, linux-rdma, x86, Ingo Molnar, linux-pci,
iss_storagedev, linux-driver, Tejun Heo, Bjorn Helgaas,
Dan Williams, Jon Mason, Solarflare linux maintainers, netdev,
linux-kernel, Ralf Baechle, e1000-devel, Martin Schwidefsky,
linux390, linuxppc-dev
In-Reply-To: <cover.1380703262.git.agordeev@redhat.com>
On Wed, 2013-10-02 at 12:48 +0200, Alexander Gordeev wrote:
> This series is against "next" branch in Bjorn's repo:
> git://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git
>
> Currently pci_enable_msi_block() and pci_enable_msix() interfaces
> return a error code in case of failure, 0 in case of success and a
> positive value which indicates the number of MSI-X/MSI interrupts
> that could have been allocated. The latter value should be passed
> to a repeated call to the interfaces until a failure or success:
>
>
> for (i = 0; i < FOO_DRIVER_MAXIMUM_NVEC; i++)
> adapter->msix_entries[i].entry = i;
>
> while (nvec >= FOO_DRIVER_MINIMUM_NVEC) {
> rc = pci_enable_msix(adapter->pdev,
> adapter->msix_entries, nvec);
> if (rc > 0)
> nvec = rc;
> else
> return rc;
> }
>
> return -ENOSPC;
>
>
> This technique proved to be confusing and error-prone. Vast share
> of device drivers simply fail to follow the described guidelines.
>
> This update converts pci_enable_msix() and pci_enable_msi_block()
> interfaces to canonical kernel functions and makes them return a
> error code in case of failure or 0 in case of success.
[...]
I think this is fundamentally flawed: pci_msix_table_size() and
pci_get_msi_cap() can only report the limits of the *device* (which the
driver usually already knows), whereas MSI allocation can also be
constrained due to *global* limits on the number of distinct IRQs.
Currently pci_enable_msix() will report a positive value if it fails due
to the global limit. Your patch 7 removes that. pci_enable_msi_block()
unfortunately doesn't appear to do this.
It seems to me that a more useful interface would take a minimum and
maximum number of vectors from the driver. This wouldn't allow the
driver to specify that it could only accept, say, any even number within
a certain range, but you could still leave the current functions
available for any driver that needs that.
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [PATCH RFC 06/77] PCI/MSI: Factor out pci_get_msi_cap() interface
From: Ben Hutchings @ 2013-10-03 21:52 UTC (permalink / raw)
To: Alexander Gordeev
Cc: linux-mips, VMware, Inc., linux-nvme, linux-ide, linux-s390,
Andy King, linux-scsi, linux-rdma, x86, Ingo Molnar, linux-pci,
iss_storagedev, linux-driver, Tejun Heo, Bjorn Helgaas,
Dan Williams, Jon Mason, Solarflare linux maintainers, netdev,
linux-kernel, Ralf Baechle, e1000-devel, Martin Schwidefsky,
linux390, linuxppc-dev
In-Reply-To: <9c282c4ab92731c719d161d2db6fc54ce33891d9.1380703262.git.agordeev@redhat.com>
On Wed, 2013-10-02 at 12:48 +0200, Alexander Gordeev wrote:
[...]
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -812,6 +812,21 @@ static int pci_msi_check_device(struct pci_dev *dev, int nvec, int type)
> return 0;
> }
>
> +int pci_get_msi_cap(struct pci_dev *dev)
> +{
> + int ret;
> + u16 msgctl;
> +
> + if (!dev->msi_cap)
> + return -EINVAL;
[...]
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1144,6 +1144,11 @@ struct msix_entry {
>
>
> #ifndef CONFIG_PCI_MSI
> +static inline int pci_get_msi_cap(struct pci_dev *dev)
> +{
> + return -1;
[...]
Shouldn't this also return -EINVAL?
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [PATCH RFC 01/77] PCI/MSI: Fix return value when populate_msi_sysfs() failed
From: Ben Hutchings @ 2013-10-03 21:46 UTC (permalink / raw)
To: Jon Mason
Cc: linux-mips, VMware, Inc., linux-nvme, linux-ide, stable,
linux-s390, Andy King, linux-scsi, linux-rdma, x86,
Alexander Gordeev, linux-pci, iss_storagedev, linux-driver,
Tejun Heo, Bjorn Helgaas, Dan Williams, Ingo Molnar,
Solarflare linux maintainers, netdev, linux-kernel, Ralf Baechle,
e1000-devel, Martin Schwidefsky, linux390, linuxppc-dev
In-Reply-To: <20131003003905.GK6768@jonmason-lab>
On Wed, 2013-10-02 at 17:39 -0700, Jon Mason wrote:
> On Wed, Oct 02, 2013 at 12:48:17PM +0200, Alexander Gordeev wrote:
> > Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
>
> Since you are changing the behavior of the msix_capability_init
> function on populate_msi_sysfs error, a comment describing why in this
> commit would be nice.
[...]
This function was already treating that error as fatal, and freeing the
MSIs. The change in behaviour is that it now returns the error code in
this case, rather than 0. This is obviously correct and properly
described by the one-line summary.
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [PATCH 5/9][v5] powerpc: implement is_instr_load_store().
From: Tom Musta @ 2013-10-03 19:52 UTC (permalink / raw)
To: Sukadev Bhattiprolu
Cc: Michael Ellerman, linux-kernel, Stephane Eranian, linuxppc-dev,
Paul Mackerras, Arnaldo Carvalho de Melo, Anshuman Khandual
In-Reply-To: <20131003190325.GB21561@us.ibm.com>
On 10/3/2013 2:03 PM, Sukadev Bhattiprolu wrote:
> Michael Ellerman [michael@ellerman.id.au] wrote:
<snip>
> |
> | if (lower == 6)
> | if (upper <= 1)
> | return true;
> | return false;
> v
Note that this case covers the lvsl/lvsr instructions, which, despite their
names are not actually loads. So you could eliminate this check and do
just a little bit better.
^ permalink raw reply
* Re: [PATCH RFC 50/77] mlx5: Update MSI/MSI-X interrupts enablement code
From: Alexander Gordeev @ 2013-10-03 19:48 UTC (permalink / raw)
To: Eli Cohen
Cc: linux-mips, VMware, Inc., linux-nvme, linux-ide, linux-s390,
Andy King, linux-scsi, linux-rdma, x86, Ingo Molnar, linux-pci,
iss_storagedev, linux-driver, Tejun Heo, Bjorn Helgaas,
Dan Williams, Jon Mason, Solarflare linux maintainers, netdev,
linux-kernel, Ralf Baechle, e1000-devel, Martin Schwidefsky,
linux390, linuxppc-dev
In-Reply-To: <20131003071433.GA7299@mtldesk30>
On Thu, Oct 03, 2013 at 10:14:33AM +0300, Eli Cohen wrote:
> On Wed, Oct 02, 2013 at 12:49:06PM +0200, Alexander Gordeev wrote:
> >
> > + err = pci_msix_table_size(dev->pdev);
> > + if (err < 0)
> > + return err;
> > +
> > nvec = dev->caps.num_ports * num_online_cpus() + MLX5_EQ_VEC_COMP_BASE;
> > nvec = min_t(int, nvec, num_eqs);
> > + nvec = min_t(int, nvec, err);
> > if (nvec <= MLX5_EQ_VEC_COMP_BASE)
> > return -ENOSPC;
>
> Making sure we don't request more vectors then the device's is capable
> of -- looks good.
> >
> > @@ -131,20 +136,15 @@ static int mlx5_enable_msix(struct mlx5_core_dev *dev)
> > for (i = 0; i < nvec; i++)
> > table->msix_arr[i].entry = i;
> >
> > -retry:
> > - table->num_comp_vectors = nvec - MLX5_EQ_VEC_COMP_BASE;
> > err = pci_enable_msix(dev->pdev, table->msix_arr, nvec);
> > - if (err <= 0) {
> > + if (err) {
> > + kfree(table->msix_arr);
> > return err;
> > - } else if (err > MLX5_EQ_VEC_COMP_BASE) {
> > - nvec = err;
> > - goto retry;
> > }
> >
>
> According to latest sources, pci_enable_msix() may still fail so why
> do you want to remove this code?
pci_enable_msix() may fail, but it can not return a positive number.
We first calculate how many MSI-Xs we need, adjust to what we can get,
check if that is enough and only then go for it.
> > - mlx5_core_dbg(dev, "received %d MSI vectors out of %d requested\n", err, nvec);
> > - kfree(table->msix_arr);
> > + table->num_comp_vectors = nvec - MLX5_EQ_VEC_COMP_BASE;
> >
> > - return -ENOSPC;
> > + return 0;
> > }
> >
> > static void mlx5_disable_msix(struct mlx5_core_dev *dev)
--
Regards,
Alexander Gordeev
agordeev@redhat.com
^ permalink raw reply
* Re: [PATCH 5/9][v5] powerpc: implement is_instr_load_store().
From: Sukadev Bhattiprolu @ 2013-10-03 19:03 UTC (permalink / raw)
To: Michael Ellerman
Cc: Michael Ellerman, linux-kernel, Stephane Eranian, linuxppc-dev,
Paul Mackerras, Arnaldo Carvalho de Melo, Anshuman Khandual
In-Reply-To: <20131003053519.GC17237@concordia>
Michael Ellerman [michael@ellerman.id.au] wrote:
| On Tue, Oct 01, 2013 at 05:15:06PM -0700, Sukadev Bhattiprolu wrote:
| > Implement is_instr_load_store() to detect whether a given instruction
| > is one of the fixed-point or floating-point load/store instructions.
| > This function will be used in a follow-on patch to save memory hierarchy
| > information of the load/store.
|
| The search over the array is a bit of a pity, especially as the worst
| case penalises you when you haven't hit a load/store.
Agree. Will try this out. This is certainly more efficient.
|
| I think we can do better. If you look at the opcode maps, and in
| particular the extended table for opcode 31, you'll see there's a
| reasonable amount of structure.
|
| The following is only valid for arch 2.06, ie. it will classify reserved
| opcodes as being load/store, but I think that's fine for the moment. If
| we need to use it somewhere in future we can update it. But we should
| add a big comment saying it's only valid in that case.
|
| Anyway, I think the following logic is all we need for opcode 31:
|
| bool is_load_store(int ext_opcode)
how about I call this is_load_store_2_06() and add a comment. Horrible
but minimizes chance of misuse.
| {
| upper = ext_opcode >> 5;
| lower = ext_opcode & 0x1f;
|
| /* Short circuit as many misses as we can */
| if (lower < 3 || lower > 23)
| return false;
|
| if (lower == 3)
| if (upper >= 16)
| return true;
|
| return false;
|
| if (lower == 6)
| if (upper <= 1)
| return true;
| return false;
|
| if (lower == 7 || lower == 12)
| return true;
|
| if (lower >= 20) /* && lower <= 23 (implicit) */
| return true;
|
| return false;
| }
|
|
| Which is not pretty, but I think it's preferable to the full search over the
| array.
|
| cheers
^ permalink raw reply
* Re: [PATCH RFC 59/77] qla2xxx: Update MSI/MSI-X interrupts enablement code
From: Saurav Kashyap @ 2013-10-03 17:42 UTC (permalink / raw)
To: Alexander Gordeev, linux-kernel
Cc: linux-mips@linux-mips.org, VMware, Inc.,
linux-nvme@lists.infradead.org, linux-ide@vger.kernel.org,
linux-s390@vger.kernel.org, Andy King, linux-scsi,
linux-rdma@vger.kernel.org, x86@kernel.org, Ingo Molnar,
linux-pci, iss_storagedev@hp.com, Dept-Eng Linux Driver,
Tejun Heo, Bjorn Helgaas, Dan Williams, Jon Mason,
Solarflare linux maintainers, netdev, Ralf Baechle,
e1000-devel@lists.sourceforge.net, Martin Schwidefsky,
linux390@de.ibm.com, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <54f6b89372f51cd27a6adf6ecc91b8bf6bb5ba74.1380703263.git.agordeev@redhat.com>
Acked-by: Saurav Kashyap <saurav.kashyap@qlogic.com>
>As result of recent re-design of the MSI/MSI-X interrupts enabling
>pattern this driver has to be updated to use the new technique to
>obtain a optimal number of MSI/MSI-X interrupts required.
>
>Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
>---
> drivers/scsi/qla2xxx/qla_isr.c | 18 +++++++++++-------
> 1 files changed, 11 insertions(+), 7 deletions(-)
>
>diff --git a/drivers/scsi/qla2xxx/qla_isr.c
>b/drivers/scsi/qla2xxx/qla_isr.c
>index df1b30b..6c11ab9 100644
>--- a/drivers/scsi/qla2xxx/qla_isr.c
>+++ b/drivers/scsi/qla2xxx/qla_isr.c
>@@ -2836,16 +2836,20 @@ qla24xx_enable_msix(struct qla_hw_data *ha,
>struct rsp_que *rsp)
> for (i =3D 0; i < ha->msix_count; i++)
> entries[i].entry =3D i;
>=20
>- ret =3D pci_enable_msix(ha->pdev, entries, ha->msix_count);
>- if (ret) {
>+ ret =3D pci_msix_table_size(ha->pdev);
>+ if (ret < 0) {
>+ goto msix_failed;
>+ } else {
> if (ret < MIN_MSIX_COUNT)
> goto msix_failed;
>=20
>- ql_log(ql_log_warn, vha, 0x00c6,
>- "MSI-X: Failed to enable support "
>- "-- %d/%d\n Retry with %d vectors.\n",
>- ha->msix_count, ret, ret);
>- ha->msix_count =3D ret;
>+ if (ret < ha->msix_count) {
>+ ql_log(ql_log_warn, vha, 0x00c6,
>+ "MSI-X: Failed to enable support "
>+ "-- %d/%d\n Retry with %d vectors.\n",
>+ ha->msix_count, ret, ret);
>+ ha->msix_count =3D ret;
>+ }
> ret =3D pci_enable_msix(ha->pdev, entries, ha->msix_count);
> if (ret) {
> msix_failed:
>--=20
>1.7.7.6
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at http://vger.kernel.org/majordomo-info.html
^ 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