On Tue, Feb 22, 2022 at 04:55:41PM -0800, Luis Chamberlain wrote: > 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. > Yeah, queue limits indicates copy offload. I think will make more readable by adding copy_offload_* prefix. > > > > +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(©_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? > > max_hw_copy_sectors is read only for user. And inside kernel, this is set only by driver at initialization.
-- dm-devel mailing list dm-devel@xxxxxxxxxx https://listman.redhat.com/mailman/listinfo/dm-devel