Hi Alexander, Thanks for the explanation, sorry I didn’t realize sooner that it’s not used as a copyout buffer anymore. FWIW, that code is really hard to read on an 80 column window, would you be ok if it was refactored for better readability?
Thanks, Scott > On Jun 20, 2019, at 6:04 PM, Alexander Motin <m...@freebsd.org> wrote: > > Hi Scott, > > You may see this buffer content is not returned directly to user any > more. It is used only as temporary storage to pull > CDAI_TYPE_SCSI_DEVID, which is then converted into readable form, > returned to user. And the code is explicitly made to not read outside > the range that was copied to the buffer with simple and dumb memcpy(). > > The effect is clearly visible in profiler when GEOM confxml calls it 3 > times for each disk, taking about 25% of the sysctl's CPU time. With > hundreds of disks it is not so small. > > On 20.06.2019 18:59, Scott Long wrote: >> Hi Alexander, >> >> I’m not a fan of removing the M_ZERO. I understand your argument that >> lengths are provided elsewhere, but having the buffer be zero’d is defensive >> against future bugs that might leak buffer contents. GETATTR isn’t typically >> in a performance path, is there any measurable benefit to this? >> >> Thanks, >> Scott >> >> >>> On Jun 20, 2019, at 2:29 PM, Alexander Motin <m...@freebsd.org> wrote: >>> >>> Author: mav >>> Date: Thu Jun 20 20:29:42 2019 >>> New Revision: 349243 >>> URL: https://svnweb.freebsd.org/changeset/base/349243 >>> >>> Log: >>> Optimize xpt_getattr(). >>> >>> Do not allocate temporary buffer for attributes we are going to return >>> as-is, just make sure to NUL-terminate them. Do not zero temporary 64KB >>> buffer for CDAI_TYPE_SCSI_DEVID, XPT tells us how much data it filled >>> and there are also length fields inside the returned data also. >>> >>> MFC after: 2 weeks >>> Sponsored by: iXsystems, Inc. >>> >>> Modified: >>> head/sys/cam/cam_xpt.c >>> >>> Modified: head/sys/cam/cam_xpt.c >>> ============================================================================== >>> --- head/sys/cam/cam_xpt.c Thu Jun 20 20:06:19 2019 (r349242) >>> +++ head/sys/cam/cam_xpt.c Thu Jun 20 20:29:42 2019 (r349243) >>> @@ -1253,6 +1253,7 @@ xpt_getattr(char *buf, size_t len, const char *attr, s >>> cdai.ccb_h.func_code = XPT_DEV_ADVINFO; >>> cdai.flags = CDAI_FLAG_NONE; >>> cdai.bufsiz = len; >>> + cdai.buf = buf; >>> >>> if (!strcmp(attr, "GEOM::ident")) >>> cdai.buftype = CDAI_TYPE_SERIAL_NUM; >>> @@ -1262,14 +1263,14 @@ xpt_getattr(char *buf, size_t len, const char >>> *attr, s >>> strcmp(attr, "GEOM::lunname") == 0) { >>> cdai.buftype = CDAI_TYPE_SCSI_DEVID; >>> cdai.bufsiz = CAM_SCSI_DEVID_MAXLEN; >>> + cdai.buf = malloc(cdai.bufsiz, M_CAMXPT, M_NOWAIT); >>> + if (cdai.buf == NULL) { >>> + ret = ENOMEM; >>> + goto out; >>> + } >>> } else >>> goto out; >>> >>> - cdai.buf = malloc(cdai.bufsiz, M_CAMXPT, M_NOWAIT|M_ZERO); >>> - if (cdai.buf == NULL) { >>> - ret = ENOMEM; >>> - goto out; >>> - } >>> xpt_action((union ccb *)&cdai); /* can only be synchronous */ >>> if ((cdai.ccb_h.status & CAM_DEV_QFRZN) != 0) >>> cam_release_devq(cdai.ccb_h.path, 0, 0, 0, FALSE); >>> @@ -1334,13 +1335,15 @@ xpt_getattr(char *buf, size_t len, const char >>> *attr, s >>> ret = EFAULT; >>> } >>> } else { >>> - ret = 0; >>> - if (strlcpy(buf, cdai.buf, len) >= len) >>> + if (cdai.provsiz < len) { >>> + cdai.buf[cdai.provsiz] = 0; >>> + ret = 0; >>> + } else >>> ret = EFAULT; >>> } >>> >>> out: >>> - if (cdai.buf != NULL) >>> + if ((char *)cdai.buf != buf) >>> free(cdai.buf, M_CAMXPT); >>> return ret; >>> } >>> >> > > -- > Alexander Motin _______________________________________________ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
