Changeset View
Standalone View
sys/cam/cam_xpt.c
Show First 20 Lines • Show All 174 Lines • ▼ Show 20 Lines | struct cam_doneq { | ||||
struct mtx_padalign cam_doneq_mtx; | struct mtx_padalign cam_doneq_mtx; | ||||
STAILQ_HEAD(, ccb_hdr) cam_doneq; | STAILQ_HEAD(, ccb_hdr) cam_doneq; | ||||
int cam_doneq_sleep; | int cam_doneq_sleep; | ||||
}; | }; | ||||
static struct cam_doneq cam_doneqs[MAXCPU]; | static struct cam_doneq cam_doneqs[MAXCPU]; | ||||
static u_int __read_mostly cam_num_doneqs; | static u_int __read_mostly cam_num_doneqs; | ||||
static struct proc *cam_proc; | static struct proc *cam_proc; | ||||
static struct cam_doneq cam_async; | |||||
SYSCTL_INT(_kern_cam, OID_AUTO, num_doneqs, CTLFLAG_RDTUN, | SYSCTL_INT(_kern_cam, OID_AUTO, num_doneqs, CTLFLAG_RDTUN, | ||||
&cam_num_doneqs, 0, "Number of completion queues/threads"); | &cam_num_doneqs, 0, "Number of completion queues/threads"); | ||||
struct cam_periph *xpt_periph; | struct cam_periph *xpt_periph; | ||||
static periph_init_t xpt_periph_init; | static periph_init_t xpt_periph_init; | ||||
▲ Show 20 Lines • Show All 75 Lines • ▼ Show 20 Lines | |||||
static int xpt_schedule_dev(struct camq *queue, cam_pinfo *dev_pinfo, | static int xpt_schedule_dev(struct camq *queue, cam_pinfo *dev_pinfo, | ||||
u_int32_t new_priority); | u_int32_t new_priority); | ||||
static xpt_devicefunc_t xptpassannouncefunc; | static xpt_devicefunc_t xptpassannouncefunc; | ||||
static void xptaction(struct cam_sim *sim, union ccb *work_ccb); | static void xptaction(struct cam_sim *sim, union ccb *work_ccb); | ||||
static void xptpoll(struct cam_sim *sim); | static void xptpoll(struct cam_sim *sim); | ||||
static void camisr_runqueue(void); | static void camisr_runqueue(void); | ||||
static void xpt_done_process(struct ccb_hdr *ccb_h); | static void xpt_done_process(struct ccb_hdr *ccb_h); | ||||
static void xpt_done_td(void *); | static void xpt_done_td(void *); | ||||
static void xpt_async_td(void *); | |||||
static dev_match_ret xptbusmatch(struct dev_match_pattern *patterns, | static dev_match_ret xptbusmatch(struct dev_match_pattern *patterns, | ||||
u_int num_patterns, struct cam_eb *bus); | u_int num_patterns, struct cam_eb *bus); | ||||
static dev_match_ret xptdevicematch(struct dev_match_pattern *patterns, | static dev_match_ret xptdevicematch(struct dev_match_pattern *patterns, | ||||
u_int num_patterns, | u_int num_patterns, | ||||
struct cam_ed *device); | struct cam_ed *device); | ||||
static dev_match_ret xptperiphmatch(struct dev_match_pattern *patterns, | static dev_match_ret xptperiphmatch(struct dev_match_pattern *patterns, | ||||
u_int num_patterns, | u_int num_patterns, | ||||
struct cam_periph *periph); | struct cam_periph *periph); | ||||
▲ Show 20 Lines • Show All 685 Lines • ▼ Show 20 Lines | for (i = 0; i < cam_num_doneqs; i++) { | ||||
} | } | ||||
} | } | ||||
if (cam_num_doneqs < 1) { | if (cam_num_doneqs < 1) { | ||||
printf("xpt_init: Cannot init completion queues " | printf("xpt_init: Cannot init completion queues " | ||||
"- failing attach\n"); | "- failing attach\n"); | ||||
return (ENOMEM); | return (ENOMEM); | ||||
} | } | ||||
mtx_init(&cam_async.cam_doneq_mtx, "CAM async", NULL, MTX_DEF); | |||||
STAILQ_INIT(&cam_async.cam_doneq); | |||||
if (kproc_kthread_add(xpt_async_td, &cam_async, | |||||
&cam_proc, NULL, 0, 0, "cam", "async") != 0) { | |||||
printf("xpt_init: Cannot init async thread " | |||||
"- failing attach\n"); | |||||
return (ENOMEM); | |||||
} | |||||
/* | /* | ||||
* Register a callback for when interrupts are enabled. | * Register a callback for when interrupts are enabled. | ||||
*/ | */ | ||||
config_intrhook_oneshot(xpt_config, NULL); | config_intrhook_oneshot(xpt_config, NULL); | ||||
return (0); | return (0); | ||||
} | } | ||||
▲ Show 20 Lines • Show All 2,158 Lines • ▼ Show 20 Lines | case XPT_NOOP: | ||||
start_ccb->ccb_h.status = CAM_REQ_CMP; | start_ccb->ccb_h.status = CAM_REQ_CMP; | ||||
break; | break; | ||||
case XPT_REPROBE_LUN: | case XPT_REPROBE_LUN: | ||||
xpt_async(AC_INQ_CHANGED, path, NULL); | xpt_async(AC_INQ_CHANGED, path, NULL); | ||||
start_ccb->ccb_h.status = CAM_REQ_CMP; | start_ccb->ccb_h.status = CAM_REQ_CMP; | ||||
xpt_done(start_ccb); | xpt_done(start_ccb); | ||||
break; | break; | ||||
case XPT_ASYNC: | case XPT_ASYNC: | ||||
/* | |||||
* Queue the async operation so it can be run from a sleepable | |||||
* context. | |||||
*/ | |||||
start_ccb->ccb_h.status = CAM_REQ_CMP; | start_ccb->ccb_h.status = CAM_REQ_CMP; | ||||
xpt_done(start_ccb); | mtx_lock(&cam_async.cam_doneq_mtx); | ||||
STAILQ_INSERT_TAIL(&cam_async.cam_doneq, &start_ccb->ccb_h, sim_links.stqe); | |||||
start_ccb->ccb_h.pinfo.index = CAM_ASYNC_INDEX; | |||||
mtx_unlock(&cam_async.cam_doneq_mtx); | |||||
wakeup(&cam_async.cam_doneq); | |||||
chs: The wakeup() should be done before you unlock the lock, otherwise the worker thread can miss… | |||||
impAuthorUnsubmitted Done Inline ActionsI suspect that if we move that, we can get rid of the run interlock too... At best it saves extra wakeups in the rare case where races are lost if we move this... I'll do a separate commit for the done part. imp: I suspect that if we move that, we can get rid of the run interlock too... At best it saves… | |||||
jhbUnsubmitted Done Inline Actions
jhb: > I suspect that if we move that, we can get rid of the run interlock too... At best it saves… | |||||
jhbUnsubmitted Done Inline ActionsAs discussed in the separate review, the wakeup can be outside of the lock safely in this case. The only consequence of losing the "race" here is that the other thread might grab the lock as soon as this one releases it and handle the work request and go to sleep before wakeup() runs (assume a preemption just before wakeup() as the worst case to make the race longer). All that happens then is that you wakeup the thread when there is nothing to do so it wakes up and goes right back to sleep again. However, by moving the wakeup() out from under the lock, you reduce lock contention since if the wakeup() resumes the thread on another CPU, it will then immediately contest on this lock and possibly block (again, assume a worst case of a preemption just _after_ wakeup() to exacerbate this race). For this reason, it's actually a fairly common pattern to do wakeup's after dropping the lock when it is feasible. You could perhaps do the 'run' optimization from xpt_done() here as well where you only do the wakeup if the queue was empty before you inserted the new ccb into it. Note that this can in theory delay the wakeup() (e.g. if you get preempted before the wakeup in the original thread and meanwhile another thread enqueue's a second ccb and wouldn't do the wakeup, so both requests sit on the queue until the first thread resumes). The tradeoff there is potential latency vs the overhead of calling wakeup() which still has to go lock a sleepq chain spin lock to look for the right sleep queue to see if there is a thread to wake up or not. It's not clear to me what the right tradeoff there is, and it might be fine to just do the wakeup() unconditionally instead. jhb: As discussed in the separate review, the wakeup can be outside of the lock safely in this case. | |||||
impAuthorUnsubmitted Done Inline ActionsSince async events are relatively rare, I suspect the other optimizations for more contended paths won't matter (even though it will add a tiny amount to the sleepq lock contention). I may try to get xpt_done_td to just handle both and allow sleeping for the async queue (they will be separate threads, so the sleeping won't delay other transactions). That would allow us to keep the same optimizations for both cases. But there's already a couple of bugzilla bugs on this so I'd like to get it in as it is. imp: Since async events are relatively rare, I suspect the other optimizations for more contended… | |||||
break; | break; | ||||
default: | default: | ||||
case XPT_SDEV_TYPE: | case XPT_SDEV_TYPE: | ||||
case XPT_TERM_IO: | case XPT_TERM_IO: | ||||
case XPT_ENG_INQ: | case XPT_ENG_INQ: | ||||
/* XXX Implement */ | /* XXX Implement */ | ||||
xpt_print(start_ccb->ccb_h.path, | xpt_print(start_ccb->ccb_h.path, | ||||
"%s: CCB type %#x %s not supported\n", __func__, | "%s: CCB type %#x %s not supported\n", __func__, | ||||
▲ Show 20 Lines • Show All 2,306 Lines • ▼ Show 20 Lines | if ((ccb_h->flags & CAM_UNLOCKED) == 0) { | ||||
} | } | ||||
} | } | ||||
/* Call the peripheral driver's callback */ | /* Call the peripheral driver's callback */ | ||||
ccb_h->pinfo.index = CAM_UNQUEUED_INDEX; | ccb_h->pinfo.index = CAM_UNQUEUED_INDEX; | ||||
(*ccb_h->cbfcnp)(ccb_h->path->periph, (union ccb *)ccb_h); | (*ccb_h->cbfcnp)(ccb_h->path->periph, (union ccb *)ccb_h); | ||||
if (mtx != NULL) | if (mtx != NULL) | ||||
mtx_unlock(mtx); | mtx_unlock(mtx); | ||||
} | |||||
/* | |||||
* Parameterize instead and use xpt_done_td? | |||||
*/ | |||||
static void | |||||
xpt_async_td(void *arg) | |||||
{ | |||||
struct cam_doneq *queue = arg; | |||||
struct ccb_hdr *ccb_h; | |||||
STAILQ_HEAD(, ccb_hdr) doneq; | |||||
STAILQ_INIT(&doneq); | |||||
mtx_lock(&queue->cam_doneq_mtx); | |||||
while (1) { | |||||
while (STAILQ_EMPTY(&queue->cam_doneq)) { | |||||
queue->cam_doneq_sleep = 1; | |||||
msleep(&queue->cam_doneq, &queue->cam_doneq_mtx, | |||||
PRIBIO, "-", 0); | |||||
queue->cam_doneq_sleep = 0; | |||||
} | |||||
STAILQ_CONCAT(&doneq, &queue->cam_doneq); | |||||
mtx_unlock(&queue->cam_doneq_mtx); | |||||
while ((ccb_h = STAILQ_FIRST(&doneq)) != NULL) { | |||||
STAILQ_REMOVE_HEAD(&doneq, sim_links.stqe); | |||||
xpt_done_process(ccb_h); | |||||
} | |||||
mtx_lock(&queue->cam_doneq_mtx); | |||||
} | |||||
} | } | ||||
void | void | ||||
xpt_done_td(void *arg) | xpt_done_td(void *arg) | ||||
{ | { | ||||
struct cam_doneq *queue = arg; | struct cam_doneq *queue = arg; | ||||
struct ccb_hdr *ccb_h; | struct ccb_hdr *ccb_h; | ||||
STAILQ_HEAD(, ccb_hdr) doneq; | STAILQ_HEAD(, ccb_hdr) doneq; | ||||
▲ Show 20 Lines • Show All 111 Lines • Show Last 20 Lines |
The wakeup() should be done before you unlock the lock, otherwise the worker thread can miss the wakeup. (xpt_done() has the same problem.)