On 21.06.2019 16:02, Scott Long wrote: > 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?
Sure, why not. >> 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 > -- 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"
