Re: [PATCH v6 05/12] block, scsi: Rename QUEUE_FLAG_PREEMPT_ONLY into DV_ONLY and introduce PM_ONLY

Bart Van Assche
2018-08-10T15:18:17Z

On Fri, 2018-08-10 at 09:39 +0800, jianchao.wang wrote:
> On 08/10/2018 03:41 AM, Bart Van Assche wrote:
> > +/*
> > + * Whether or not blk_queue_enter() should procee=
d. RQF_PM requests are always
> > + * allowed. RQF_DV requests are allowed if the PM_=
-ONLY queue flag has not been
> > + * set. Other requests are only allowed if neither PM_=
-ONLY nor DV_ONLY has been
> > + * set.
> > + */
> > +static inline bool blk_enter_allowed(struct request=
F8-queue *q,
> > + blk_mq_req_flags_t flags)
> > +{
> > + return flags & BLK_MQ_REQ_PM ||
> > + (!blk_queue_pm_only(q) &&
> > + (flags & BLK_MQ_REQ_DV || !blk=
F8-queue_dv_only(q)));
> > +}
>=20
> If a new state is indeed necessary, I think this kind of checking in =
hot path is inefficient.
> How about introduce a new state into request_queue, such as reque=
st_queue->gate_state.
> Set the PM_ONLY and DV_ONLY into this state, then we could ju=
st check request_queue->gate_state > 0
> before do further checking.

Hello Jianchao,

I agree with you that the hot path should be as efficient as possible. But =
I
don't think that the above code adds significantly more overhead to the hot
path. The requests for which we care about performance are the requests tha=
t
read and write data. BLK_MQ_REQ_PM is not set for any of these =
requests.
For the high performance case we care about the pm-only flag won't be set.
If that flag is not set the rest of the boolean expression does not have to
be evaluated (flags & BLK_MQ_REQ_DV || !blk_q=
ueue_dv_only(q)). So I think the
above change only adds one additional boolean test to the hot path. That
shouldn't have a measurable performance impact.

Bart.