Discussion:
[developer] [WEBREV 3/3] 5256 document some nonsensical code in the px driver
Josef 'Jeff' Sipek via illumos-developer
2014-10-23 15:05:05 UTC
Permalink
Please review:

Bug: https://www.illumos.org/issues/5256
Webrev: http://31bits.net/illumos/cr/5256-document-some-nonsensical-code-in-the-px-driver/
Patch: http://repo.or.cz/w/illumos-gate/jeffpc.git/patch/37ab3c02bb32d8c768b66d588f2e92598e221045


Thanks,

Jeff.
Josef 'Jeff' Sipek via illumos-developer
2014-10-23 15:05:05 UTC
Permalink
Please review:

Bug: https://www.illumos.org/issues/5255
Webrev: http://31bits.net/illumos/cr/5255-uts-shouldnt-open-code-ISP2/
Patch: http://repo.or.cz/w/illumos-gate/jeffpc.git/patch/8fe754f6536556815bae75923ec884c3cd4fb704


This change make wsdiff spew more than the normal things
(https://www.illumos.org/issues/2241) specifically drm and ip.

As far as ip is concerned, udp.c includes ddi.h rather early on then later
on it indirectly includes sysmacros.h. sysmacros.h and ddi.h both define a
couple of things including:

- makedevice
- getemajor

sysmacros.h just blindly defines these as macros.

ddi.h is paranoid and it #undefs these and then provides function
prototypes. Sadly, if sysmacros gets included after ddi, the sysmacros's
macros get used instead of the more proper ddi's definitions. Including
sysmacros before ddi causes ddi to undo the "damage" that sysmacros left
behind. This obviously causes harmless wsdiff differences.

drm is similar except it's because of getminor.


Thanks,

Jeff.
Marcel Telka via illumos-developer
2014-10-23 19:24:52 UTC
Permalink
Post by Josef 'Jeff' Sipek via illumos-developer
Bug: https://www.illumos.org/issues/5255
Webrev: http://31bits.net/illumos/cr/5255-uts-shouldnt-open-code-ISP2/
Patch: http://repo.or.cz/w/illumos-gate/jeffpc.git/patch/8fe754f6536556815bae75923ec884c3cd4fb704
Basically LGTM, with one minor nit:

usr/src/uts/common/io/drm/drmP.h

The bug synopsis is about ISP2 only, but you are cleaning offsetof() here. I
suggest either to adjust the bug synopsis, or create a separate bug for this.
--
+-------------------------------------------+
| Marcel Telka e-mail: ***@telka.sk |
| homepage: http://telka.sk/ |
| jabber: ***@jabber.sk |
+-------------------------------------------+


-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-86d49504
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-abdf7b7e
Powered by Listbox: http://www.listbox.com
Dan McDonald via illumos-developer
2014-10-23 19:36:50 UTC
Permalink
I'm generally fine with this (and can be a reviewer if you wish).

One question: Does the system ISP2() work like what you replaced? If so, the compiled code *should* be no different than what you started with. Did you verify that on one or more of the objects you compiled? If ISP2() (and its frequent usage with !) makes things different, you can just disregard checking disassemblies.

Dan



-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-86d49504
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-abdf7b7e
Powered by Listbox: http://www.listbox.com
Josef 'Jeff' Sipek via illumos-developer
2014-10-23 19:42:55 UTC
Permalink
Post by Dan McDonald via illumos-developer
I'm generally fine with this (and can be a reviewer if you wish).
One question: Does the system ISP2() work like what you replaced?
If so, the compiled code *should* be no different than what you started
with. Did you verify that on one or more of the objects you compiled? If
ISP2() (and its frequent usage with !) makes things different, you can
just disregard checking disassemblies.
Yep. I ran wsdiff on the proto area after a nightly and it was all good as
far as ISP2 conversions are concerned. (See my note about ip and drm
getting some differences related to other macros.)

Jeff.
--
Defenestration n. (formal or joc.):
The act of removing Windows from your computer in disgust, usually
followed by the installation of Linux or some other Unix-like operating
system.


-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-86d49504
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-abdf7b7e
Powered by Listbox: http://www.listbox.com
Josef 'Jeff' Sipek via illumos-developer
2014-10-23 19:52:40 UTC
Permalink
Post by Dan McDonald via illumos-developer
I'm generally fine with this (and can be a reviewer if you wish).
One question: Does the system ISP2() work like what you replaced?
I suppose I can dump some more info here... yes and no.

ISP2 returns 0 or 1. In some cases, the code that got replaced only yields
0 or non-zero - a subtle but possibly important difference. Virtually all
uses of ISP2 are as part of conditions in if or while statements. There
were a couple of places that did not want the boolean is-this-a-power-of-two
but instead wanted to know the non-zero value. I left those alone and
thefore they do not show up in the webrev.

Jeff.
--
Intellectuals solve problems; geniuses prevent them
- Albert Einstein


-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-86d49504
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-abdf7b7e
Powered by Listbox: http://www.listbox.com
Dan McDonald via illumos-developer
2014-10-23 19:54:55 UTC
Permalink
Post by Josef 'Jeff' Sipek via illumos-developer
Post by Dan McDonald via illumos-developer
I'm generally fine with this (and can be a reviewer if you wish).
One question: Does the system ISP2() work like what you replaced?
I suppose I can dump some more info here... yes and no.
ISP2 returns 0 or 1. In some cases, the code that got replaced only yields
0 or non-zero - a subtle but possibly important difference. Virtually all
uses of ISP2 are as part of conditions in if or while statements. There
were a couple of places that did not want the boolean is-this-a-power-of-two
but instead wanted to know the non-zero value. I left those alone and
thefore they do not show up in the webrev.
Count me as a reviewer then. I check to make sure you didn't accidentally invert the test (I've done that mistake occasionally, and had to fix someone else's once by hotpatching a running kernel).

Dan



-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-86d49504
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-abdf7b7e
Powered by Listbox: http://www.listbox.com
Josef 'Jeff' Sipek via illumos-developer
2014-10-23 19:47:05 UTC
Permalink
Post by Marcel Telka via illumos-developer
Post by Josef 'Jeff' Sipek via illumos-developer
Bug: https://www.illumos.org/issues/5255
Webrev: http://31bits.net/illumos/cr/5255-uts-shouldnt-open-code-ISP2/
Patch: http://repo.or.cz/w/illumos-gate/jeffpc.git/patch/8fe754f6536556815bae75923ec884c3cd4fb704
usr/src/uts/common/io/drm/drmP.h
The bug synopsis is about ISP2 only, but you are cleaning offsetof() here. I
suggest either to adjust the bug synopsis, or create a separate bug for this.
Right, it is just about ISP2. In drmP.h I had to add sysmacros include to
get ISP2. sysmacros.h however defines offsetoff as well, so I got
conflicting definitions. The easiest way to resolve this was to remove the
local definition of offsetof.

Jeff.
--
Reality is merely an illusion, albeit a very persistent one.
- Albert Einstein


-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-86d49504
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-abdf7b7e
Powered by Listbox: http://www.listbox.com
Marcel Telka via illumos-developer
2014-10-23 19:49:43 UTC
Permalink
Post by Josef 'Jeff' Sipek via illumos-developer
Post by Marcel Telka via illumos-developer
Post by Josef 'Jeff' Sipek via illumos-developer
Bug: https://www.illumos.org/issues/5255
Webrev: http://31bits.net/illumos/cr/5255-uts-shouldnt-open-code-ISP2/
Patch: http://repo.or.cz/w/illumos-gate/jeffpc.git/patch/8fe754f6536556815bae75923ec884c3cd4fb704
usr/src/uts/common/io/drm/drmP.h
The bug synopsis is about ISP2 only, but you are cleaning offsetof() here. I
suggest either to adjust the bug synopsis, or create a separate bug for this.
Right, it is just about ISP2. In drmP.h I had to add sysmacros include to
get ISP2. sysmacros.h however defines offsetoff as well, so I got
conflicting definitions. The easiest way to resolve this was to remove the
local definition of offsetof.
I see. Thanks for the explanation.

LGTM.
--
+-------------------------------------------+
| Marcel Telka e-mail: ***@telka.sk |
| homepage: http://telka.sk/ |
| jabber: ***@jabber.sk |
+-------------------------------------------+


-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-86d49504
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-abdf7b7e
Powered by Listbox: http://www.listbox.com
Jason King via illumos-developer
2014-10-23 19:30:44 UTC
Permalink
/usr/src/uts/common/dtrace/dtrace.c:13047 -- Shouldn't this be updated too?


On Thu, Oct 23, 2014 at 10:05 AM, Josef 'Jeff' Sipek via illumos-developer <
Post by Josef 'Jeff' Sipek via illumos-developer
Bug: https://www.illumos.org/issues/5255
Webrev: http://31bits.net/illumos/cr/5255-uts-shouldnt-open-code-ISP2/
http://repo.or.cz/w/illumos-gate/jeffpc.git/patch/8fe754f6536556815bae75923ec884c3cd4fb704
This change make wsdiff spew more than the normal things
(https://www.illumos.org/issues/2241) specifically drm and ip.
As far as ip is concerned, udp.c includes ddi.h rather early on then later
on it indirectly includes sysmacros.h. sysmacros.h and ddi.h both define a
- makedevice
- getemajor
sysmacros.h just blindly defines these as macros.
ddi.h is paranoid and it #undefs these and then provides function
prototypes. Sadly, if sysmacros gets included after ddi, the sysmacros's
macros get used instead of the more proper ddi's definitions. Including
sysmacros before ddi causes ddi to undo the "damage" that sysmacros left
behind. This obviously causes harmless wsdiff differences.
drm is similar except it's because of getminor.
Thanks,
Jeff.
-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-86d49504
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-abdf7b7e
Powered by Listbox: http://www.listbox.com
Josef 'Jeff' Sipek via illumos-developer
2014-10-23 19:44:45 UTC
Permalink
Post by Jason King via illumos-developer
/usr/src/uts/common/dtrace/dtrace.c:13047 -- Shouldn't this be updated too?
That's not a ISP2. Note the difference between dofs_offset and dofs_align.
There are other macros in sysmacros.h that'd handle this, but that's outside
of the scope for this commit. (I do hope to slowly go through all of them.)

Jeff.
Post by Jason King via illumos-developer
On Thu, Oct 23, 2014 at 10:05 AM, Josef 'Jeff' Sipek via illumos-developer <
Post by Josef 'Jeff' Sipek via illumos-developer
Bug: https://www.illumos.org/issues/5255
Webrev: http://31bits.net/illumos/cr/5255-uts-shouldnt-open-code-ISP2/
http://repo.or.cz/w/illumos-gate/jeffpc.git/patch/8fe754f6536556815bae75923ec884c3cd4fb704
This change make wsdiff spew more than the normal things
(https://www.illumos.org/issues/2241) specifically drm and ip.
As far as ip is concerned, udp.c includes ddi.h rather early on then later
on it indirectly includes sysmacros.h. sysmacros.h and ddi.h both define a
- makedevice
- getemajor
sysmacros.h just blindly defines these as macros.
ddi.h is paranoid and it #undefs these and then provides function
prototypes. Sadly, if sysmacros gets included after ddi, the sysmacros's
macros get used instead of the more proper ddi's definitions. Including
sysmacros before ddi causes ddi to undo the "damage" that sysmacros left
behind. This obviously causes harmless wsdiff differences.
drm is similar except it's because of getminor.
Thanks,
Jeff.
--
I have always wished for my computer to be as easy to use as my telephone;
my wish has come true because I can no longer figure out how to use my
telephone.
- Bjarne Stroustrup


-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-86d49504
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-abdf7b7e
Powered by Listbox: http://www.listbox.com
Josef 'Jeff' Sipek via illumos-developer
2014-10-23 15:04:49 UTC
Permalink
Please review:

Bug: https://www.illumos.org/issues/5253
Webrev: http://31bits.net/illumos/cr/5253-kmem_alloc-kmem_zalloc-wont-fail-with-KM_SLEEP/
Patch: http://repo.or.cz/w/illumos-gate/jeffpc.git/patch/d8eb8859bd8b557647b1524e8c7ab1d8081842f7


Thanks,

Jeff.
Marcel Telka via illumos-developer
2014-10-23 18:50:46 UTC
Permalink
Post by Josef 'Jeff' Sipek via illumos-developer
Bug: https://www.illumos.org/issues/5253
Webrev: http://31bits.net/illumos/cr/5253-kmem_alloc-kmem_zalloc-wont-fail-with-KM_SLEEP/
Patch: http://repo.or.cz/w/illumos-gate/jeffpc.git/patch/d8eb8859bd8b557647b1524e8c7ab1d8081842f7
Thanks,
Jeff.
From d8eb8859bd8b557647b1524e8c7ab1d8081842f7 Mon Sep 17 00:00:00 2001
Date: Thu, 23 Oct 2014 09:45:27 -0400
Subject: [PATCH] 5253 kmem_alloc/kmem_zalloc won't fail with KM_SLEEP 5254
getrbuf won't fail with KM_SLEEP
---
usr/src/uts/common/avs/ns/rdc/rdc_bitmap.c | 4 -
Are you sure krdc->bitmap_size is non-zero?
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/common/avs/ns/rdc/rdc_io.c | 6 -
I suggest to add ASSERT(rdc_max_sets != 0) right before the first kmem_zalloc()
and remove empty line at 336.
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/common/avs/ns/solaris/nsc_raw.c | 7 -
Are you sure maxdevs is non-zero?
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/common/avs/ns/sv/sv.c | 4 -
ok
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/common/crypto/io/dca.c | 12 +-
Are you sure the size is non-zero?
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/common/crypto/io/dca_rng.c | 15 +--
I suggest to add ASSERT(dca->dca_buf1 != NULL) and ASSERT(dca->dca_buf2 !=
NULL).
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/common/crypto/io/dprov.c | 2 -
ok
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/common/fs/nfs/nfs_sys.c | 2 -
ok
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/common/fs/ufs/ufs_acl.c | 3 +-
ok
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/common/io/aac/aac.c | 4 -
Are you sure the softs->total_slots is non-zero?
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/common/io/arn/arn_phy.c | 12 --
Are you sure you are allocating more than zero bytes here?
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/common/io/beep.c | 8 --
The beep_queue_size looks like a some sort of tunable. You should make sure
somehow it is non-zero.
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/common/io/blkdev/blkdev.c | 4 +-
As an act of defensive programming, I suggest to add ASSERT(bp != NULL) right
after the getrbuf() call.
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/common/io/comstar/lu/stmf_sbd/sbd.c | 4 +-
I suggest to add ASSERT(sl->sl_zfs_meta != NULL) right after the kmem_zalloc()
call.
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/common/io/comstar/port/pppt/pppt.c | 2 -
Are you sure iocd.pppt_buf_size is non-zero?
Post by Josef 'Jeff' Sipek via illumos-developer
.../uts/common/io/fibre-channel/fca/qlc/ql_init.c | 13 --
I suggest to add ASSERT(list_size != 0) right before the kmem_zalloc() call.
Post by Josef 'Jeff' Sipek via illumos-developer
.../uts/common/io/fibre-channel/fca/qlc/ql_ioctl.c | 46 +------
line 931: Are you sure ha->nvram_cache->size is always non-zero?

lines 1150 and 1289: I suggest to add ASSERT(vpd != NULL) after the
kmem_zalloc() call.

line 1557: Are you sure the ha->xioctl->fdesc.block_size is always non-zero?

line 2012 and 2191: Are you sure the dop->length is always non-zero?

line 2065: Are you sure the ha->risc_dump_size is always non-zero?
Post by Josef 'Jeff' Sipek via illumos-developer
.../uts/common/io/fibre-channel/fca/qlc/ql_mbx.c | 13 +-
ok
Post by Josef 'Jeff' Sipek via illumos-developer
.../uts/common/io/fibre-channel/fca/qlc/ql_nx.c | 4 -
Are you sure n is aways non-zero?
Post by Josef 'Jeff' Sipek via illumos-developer
.../common/io/fibre-channel/fca/qlc/ql_xioctl.c | 140 +-------------------
line 3338: Are you sure plbreq.TransferCount is always non-zero?

line 3630: I suggest to add ASSERT(tmp_buf != NULL) after the kmem_zalloc()
call.

line 6959: I suggest to add ASSERT(bp != NULL) after the kmem_zalloc() call.

lines 7634 and 7717: I suggest to add ASSERT(payload != NULL) after the
kmem_zalloc() call.

line 7833: Are you sure the pci_size is always non-zero?

line 8709: I suggest to add ASSERT(tmp_buf != NULL) after the kmem_zalloc()
call.

line 8951: I suggest to add ASSERT(size != 0) before the kmem_zalloc() call.

line 9028: I suggest to add ASSERT(fcf_list.BufSize != 0) before the
kmem_zalloc() call.
Post by Josef 'Jeff' Sipek via illumos-developer
.../common/io/fibre-channel/fca/qlge/qlge_flash.c | 6 -
Are you sure qlge->fdesc.block_size is always non-zero?
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/common/io/iwh/iwh.c | 5 -
ok
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/common/io/iwp/iwp.c | 5 -
ok
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/common/io/mwl/mwl.c | 11 --
line 571: Are you sure count is always non-zero?
Post by Josef 'Jeff' Sipek via illumos-developer
.../uts/common/io/net80211/net80211_crypto_ccmp.c | 2 -
ok
Post by Josef 'Jeff' Sipek via illumos-developer
.../uts/common/io/net80211/net80211_crypto_tkip.c | 2 -
ok
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/common/io/ntxn/unm_nic_init.c | 5 -
Are you sure the n is always non-zero?
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/common/io/pciex/hotplug/pcie_hp.c | 3 +-
Maybe ASSERT(packed != NULL) should be added, but not needed.
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/common/io/rtw/rtw.c | 6 -
Are you sure sr->sr_size is always non-zero?
Post by Josef 'Jeff' Sipek via illumos-developer
.../common/io/scsi/adapters/mpt_sas/mptsas_impl.c | 3 -
I suggest to add ASSERT(ioc_cmd != NULL) before the kmem_zalloc() call.
Post by Josef 'Jeff' Sipek via illumos-developer
.../uts/common/io/scsi/adapters/pmcs/pmcs_attach.c | 5 -
ok
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/common/io/scsi/targets/ses_safte.c | 8 --
line 112: I suggest to add ASSERT(sdata != NULL) right after the kmem_alloc()
call.

line 178: I suggest to add ASSERT(ssc->ses_private != NULL) right after the
kmem_zalloc() call.
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/common/io/usb/hcd/uhci/uhciutil.c | 10 +-
Are you sure info->num_pools is always non-zero?

Note: No need for extra parens in the kmem_zalloc() call.
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/common/io/usb/usba/whcdi.c | 3 -
ok
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/common/io/xge/drv/xgell.c | 21 ---
lines 2500, 2563, 2586, 2611, 2653, 2682, and 2720: I suggest to add ASSERT(buf
!= NULL) right after the kmem_alloc() call.
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/common/io/yge/yge.c | 4 -
I suggest to add ASSERT(dev->d_intrh != NULL) right after the kmem_zalloc()
call.
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/common/os/sunddi.c | 7 +-
line 5540: I suggest to add ASSERT(pathname != NULL) right after the
kmem_alloc() call.
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/common/rpc/sec_gss/svc_rpcsec_gss.c | 3 -
ok
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/i86pc/io/gfx_private/gfxp_devmap.c | 5 -
ok
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/i86pc/io/gfx_private/gfxp_pci.c | 10 +-
ok
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/intel/io/dktp/dcdev/dadk.c | 11 +-
line 743: I suggest to add ASSERT(secbuf != NULL) right after the kmem_zalloc()
call.

lines 745 and 1689: As an act of defensive programming, I suggest to add
ASSERT(bp != NULL) right after the getrbuf() call.
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/intel/io/drm/i915_gem.c | 4 -
Are you sure np is always non-zero?
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/intel/io/heci/heci_init.c | 8 --
line 785: Are you sure dev->num_heci_me_clients is always non-zero?

Note: Please add space before the asterisk at line 785.
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/intel/os/fmsmb.c | 2 -
ok
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/sun4/io/pcicfg.c | 6 -
Are you sure the *fcode_size is always non-zero?
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/sun4/io/px/px_mmu.c | 2 -
ok
--
+-------------------------------------------+
| Marcel Telka e-mail: ***@telka.sk |
| homepage: http://telka.sk/ |
| jabber: ***@jabber.sk |
+-------------------------------------------+


-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-86d49504
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-abdf7b7e
Powered by Listbox: http://www.listbox.com
Josef 'Jeff' Sipek via illumos-developer
2014-10-23 20:01:49 UTC
Permalink
Thanks for the review! It'll take me a bit to properly go through it all.

Jeff.
Post by Marcel Telka via illumos-developer
Post by Josef 'Jeff' Sipek via illumos-developer
Bug: https://www.illumos.org/issues/5253
Webrev: http://31bits.net/illumos/cr/5253-kmem_alloc-kmem_zalloc-wont-fail-with-KM_SLEEP/
Patch: http://repo.or.cz/w/illumos-gate/jeffpc.git/patch/d8eb8859bd8b557647b1524e8c7ab1d8081842f7
Thanks,
Jeff.
From d8eb8859bd8b557647b1524e8c7ab1d8081842f7 Mon Sep 17 00:00:00 2001
Date: Thu, 23 Oct 2014 09:45:27 -0400
Subject: [PATCH] 5253 kmem_alloc/kmem_zalloc won't fail with KM_SLEEP 5254
getrbuf won't fail with KM_SLEEP
---
usr/src/uts/common/avs/ns/rdc/rdc_bitmap.c | 4 -
Are you sure krdc->bitmap_size is non-zero?
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/common/avs/ns/rdc/rdc_io.c | 6 -
I suggest to add ASSERT(rdc_max_sets != 0) right before the first kmem_zalloc()
and remove empty line at 336.
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/common/avs/ns/solaris/nsc_raw.c | 7 -
Are you sure maxdevs is non-zero?
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/common/avs/ns/sv/sv.c | 4 -
ok
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/common/crypto/io/dca.c | 12 +-
Are you sure the size is non-zero?
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/common/crypto/io/dca_rng.c | 15 +--
I suggest to add ASSERT(dca->dca_buf1 != NULL) and ASSERT(dca->dca_buf2 !=
NULL).
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/common/crypto/io/dprov.c | 2 -
ok
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/common/fs/nfs/nfs_sys.c | 2 -
ok
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/common/fs/ufs/ufs_acl.c | 3 +-
ok
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/common/io/aac/aac.c | 4 -
Are you sure the softs->total_slots is non-zero?
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/common/io/arn/arn_phy.c | 12 --
Are you sure you are allocating more than zero bytes here?
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/common/io/beep.c | 8 --
The beep_queue_size looks like a some sort of tunable. You should make sure
somehow it is non-zero.
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/common/io/blkdev/blkdev.c | 4 +-
As an act of defensive programming, I suggest to add ASSERT(bp != NULL) right
after the getrbuf() call.
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/common/io/comstar/lu/stmf_sbd/sbd.c | 4 +-
I suggest to add ASSERT(sl->sl_zfs_meta != NULL) right after the kmem_zalloc()
call.
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/common/io/comstar/port/pppt/pppt.c | 2 -
Are you sure iocd.pppt_buf_size is non-zero?
Post by Josef 'Jeff' Sipek via illumos-developer
.../uts/common/io/fibre-channel/fca/qlc/ql_init.c | 13 --
I suggest to add ASSERT(list_size != 0) right before the kmem_zalloc() call.
Post by Josef 'Jeff' Sipek via illumos-developer
.../uts/common/io/fibre-channel/fca/qlc/ql_ioctl.c | 46 +------
line 931: Are you sure ha->nvram_cache->size is always non-zero?
lines 1150 and 1289: I suggest to add ASSERT(vpd != NULL) after the
kmem_zalloc() call.
line 1557: Are you sure the ha->xioctl->fdesc.block_size is always non-zero?
line 2012 and 2191: Are you sure the dop->length is always non-zero?
line 2065: Are you sure the ha->risc_dump_size is always non-zero?
Post by Josef 'Jeff' Sipek via illumos-developer
.../uts/common/io/fibre-channel/fca/qlc/ql_mbx.c | 13 +-
ok
Post by Josef 'Jeff' Sipek via illumos-developer
.../uts/common/io/fibre-channel/fca/qlc/ql_nx.c | 4 -
Are you sure n is aways non-zero?
Post by Josef 'Jeff' Sipek via illumos-developer
.../common/io/fibre-channel/fca/qlc/ql_xioctl.c | 140 +-------------------
line 3338: Are you sure plbreq.TransferCount is always non-zero?
line 3630: I suggest to add ASSERT(tmp_buf != NULL) after the kmem_zalloc()
call.
line 6959: I suggest to add ASSERT(bp != NULL) after the kmem_zalloc() call.
lines 7634 and 7717: I suggest to add ASSERT(payload != NULL) after the
kmem_zalloc() call.
line 7833: Are you sure the pci_size is always non-zero?
line 8709: I suggest to add ASSERT(tmp_buf != NULL) after the kmem_zalloc()
call.
line 8951: I suggest to add ASSERT(size != 0) before the kmem_zalloc() call.
line 9028: I suggest to add ASSERT(fcf_list.BufSize != 0) before the
kmem_zalloc() call.
Post by Josef 'Jeff' Sipek via illumos-developer
.../common/io/fibre-channel/fca/qlge/qlge_flash.c | 6 -
Are you sure qlge->fdesc.block_size is always non-zero?
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/common/io/iwh/iwh.c | 5 -
ok
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/common/io/iwp/iwp.c | 5 -
ok
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/common/io/mwl/mwl.c | 11 --
line 571: Are you sure count is always non-zero?
Post by Josef 'Jeff' Sipek via illumos-developer
.../uts/common/io/net80211/net80211_crypto_ccmp.c | 2 -
ok
Post by Josef 'Jeff' Sipek via illumos-developer
.../uts/common/io/net80211/net80211_crypto_tkip.c | 2 -
ok
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/common/io/ntxn/unm_nic_init.c | 5 -
Are you sure the n is always non-zero?
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/common/io/pciex/hotplug/pcie_hp.c | 3 +-
Maybe ASSERT(packed != NULL) should be added, but not needed.
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/common/io/rtw/rtw.c | 6 -
Are you sure sr->sr_size is always non-zero?
Post by Josef 'Jeff' Sipek via illumos-developer
.../common/io/scsi/adapters/mpt_sas/mptsas_impl.c | 3 -
I suggest to add ASSERT(ioc_cmd != NULL) before the kmem_zalloc() call.
Post by Josef 'Jeff' Sipek via illumos-developer
.../uts/common/io/scsi/adapters/pmcs/pmcs_attach.c | 5 -
ok
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/common/io/scsi/targets/ses_safte.c | 8 --
line 112: I suggest to add ASSERT(sdata != NULL) right after the kmem_alloc()
call.
line 178: I suggest to add ASSERT(ssc->ses_private != NULL) right after the
kmem_zalloc() call.
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/common/io/usb/hcd/uhci/uhciutil.c | 10 +-
Are you sure info->num_pools is always non-zero?
Note: No need for extra parens in the kmem_zalloc() call.
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/common/io/usb/usba/whcdi.c | 3 -
ok
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/common/io/xge/drv/xgell.c | 21 ---
lines 2500, 2563, 2586, 2611, 2653, 2682, and 2720: I suggest to add ASSERT(buf
!= NULL) right after the kmem_alloc() call.
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/common/io/yge/yge.c | 4 -
I suggest to add ASSERT(dev->d_intrh != NULL) right after the kmem_zalloc()
call.
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/common/os/sunddi.c | 7 +-
line 5540: I suggest to add ASSERT(pathname != NULL) right after the
kmem_alloc() call.
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/common/rpc/sec_gss/svc_rpcsec_gss.c | 3 -
ok
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/i86pc/io/gfx_private/gfxp_devmap.c | 5 -
ok
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/i86pc/io/gfx_private/gfxp_pci.c | 10 +-
ok
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/intel/io/dktp/dcdev/dadk.c | 11 +-
line 743: I suggest to add ASSERT(secbuf != NULL) right after the kmem_zalloc()
call.
lines 745 and 1689: As an act of defensive programming, I suggest to add
ASSERT(bp != NULL) right after the getrbuf() call.
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/intel/io/drm/i915_gem.c | 4 -
Are you sure np is always non-zero?
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/intel/io/heci/heci_init.c | 8 --
line 785: Are you sure dev->num_heci_me_clients is always non-zero?
Note: Please add space before the asterisk at line 785.
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/intel/os/fmsmb.c | 2 -
ok
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/sun4/io/pcicfg.c | 6 -
Are you sure the *fcode_size is always non-zero?
Post by Josef 'Jeff' Sipek via illumos-developer
usr/src/uts/sun4/io/px/px_mmu.c | 2 -
ok
--
+-------------------------------------------+
| homepage: http://telka.sk/ |
+-------------------------------------------+
--
I have always wished for my computer to be as easy to use as my telephone;
my wish has come true because I can no longer figure out how to use my
telephone.
- Bjarne Stroustrup


-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-86d49504
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-abdf7b7e
Powered by Listbox: http://www.listbox.com
Dan McDonald via illumos-developer
2014-10-23 19:02:05 UTC
Permalink
Post by Josef 'Jeff' Sipek via illumos-developer
Bug: https://www.illumos.org/issues/5253
Webrev: http://31bits.net/illumos/cr/5253-kmem_alloc-kmem_zalloc-wont-fail-with-KM_SLEEP/
Patch: http://repo.or.cz/w/illumos-gate/jeffpc.git/patch/d8eb8859bd8b557647b1524e8c7ab1d8081842f7
Silly question.

How many of these should keep checking for NULL, but instead allocate with:

KM_NOSLEEP|KM_NORMALPRI

The just-in-before-OSol-closed KM_NORMALPRI won't try as hard to reclaim as a stock KM_NOSLEEP is apt to do.

(This reminds me... the networking stack would benefit from allocb() that doesn't always do KM_NOSLEEP. I believe Bryan fixed DTrace to DTRT here not long after illumos happened, as a positive example.)

Dan



-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-86d49504
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-abdf7b7e
Powered by Listbox: http://www.listbox.com
Josef 'Jeff' Sipek via illumos-developer
2014-10-23 19:58:47 UTC
Permalink
Post by Dan McDonald via illumos-developer
Post by Josef 'Jeff' Sipek via illumos-developer
Bug: https://www.illumos.org/issues/5253
Webrev: http://31bits.net/illumos/cr/5253-kmem_alloc-kmem_zalloc-wont-fail-with-KM_SLEEP/
Patch: http://repo.or.cz/w/illumos-gate/jeffpc.git/patch/d8eb8859bd8b557647b1524e8c7ab1d8081842f7
Silly question.
KM_NOSLEEP|KM_NORMALPRI
The just-in-before-OSol-closed KM_NORMALPRI won't try as hard to reclaim
as a stock KM_NOSLEEP is apt to do.
Silly answer: I don't know.

It looks like there are a handful of users of KM_NORMALPRI in dtrace and a
single use in stmf sbd. This doesn't tell me enough to even have an
intuition about when it is appropriate to use it. Any suggestions?

Jeff.
--
All science is either physics or stamp collecting.
- Ernest Rutherford


-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-86d49504
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-abdf7b7e
Powered by Listbox: http://www.listbox.com
Dan McDonald via illumos-developer
2014-10-23 20:05:06 UTC
Permalink
Pinging Bryan explicitly, as he can speak to DTrace-specifics I may munge.
Post by Josef 'Jeff' Sipek via illumos-developer
Post by Dan McDonald via illumos-developer
KM_NOSLEEP|KM_NORMALPRI
The just-in-before-OSol-closed KM_NORMALPRI won't try as hard to reclaim
as a stock KM_NOSLEEP is apt to do.
Silly answer: I don't know.
It looks like there are a handful of users of KM_NORMALPRI in dtrace and a
single use in stmf sbd. This doesn't tell me enough to even have an
intuition about when it is appropriate to use it. Any suggestions?
It's something more than a few of us should discuss together.

KM_NOSLEEP will go through massive contortions trying to get you memory before it gives up. This can cause lots of CPU spin and other slowdowns. Apparently DTrace got to see such slowdowns manifest in production more than most, due to its use of memory. Like IP, DTrace is content to drop things in the face of no resources, but the stock KM_NOSLEEP does lots of gyrations. One tiny bit of VM2 that made it in early was the KM_NORMALPRI modifier. Basically, KM_NOSLEEP|KM_NORMALPRI means it behaves like the lighter-weight KM_SLEEP algorithm, but instead of sleeping when things fail, it returns NULL, which does so more quickly than the stock KM_NOSLEEP.

It has fascinating performance potential, but I suspect there are risks just changing things without careful measurement. THAT's why I never tried to do anything like that wholesale in the TCP/IP or even STREAMS (which is where allocb() comes from) stack. I didn't have the proof in production like DTrace had (though I've seen low-memory TCP/IP failures that might have been helped, nothing drastic enough to fix).

Dan



-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-86d49504
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-abdf7b7e
Powered by Listbox: http://www.listbox.com

Marcel Telka via illumos-developer
2014-10-23 18:56:18 UTC
Permalink
Post by Josef 'Jeff' Sipek via illumos-developer
Bug: https://www.illumos.org/issues/5256
Webrev: http://31bits.net/illumos/cr/5256-document-some-nonsensical-code-in-the-px-driver/
Patch: http://repo.or.cz/w/illumos-gate/jeffpc.git/patch/37ab3c02bb32d8c768b66d588f2e92598e221045
Shouldn't we instead of adding a comment just round up the px_dbg_msg_size
properly?
--
+-------------------------------------------+
| Marcel Telka e-mail: ***@telka.sk |
| homepage: http://telka.sk/ |
| jabber: ***@jabber.sk |
+-------------------------------------------+


-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-86d49504
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-abdf7b7e
Powered by Listbox: http://www.listbox.com
Josef 'Jeff' Sipek via illumos-developer
2014-10-23 19:36:42 UTC
Permalink
Post by Marcel Telka via illumos-developer
Post by Josef 'Jeff' Sipek via illumos-developer
Bug: https://www.illumos.org/issues/5256
Webrev: http://31bits.net/illumos/cr/5256-document-some-nonsensical-code-in-the-px-driver/
Patch: http://repo.or.cz/w/illumos-gate/jeffpc.git/patch/37ab3c02bb32d8c768b66d588f2e92598e221045
Shouldn't we instead of adding a comment just round up the px_dbg_msg_size
properly?
I don't actually know what it wants - that's why I opted to just comment.
(Also, it's easier to argue the correctness of a comment instead of trying
to enable some debug code on sparc hardware which I don't have and therefore
I'm not able to test.)

I suppose given some of the subsequent code, it wants size to be a power of
two so rounding up to the next power of two would be the way to go. I have
no problem getting a patch to RTI, but compiling for sparc in a PITA, and I
can't test it at all.

Jeff.
--
Once you have their hardware. Never give it back.
(The First Rule of Hardware Acquisition)


-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-86d49504
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-abdf7b7e
Powered by Listbox: http://www.listbox.com
Marcel Telka via illumos-developer
2014-10-23 19:46:02 UTC
Permalink
Post by Josef 'Jeff' Sipek via illumos-developer
Post by Marcel Telka via illumos-developer
Post by Josef 'Jeff' Sipek via illumos-developer
Bug: https://www.illumos.org/issues/5256
Webrev: http://31bits.net/illumos/cr/5256-document-some-nonsensical-code-in-the-px-driver/
Patch: http://repo.or.cz/w/illumos-gate/jeffpc.git/patch/37ab3c02bb32d8c768b66d588f2e92598e221045
Shouldn't we instead of adding a comment just round up the px_dbg_msg_size
properly?
I don't actually know what it wants - that's why I opted to just comment.
(Also, it's easier to argue the correctness of a comment instead of trying
to enable some debug code on sparc hardware which I don't have and therefore
I'm not able to test.)
I suppose given some of the subsequent code, it wants size to be a power of
two so rounding up to the next power of two would be the way to go. I have
no problem getting a patch to RTI, but compiling for sparc in a PITA, and I
can't test it at all.
Yes, I believe it should be rounded up to the next power of two. I didn't
noticed earlier this is for sparc only :-(. Maybe some guys with sparc boxes
should go and fix it properly, but since this is very low priority (SPARC only
+ DEBUG only + unlikely case), then I'm okay with your comment added.

So, LGTM.
--
+-------------------------------------------+
| Marcel Telka e-mail: ***@telka.sk |
| homepage: http://telka.sk/ |
| jabber: ***@jabber.sk |
+-------------------------------------------+


-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-86d49504
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-abdf7b7e
Powered by Listbox: http://www.listbox.com
Loading...