lkml.org 
[lkml]   [2022]   [Feb]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v3 02/10] block: Introduce queue limits for copy-offload support
On Thu, Feb 17, 2022 at 06:29:01PM +0530, Nitesh Shetty wrote:
> Thu, Feb 17, 2022 at 01:07:00AM -0800, Luis Chamberlain wrote:
> > The subject says limits for copy-offload...
> >
> > On Mon, Feb 14, 2022 at 01:29:52PM +0530, Nitesh Shetty wrote:
> > > Add device limits as sysfs entries,
> > > - copy_offload (RW)
> > > - copy_max_bytes (RW)
> > > - copy_max_hw_bytes (RO)
> > > - copy_max_range_bytes (RW)
> > > - copy_max_range_hw_bytes (RO)
> > > - copy_max_nr_ranges (RW)
> > > - copy_max_nr_ranges_hw (RO)
> >
> > Some of these seem like generic... and also I see a few more max_hw ones
> > not listed above...
> >
> queue_limits and sysfs entries are differently named.
> All sysfs entries start with copy_* prefix. Also it makes easy to lookup
> all copy sysfs.
> For queue limits naming, I tried to following existing queue limit
> convention (like discard).

My point was that your subject seems to indicate the changes are just
for copy-offload, but you seem to be adding generic queue limits as
well. Is that correct? If so then perhaps the subject should be changed
or the patch split up.

> > > +static ssize_t queue_copy_offload_store(struct request_queue *q,
> > > + const char *page, size_t count)
> > > +{
> > > + unsigned long copy_offload;
> > > + ssize_t ret = queue_var_store(&copy_offload, page, count);
> > > +
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + if (copy_offload && !q->limits.max_hw_copy_sectors)
> > > + return -EINVAL;
> >
> >
> > If the kernel schedules, copy_offload may still be true and
> > max_hw_copy_sectors may be set to 0. Is that an issue?
> >
>
> This check ensures that, we dont enable offload if device doesnt support
> offload. I feel it shouldn't be an issue.

My point was this:

CPU1 CPU2
Time
1) if (copy_offload
2) ---> preemption so it schedules
3) ---> some other high priority task Sets q->limits.max_hw_copy_sectors to 0
4) && !q->limits.max_hw_copy_sectors)

Can something bad happen if we allow for this?

\
 
 \ /
  Last update: 2022-02-23 01:57    [W:0.100 / U:0.472 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site