| Message ID | 1457210381-23325-1-git-send-email-aesmade@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Nikos Filippakis <aesmade@gmail.com> writes: > Hello everyone! I am interested in getting to know the codebase a little better > so that I can eventually apply for a GSOC position. > This is my first attempt at posting a patch to a mailing list, any feedback > is greatly appreciated. OK first things first this sort of meta comment belongs in the cover letter. However for a single patch you may want to put such things below the --- in the commit message as that will get stripped when the maintainer eventually applies the patch. Otherwise your meta-comments will end up in the version log ;-) You'll see people use the --- area to keep version notes as patches go through revisions. > > Signed-off-by: Nikos Filippakis <aesmade@gmail.com> > --- > net/net.c | 17 ++++++++++++----- > 1 file changed, 12 insertions(+), 5 deletions(-) > > diff --git a/net/net.c b/net/net.c > index aebf753..79e9d7c 100644 > --- a/net/net.c > +++ b/net/net.c > @@ -710,23 +710,30 @@ ssize_t qemu_send_packet_raw(NetClientState *nc, const uint8_t *buf, int size) > static ssize_t nc_sendv_compat(NetClientState *nc, const struct iovec *iov, > int iovcnt, unsigned flags) > { > - uint8_t buf[NET_BUFSIZE]; > uint8_t *buffer; > size_t offset; > + ssize_t ret; With that said your comment needs to explain why you've just made the change. I see NET_BUFSIZE is quite large so maybe this should be a clean-up across the rest of the code-base, what's so special about this function? Have you measured any difference in performance? > > if (iovcnt == 1) { > buffer = iov[0].iov_base; > offset = iov[0].iov_len; > } else { > - buffer = buf; > - offset = iov_to_buf(iov, iovcnt, 0, buf, sizeof(buf)); > + buffer = g_malloc(NET_BUFSIZE * sizeof(uint8_t)); > + offset = iov_to_buf(iov, iovcnt, 0, buffer, > + NET_BUFSIZE * sizeof(uint8_t)); > } > > if (flags & QEMU_NET_PACKET_FLAG_RAW && nc->info->receive_raw) { > - return nc->info->receive_raw(nc, buffer, offset); > + ret = nc->info->receive_raw(nc, buffer, offset); > } else { > - return nc->info->receive(nc, buffer, offset); > + ret = nc->info->receive(nc, buffer, offset); > } > + > + if (iovcnt != 1) { > + g_free(buffer); > + } This is a short function so you can get away with it but this sort of logic can be confusing ("The iovec count was 1 therefor I should have allocated a buffer" vs "I have an allocated buffer"). In general you should know the various g_* functions tolerate NULLs well so maybe you can structure the code differently (skipping the details ;-): uint8_t *buffer, *dynbuf = NULL; if (iovcnt == 1) { buffer = ... } else { buffer = dynbuf = g_malloc(NET_BUFSIZE * sizeof(uint8_t)); ... } ... g_free(dynbuf) > + > + return ret; > } > > ssize_t qemu_deliver_packet_iov(NetClientState *sender, -- Alex Bennée
Thank you for your comments! On Sun, Mar 6, 2016 at 9:47 AM, Alex Bennée <alex.bennee@linaro.org> wrote: > > > Nikos Filippakis <aesmade@gmail.com> writes: > > > Hello everyone! I am interested in getting to know the codebase a little better > > so that I can eventually apply for a GSOC position. > > This is my first attempt at posting a patch to a mailing list, any feedback > > is greatly appreciated. > > OK first things first this sort of meta comment belongs in the cover > letter. However for a single patch you may want to put such things below > the --- in the commit message as that will get stripped when the > maintainer eventually applies the patch. Otherwise your meta-comments > will end up in the version log ;-) > > You'll see people use the --- area to keep version notes as patches go > through revisions. > I thought that could be considered part of the cover letter, didn't realize it would end up on the version log. Sorry about that (: > > > > Signed-off-by: Nikos Filippakis <aesmade@gmail.com> > > --- > > net/net.c | 17 ++++++++++++----- > > 1 file changed, 12 insertions(+), 5 deletions(-) > > > > diff --git a/net/net.c b/net/net.c > > index aebf753..79e9d7c 100644 > > --- a/net/net.c > > +++ b/net/net.c > > @@ -710,23 +710,30 @@ ssize_t qemu_send_packet_raw(NetClientState *nc, const uint8_t *buf, int size) > > static ssize_t nc_sendv_compat(NetClientState *nc, const struct iovec *iov, > > int iovcnt, unsigned flags) > > { > > - uint8_t buf[NET_BUFSIZE]; > > uint8_t *buffer; > > size_t offset; > > + ssize_t ret; > > With that said your comment needs to explain why you've just made the > change. I see NET_BUFSIZE is quite large so maybe this should be a > clean-up across the rest of the code-base, what's so special about this > function? Have you measured any difference in performance? > This method is one of several mentioned on the wiki as having big stack frames because of such arrays, something someone new to the project could easily fix, either by moving it to the heap or reducing the array size. Since further down there is a call to memcpy with NET_BUFSIZE length, I thought I'd just move it to the heap. Nothing special about this method, I'm planning to do the same with the others, just trying to get some familiarity with the mailing list. Besides, I'm not sure if I should put such small changes to different files in many small commits, or a large one. > > > > if (iovcnt == 1) { > > buffer = iov[0].iov_base; > > offset = iov[0].iov_len; > > } else { > > - buffer = buf; > > - offset = iov_to_buf(iov, iovcnt, 0, buf, sizeof(buf)); > > + buffer = g_malloc(NET_BUFSIZE * sizeof(uint8_t)); > > + offset = iov_to_buf(iov, iovcnt, 0, buffer, > > + NET_BUFSIZE * sizeof(uint8_t)); > > } > > > > if (flags & QEMU_NET_PACKET_FLAG_RAW && nc->info->receive_raw) { > > - return nc->info->receive_raw(nc, buffer, offset); > > + ret = nc->info->receive_raw(nc, buffer, offset); > > } else { > > - return nc->info->receive(nc, buffer, offset); > > + ret = nc->info->receive(nc, buffer, offset); > > } > > + > > + if (iovcnt != 1) { > > + g_free(buffer); > > + } > > This is a short function so you can get away with it but this sort of > logic can be confusing ("The iovec count was 1 therefor I should have > allocated a buffer" vs "I have an allocated buffer"). In general you > should know the various g_* functions tolerate NULLs well so maybe you > can structure the code differently (skipping the details ;-): > > uint8_t *buffer, *dynbuf = NULL; > > if (iovcnt == 1) > { > buffer = ... > } else { > buffer = dynbuf = g_malloc(NET_BUFSIZE * sizeof(uint8_t)); > ... > } > ... > > g_free(dynbuf) > You're right, I didn't quite like the way I did it either. I'm resubmitting it, hopefully fixing these mistakes. > > + > > + return ret; > > } > > > > ssize_t qemu_deliver_packet_iov(NetClientState *sender, > > > -- > Alex Bennée
Nikos Filippakis <aesmade@gmail.com> writes: > Thank you for your comments! > > On Sun, Mar 6, 2016 at 9:47 AM, Alex Bennée <alex.bennee@linaro.org> wrote: >> >> >> Nikos Filippakis <aesmade@gmail.com> writes: >> >> > Hello everyone! I am interested in getting to know the codebase a little better >> > so that I can eventually apply for a GSOC position. >> > This is my first attempt at posting a patch to a mailing list, any feedback >> > is greatly appreciated. >> >> OK first things first this sort of meta comment belongs in the cover >> letter. However for a single patch you may want to put such things below >> the --- in the commit message as that will get stripped when the >> maintainer eventually applies the patch. Otherwise your meta-comments >> will end up in the version log ;-) >> >> You'll see people use the --- area to keep version notes as patches go >> through revisions. >> > > I thought that could be considered part of the cover letter, didn't > realize it would end up on the version log. Sorry about that (: When you use git format-patch with --cover-letter to format a series of patches you'll get exactly that. For individual patches like this one then bellow the --- works. The fact your a potential GSOC student is useful information to us on the list, just not in the actual commit log in git ;-) > >> > >> > Signed-off-by: Nikos Filippakis <aesmade@gmail.com> >> > --- >> > net/net.c | 17 ++++++++++++----- >> > 1 file changed, 12 insertions(+), 5 deletions(-) >> > >> > diff --git a/net/net.c b/net/net.c >> > index aebf753..79e9d7c 100644 >> > --- a/net/net.c >> > +++ b/net/net.c >> > @@ -710,23 +710,30 @@ ssize_t qemu_send_packet_raw(NetClientState *nc, const uint8_t *buf, int size) >> > static ssize_t nc_sendv_compat(NetClientState *nc, const struct iovec *iov, >> > int iovcnt, unsigned flags) >> > { >> > - uint8_t buf[NET_BUFSIZE]; >> > uint8_t *buffer; >> > size_t offset; >> > + ssize_t ret; >> >> With that said your comment needs to explain why you've just made the >> change. I see NET_BUFSIZE is quite large so maybe this should be a >> clean-up across the rest of the code-base, what's so special about this >> function? Have you measured any difference in performance? >> > > This method is one of several mentioned on the wiki as having big > stack frames because of such arrays, something > someone new to the project could easily fix, either by moving it to > the heap or reducing the array size. Since further > down there is a call to memcpy with NET_BUFSIZE length, I thought I'd > just move it to the heap. That's fine. In fact referencing the wiki bite-sized tasks would probably be enough context for the commit message. > Nothing special about this method, I'm planning to do the same with > the others, just trying to get some > familiarity with the mailing list. Don't worry too much, it usually takes a few attempts to get your first patch applied and the workflow sorted out. > Besides, I'm not sure if I should put such small changes to different > files in many small commits, or a large one. The key byword here is bisectability. If regressions get introduced we want to be able to quickly identify the culprit with git-bisect. So it is important that every commit in the project builds cleanly. For something like this I'd argue a series of patches would make sense as they are likely in different functional places in the code. > >> > >> > if (iovcnt == 1) { >> > buffer = iov[0].iov_base; >> > offset = iov[0].iov_len; >> > } else { >> > - buffer = buf; >> > - offset = iov_to_buf(iov, iovcnt, 0, buf, sizeof(buf)); >> > + buffer = g_malloc(NET_BUFSIZE * sizeof(uint8_t)); >> > + offset = iov_to_buf(iov, iovcnt, 0, buffer, >> > + NET_BUFSIZE * sizeof(uint8_t)); >> > } >> > >> > if (flags & QEMU_NET_PACKET_FLAG_RAW && nc->info->receive_raw) { >> > - return nc->info->receive_raw(nc, buffer, offset); >> > + ret = nc->info->receive_raw(nc, buffer, offset); >> > } else { >> > - return nc->info->receive(nc, buffer, offset); >> > + ret = nc->info->receive(nc, buffer, offset); >> > } >> > + >> > + if (iovcnt != 1) { >> > + g_free(buffer); >> > + } >> >> This is a short function so you can get away with it but this sort of >> logic can be confusing ("The iovec count was 1 therefor I should have >> allocated a buffer" vs "I have an allocated buffer"). In general you >> should know the various g_* functions tolerate NULLs well so maybe you >> can structure the code differently (skipping the details ;-): >> >> uint8_t *buffer, *dynbuf = NULL; >> >> if (iovcnt == 1) >> { >> buffer = ... >> } else { >> buffer = dynbuf = g_malloc(NET_BUFSIZE * sizeof(uint8_t)); >> ... >> } >> ... >> >> g_free(dynbuf) >> > > You're right, I didn't quite like the way I did it either. I'm > resubmitting it, hopefully fixing these mistakes. This is more a question of style rather than mistakes in the code. However taste is a good guide, while sometimes code is as ugly as it needs to be it is often worthwhile investigating alternatives if your initial reaction is ambivalent. > >> > + >> > + return ret; >> > } >> > >> > ssize_t qemu_deliver_packet_iov(NetClientState *sender, >> >> >> -- >> Alex Bennée -- Alex Bennée
On 05/03/2016 21:39, Nikos Filippakis wrote: > Hello everyone! I am interested in getting to know the codebase a little better > so that I can eventually apply for a GSOC position. > This is my first attempt at posting a patch to a mailing list, any feedback > is greatly appreciated. > > Signed-off-by: Nikos Filippakis <aesmade@gmail.com> > --- > net/net.c | 17 ++++++++++++----- > 1 file changed, 12 insertions(+), 5 deletions(-) > > diff --git a/net/net.c b/net/net.c > index aebf753..79e9d7c 100644 > --- a/net/net.c > +++ b/net/net.c > @@ -710,23 +710,30 @@ ssize_t qemu_send_packet_raw(NetClientState *nc, const uint8_t *buf, int size) > static ssize_t nc_sendv_compat(NetClientState *nc, const struct iovec *iov, > int iovcnt, unsigned flags) > { > - uint8_t buf[NET_BUFSIZE]; > uint8_t *buffer; > size_t offset; > + ssize_t ret; > > if (iovcnt == 1) { > buffer = iov[0].iov_base; > offset = iov[0].iov_len; > } else { > - buffer = buf; > - offset = iov_to_buf(iov, iovcnt, 0, buf, sizeof(buf)); > + buffer = g_malloc(NET_BUFSIZE * sizeof(uint8_t)); > + offset = iov_to_buf(iov, iovcnt, 0, buffer, > + NET_BUFSIZE * sizeof(uint8_t)); > } Hi Nikos, I suspect an allocation (especially a large one) on this path can have a performance impact. On one hand, nc_sendv_compat is not used in high-performance network cards (e1000 and virtio-net, for example) so this is not a major issue. On the other hand, this is the first such conversion so you could set a good example. Another possibility thus is to make the buffer smaller (e.g. 2K) and check the size with iov_size. If the size is at most 2K, you use the on-stack buffer. If the size is bigger than 2K and smaller than NET_BUFSIZE, you go through the allocation but you only need to allocate the correct number of bytes. Finally, if the size is bigger than NET_BUFSIZE you set errno = EINVAL and return -1. Thanks, Paolo > if (flags & QEMU_NET_PACKET_FLAG_RAW && nc->info->receive_raw) { > - return nc->info->receive_raw(nc, buffer, offset); > + ret = nc->info->receive_raw(nc, buffer, offset); > } else { > - return nc->info->receive(nc, buffer, offset); > + ret = nc->info->receive(nc, buffer, offset); > } > + > + if (iovcnt != 1) { > + g_free(buffer); > + } > + > + return ret; > } > > ssize_t qemu_deliver_packet_iov(NetClientState *sender, >
diff --git a/net/net.c b/net/net.c index aebf753..79e9d7c 100644 --- a/net/net.c +++ b/net/net.c @@ -710,23 +710,30 @@ ssize_t qemu_send_packet_raw(NetClientState *nc, const uint8_t *buf, int size) static ssize_t nc_sendv_compat(NetClientState *nc, const struct iovec *iov, int iovcnt, unsigned flags) { - uint8_t buf[NET_BUFSIZE]; uint8_t *buffer; size_t offset; + ssize_t ret; if (iovcnt == 1) { buffer = iov[0].iov_base; offset = iov[0].iov_len; } else { - buffer = buf; - offset = iov_to_buf(iov, iovcnt, 0, buf, sizeof(buf)); + buffer = g_malloc(NET_BUFSIZE * sizeof(uint8_t)); + offset = iov_to_buf(iov, iovcnt, 0, buffer, + NET_BUFSIZE * sizeof(uint8_t)); } if (flags & QEMU_NET_PACKET_FLAG_RAW && nc->info->receive_raw) { - return nc->info->receive_raw(nc, buffer, offset); + ret = nc->info->receive_raw(nc, buffer, offset); } else { - return nc->info->receive(nc, buffer, offset); + ret = nc->info->receive(nc, buffer, offset); } + + if (iovcnt != 1) { + g_free(buffer); + } + + return ret; } ssize_t qemu_deliver_packet_iov(NetClientState *sender,
Hello everyone! I am interested in getting to know the codebase a little better so that I can eventually apply for a GSOC position. This is my first attempt at posting a patch to a mailing list, any feedback is greatly appreciated. Signed-off-by: Nikos Filippakis <aesmade@gmail.com> --- net/net.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-)