Re: [PATCH 5/8] sg: add free list, rework locking
Hi linux-scsi-owner,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on mkp-scsi/for-next]
[also build test ERROR on v4.19-rc8 next-20181019]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system]
url:
https://github.com/0day-ci/linux/commits/linux-scsi-owner-vger-kernel-org/sg-major-cleanup-remove-max_queue-limit/20181019-183809
base: https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
config: i386-randconfig-i1-201841 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
drivers/scsi/sg.o: In function `sg_rq_end_io_usercontext':
>> drivers/scsi/sg.c:1494: undefined reference to `sg_rq_state_str'
>> drivers/scsi/sg.c:1494: undefined reference to `sg_rq_state_str'
vim +1494 drivers/scsi/sg.c
1470
1471 /*
1472 * This user context function is needed to clean up a request that has
been
1473 * interrupted (e.g. by control-C at keyboard). That leads to a request
1474 * being an 'orphan' and will be cleared here unless the 'keep_orphan'
flag
1475 * has been set on the owning file descriptor. In that case the user is
1476 * expected to call read() or ioctl(SG_IORECEIVE) to receive the
response
1477 * and free resources held by the interrupted request.
1478 */
1479 static void
1480 sg_rq_end_io_usercontext(struct work_struct *work)
1481 {
1482 struct sg_request *srp = container_of(work, struct sg_request,
ew.work);
1483 struct sg_fd *sfp;
1484
1485 if (!srp) {
1486 WARN_ONCE("s: srp unexpectedly NULL\n", __func__);
1487 return;
1488 }
1489 sfp = srp->parentfp;
1490 if (!sfp) {
1491 WARN_ONCE(1, "%s: sfp unexpectedly NULL\n", __func__);
1492 return;
1493 }
> 1494 SG_LOG(3, sfp->parentdp, "%s: clean srp=0x%p, rq_state: %s\n",
1495 __func__, srp, sg_rq_state_str(srp->rq_state, true));
1496 sg_finish_scsi_blk_rq(srp);
1497 sg_remove_request(sfp, srp);
1498 kref_put(>f_ref, sg_remove_sfp);
1499 }
1500
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
.config.gz
Description: application/gzip
Re: [PATCH 5/8] sg: add free list, rework locking
On Fri, 2018-10-19 at 15:55 -0400, Douglas Gilbert wrote:
> On 2018-10-19 11:22 a.m., Bart Van Assche wrote:
> > On Fri, 2018-10-19 at 02:24 -0400, Douglas Gilbert wrote:
> > > static void
> > > -sg_fill_request_table(struct sg_fd *sfp, struct sg_req_info *rinfo)
> > > +sg_fill_request_table(struct sg_fd *sfp, struct sg_req_info *rinfo,
> > > + int max_num)
> > > {
> > > struct sg_request *srp;
> > > int val;
> > > - unsigned int ms;
> > >
> > > val = 0;
> > > - list_for_each_entry(srp, >rq_list, entry) {
> > > - if (val >= SG_MAX_QUEUE)
> > > - break;
> > > - rinfo[val].req_state = srp->done + 1;
> > > + list_for_each_entry(srp, >rq_list, rq_entry) {
> > > + if (val >= max_num)
> > > + return;
> >
> > What protects the sfp->rq_list against concurrent changes? It seems to me
> > like all other code that iterates over or modifies that list protects that
> > list with rq_list_lock?
>
> Bart,
> The function is called from sg_ioctl() case SG_GET_REQUEST_TABLE and at the
> call point the read_lock is held on rq_list_lock. Maybe I can add a comment
> above the function about the lock being held. [At least it is as the end
> of the patch series, and that is all I care about :-)]
Hi Doug,
Thanks for the clarification. How about adding a __must_hold() annotation?
Bart.
Re: [PATCH 5/8] sg: add free list, rework locking
On 2018-10-19 11:22 a.m., Bart Van Assche wrote:
On Fri, 2018-10-19 at 02:24 -0400, Douglas Gilbert wrote:
static void
-sg_fill_request_table(struct sg_fd *sfp, struct sg_req_info *rinfo)
+sg_fill_request_table(struct sg_fd *sfp, struct sg_req_info *rinfo,
+ int max_num)
{
struct sg_request *srp;
int val;
- unsigned int ms;
val = 0;
- list_for_each_entry(srp, >rq_list, entry) {
- if (val >= SG_MAX_QUEUE)
- break;
- rinfo[val].req_state = srp->done + 1;
+ list_for_each_entry(srp, >rq_list, rq_entry) {
+ if (val >= max_num)
+ return;
What protects the sfp->rq_list against concurrent changes? It seems to me
like all other code that iterates over or modifies that list protects that
list with rq_list_lock?
Bart,
The function is called from sg_ioctl() case SG_GET_REQUEST_TABLE and at the
call point the read_lock is held on rq_list_lock. Maybe I can add a comment
above the function about the lock being held. [At least it is as the end
of the patch series, and that is all I care about :-)]
Doug Gilbert
P.S. Best to look at sg.c after each patch is applied, not the !@#$ing
stupid patches.
Re: [PATCH 5/8] sg: add free list, rework locking
On Fri, 2018-10-19 at 10:38 -0400, Tony Battersby wrote: > Incidentally, I have been using my own home-grown target-mode SCSI > system for the past 16 years, but now I am starting to look into > switching to SCST. I was just reading about their "SGV cache": > > http://scst.sourceforge.net/scst_pg.html > > It looks like it serves a similar purpose to what you are trying to > accomplish with recycling the indirect I/O buffers between different > requests. Perhaps you can borrow some inspiration from them (or even > some code). Although the current SCST SGV cache implementation is more complicated than necessary, I agree that it would be useful to have such a cache implementation in the Linux kernel. The NVMe target driver and the SCSI target core would also benefit from caching SGV allocations. Bart.
Re: [PATCH 5/8] sg: add free list, rework locking
On Fri, 2018-10-19 at 02:24 -0400, Douglas Gilbert wrote:
> static void
> -sg_fill_request_table(struct sg_fd *sfp, struct sg_req_info *rinfo)
> +sg_fill_request_table(struct sg_fd *sfp, struct sg_req_info *rinfo,
> + int max_num)
> {
> struct sg_request *srp;
> int val;
> - unsigned int ms;
>
> val = 0;
> - list_for_each_entry(srp, >rq_list, entry) {
> - if (val >= SG_MAX_QUEUE)
> - break;
> - rinfo[val].req_state = srp->done + 1;
> + list_for_each_entry(srp, >rq_list, rq_entry) {
> + if (val >= max_num)
> + return;
What protects the sfp->rq_list against concurrent changes? It seems to me
like all other code that iterates over or modifies that list protects that
list with rq_list_lock?
Thanks,
Bart.
Re: [PATCH 5/8] sg: add free list, rework locking
On 10/19/18 2:24 AM, Douglas Gilbert wrote:
> + if (sfp->tot_fd_thresh)
> + sfp->sum_fd_dlens += align_sz;
>
What lock protects sfp->sum_fd_dlens?
> /*
> @@ -2216,38 +2401,85 @@ sg_add_request(struct sg_fd *sfp)
> * data length exceeds rem_sgat_thresh then the data (or sgat) is
> * cleared and the request is appended to the tail of the free list.
> */
> -static int
> +static void
> sg_remove_request(struct sg_fd *sfp, struct sg_request *srp)
> {
> + bool reserve;
> unsigned long iflags;
> - int res = 0;
> + const char *cp = "head";
> + char b[64];
>
> - if (!sfp || !srp || list_empty(>rq_list))
> - return res;
> + if (WARN_ON(!sfp || !srp))
> + return;
> write_lock_irqsave(>rq_list_lock, iflags);
> - if (!list_empty(>entry)) {
> - list_del(>entry);
> - srp->parentfp = NULL;
> - res = 1;
> - }
> + spin_lock(>rq_entry_lck);
> + /*
> + * N.B. sg_request object not de-allocated (freed). The contents of
> + * rq_list and rq_free_list lists are de-allocated (freed) when the
> + * owning file descriptor is closed. The free list acts as a LIFO.
> + * This can improve the chance of a cache hit when request is re-used.
> + */
> + reserve = (sfp->reserve_srp == srp);
> + if (reserve || srp->data.dlen <= sfp->rem_sgat_thresh) {
> + list_del(>rq_entry);
> + if (srp->data.dlen > 0)
> + list_add(>free_entry, >rq_free_list);
> + else {
> + list_add_tail(>free_entry, >rq_free_list);
> + cp = "tail";
> + }
> + snprintf(b, sizeof(b), "%ssrp=0x%p move to fl %s",
> + (reserve ? "reserve " : ""), srp, cp);
> + } else {
> + srp->rq_state = SG_RQ_BUSY;
> + list_del(>rq_entry);
> + spin_unlock(>rq_entry_lck);
> + write_unlock_irqrestore(>rq_list_lock, iflags);
> + if (sfp->sum_fd_dlens) {
> + u32 uv = srp->data.dlen;
> +
> + if (uv <= sfp->sum_fd_dlens)
> + sfp->sum_fd_dlens -= uv;
> + else {
> + SG_LOG(2, sfp->parentdp,
> +"%s: logic error this dlen > %s\n",
> +__func__, "sum_fd_dlens");
> + sfp->sum_fd_dlens = 0;
> + }
> + }
> + sg_remove_sgat(srp);
> + /* don't kfree(srp), move clear request to tail of fl */
> + write_lock_irqsave(>rq_list_lock, iflags);
> + spin_lock(>rq_entry_lck);
> + list_add_tail(>free_entry, >rq_free_list);
> + snprintf(b, sizeof(b), "clear sgat srp=0x%p move to fl tail",
> + srp);
> + }
>
>
Here again, no lock protecting sfp->sum_fd_dlens.
Incidentally, I have been using my own home-grown target-mode SCSI
system for the past 16 years, but now I am starting to look into
switching to SCST. I was just reading about their "SGV cache":
http://scst.sourceforge.net/scst_pg.html
It looks like it serves a similar purpose to what you are trying to
accomplish with recycling the indirect I/O buffers between different
requests. Perhaps you can borrow some inspiration from them (or even
some code).
Tony Battersby
Cybernetics
Re: [PATCH 5/8] sg: add free list, rework locking
Hi linux-scsi-owner, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on mkp-scsi/for-next] [also build test WARNING on v4.19-rc8 next-20181019] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/linux-scsi-owner-vger-kernel-org/sg-major-cleanup-remove-max_queue-limit/20181019-183809 base: https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next config: i386-randconfig-x078-201841 (attached as .config) compiler: gcc-7 (Debian 7.3.0-1) 7.3.0 reproduce: # save the attached .config to linux build tree make ARCH=i386 All warnings (new ones prefixed by >>): >> drivers//scsi/sg.c:240:20: warning: 'sg_rq_state_str' used but never defined static const char *sg_rq_state_str(u8 rq_state, bool long_str); ^~~ drivers//scsi/sg.c:933:1: warning: 'sg_fill_request_table' defined but not used [-Wunused-function] sg_fill_request_table(struct sg_fd *sfp, struct sg_req_info *rinfo, ^ drivers//scsi/sg.c:19:12: warning: 'sg_version_num' defined but not used [-Wunused-variable] static int sg_version_num = 30901; /* 2 digits for each component */ ^~ vim +/sg_rq_state_str +240 drivers//scsi/sg.c 212 213 /* tasklet or soft irq callback */ 214 static void sg_rq_end_io(struct request *rq, blk_status_t status); 215 static int sg_start_req(struct sg_request *srp, u8 *cmd); 216 static void sg_finish_scsi_blk_rq(struct sg_request *srp); 217 static int sg_mk_sgat_dlen(struct sg_request *srp, struct sg_fd *sfp, 218 int dlen); 219 static ssize_t sg_new_read(struct sg_fd *sfp, char __user *buf, size_t count, 220 struct sg_request *srp); 221 static ssize_t sg_v3_write(struct sg_fd *sfp, struct file *file, 222 const char __user *buf, size_t count, 223 bool read_only, bool sync, 224 struct sg_request **o_srp); 225 static struct sg_request *sg_common_write(struct sg_fd *sfp, 226const struct sg_io_hdr *hp, 227struct sg_io_v4 *h4p, u8 *cmnd, 228bool sync, int timeout); 229 static int sg_read_oxfer(struct sg_request *srp, char __user *outp, 230 int num_xfer); 231 static void sg_remove_sgat(struct sg_request *srp); 232 static struct sg_fd *sg_add_sfp(struct sg_device *sdp); 233 static void sg_remove_sfp(struct kref *); 234 static struct sg_request *sg_get_rq_pack_id(struct sg_fd *sfp, int pack_id); 235 static struct sg_request *sg_add_request(struct sg_fd *sfp, int dxfr_len, 236 bool sync); 237 static void sg_remove_request(struct sg_fd *sfp, struct sg_request *srp); 238 static struct sg_device *sg_get_dev(int min_dev); 239 static void sg_device_destroy(struct kref *kref); > 240 static const char *sg_rq_state_str(u8 rq_state, bool long_str); 241 --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: application/gzip
