Page MenuHomeFreeBSD
Differential D28968 Diff 84798 lib/libgeom/geom_stats.c

Changeset View

Standalone View

View Options

lib/libgeom/geom_stats.c

Show All 26 Lines
* LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
* OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
* SUCH DAMAGE. * SUCH DAMAGE.
* *
* $FreeBSD$ * $FreeBSD$
*/ */
#include <sys/types.h> #include <sys/types.h>
#include <sys/ioctl.h>
#include <sys/disk.h>
#include <sys/devicestat.h> #include <sys/devicestat.h>
#include <sys/mman.h> #include <sys/mman.h>
#include <sys/time.h> #include <sys/time.h>
#include <err.h>
#include <errno.h> #include <errno.h>
#include <fcntl.h> #include <fcntl.h>
#include <paths.h> #include <paths.h>
#include <stdlib.h> #include <stdlib.h>
#include <string.h> #include <string.h>
#include <unistd.h> #include <unistd.h>
#include <libgeom.h> #include <libgeom.h>
Show All 12 Linesgeom_stats_close(void)
close (statsfd); close (statsfd);
statsfd = -1; statsfd = -1;
} }
void void
geom_stats_resync(void) geom_stats_resync(void)
{ {
void *p; void *p;
off_t mediasize;
int error;
if (statsfd == -1) if (statsfd == -1)
return; return;
for (;;) { error = ioctl(statsfd, DIOCGMEDIASIZE, &mediasize);
p = mmap(statp, (npages + 1) * pagesize, if (error)
PROT_READ, MAP_SHARED, statsfd, 0); err(1, "DIOCGMEDIASIZE(" _PATH_DEV DEVSTAT_DEVICE_NAME ")");
mavUnsubmitted
Not Done
Inline Actions

I haven't used mmap() that much to know. I am curios whether the lack of munmap() before used some trick not avoid extra actions.

mav: I haven't used mmap() that much to know. I am curios whether the lack of munmap() before used…
asomersAuthorUnsubmitted
Done
Inline Actions

I don't know. But the resulted in rapid accumulation of MAP ENTRY memory with every call to geom_stats_resync

asomers: I don't know. But the resulted in rapid accumulation of `MAP ENTRY` memory with every call to…
p = mmap(statp, mediasize, PROT_READ, MAP_SHARED, statsfd, 0);
if (p == MAP_FAILED) if (p == MAP_FAILED)
break; err(1, "mmap:");
mavUnsubmitted
Not Done
Inline Actions

In the new code you are neither reading nor updating npages variable. You could read npages to skip mmap() call if nothing has changed. And you should update it for other library parts to work correctly.

mav: In the new code you are neither reading nor updating npages variable. You could read npages to…
mavUnsubmitted
Not Done
Inline Actions

I don't very like the use of DIOCGMEDIASIZE here, but I have no better ideas.

mav: I don't very like the use of DIOCGMEDIASIZE here, but I have no better ideas.
impUnsubmitted
Not Done
Inline Actions

I had the same thought... but didn't want to suggest a new one

imp: I had the same thought... but didn't want to suggest a new one
else
statp = p;
npages++;
}
} }
int int
geom_stats_open(void) geom_stats_open(void)
{ {
int error; int error;
void *p; void *p;
Show All 39 Linesgeom_stats_snapshot_get(void)
memset(sp, 0, sizeof *sp); memset(sp, 0, sizeof *sp);
sp->ptr = malloc(pagesize * npages); sp->ptr = malloc(pagesize * npages);
if (sp->ptr == NULL) { if (sp->ptr == NULL) {
free(sp); free(sp);
return (NULL); return (NULL);
} }
memset(sp->ptr, 0, pagesize * npages); /* page in, cache */ memset(sp->ptr, 0, pagesize * npages); /* page in, cache */
clock_gettime(CLOCK_REALTIME, &sp->time); clock_gettime(CLOCK_REALTIME, &sp->time);
memset(sp->ptr, 0, pagesize * npages); /* page in, cache */ memset(sp->ptr, 0, pagesize * npages); /* page in, cache */
mavUnsubmitted
Not Done
Inline Actions

I think previous order was intentional to make time more up to date of paging on access take some time.

mav: I think previous order was intentional to make time more up to date of paging on access take…
asomersAuthorUnsubmitted
Done
Inline Actions

Interesting idea. Do you have any good idea why there used to be two memset calls?

asomers: Interesting idea. Do you have any good idea why there used to be two memset calls?
mavUnsubmitted
Not Done
Inline Actions

Not really. Guess some form of swap pager woodoo. Something like first pass triggers swap-in, while second brings up to date TLB caches if not yet, to make following copy as coherent as possible.

mav: Not really. Guess some form of swap pager woodoo. Something like first pass triggers swap-in…
asomersAuthorUnsubmitted
Done
Inline Actions

So you really think both memsets are required?

asomers: So you really think both memsets are required?
mavUnsubmitted
Not Done
Inline Actions

And you think it is a copy/paste bug?

mav: And you think it is a copy/paste bug?
asomersAuthorUnsubmitted
Done
Inline Actions

Yes, I think it's a copy/paste bug.

asomers: Yes, I think it's a copy/paste bug.
mavUnsubmitted
Not Done
Inline Actions

I this case I'd say don't fix what's not broken. But whatever you decide. Responsibility is yours. ;)

mav: I this case I'd say don't fix what's not broken. But whatever you decide. Responsibility is…
asomersAuthorUnsubmitted
Done
Inline Actions

Even though that code is very old, the original author is still active. So I'll pull it out of this review and take it up separately.

asomers: Even though that code is very old, the original author is still active. So I'll pull it out of…
memcpy(sp->ptr, statp, pagesize * npages); memcpy(sp->ptr, statp, pagesize * npages);
sp->pages = npages; sp->pages = npages;
sp->perpage = spp; sp->perpage = spp;
sp->pagesize = pagesize; sp->pagesize = pagesize;
return (sp); return (sp);
} }
void void
▲ Show 20 Lines • Show All 44 Lines • Show Last 20 Lines