| Message ID | 1331430564-32745-7-git-send-email-mjt@msgid.tls.msk.ru |
|---|---|
| State | New |
| Headers | show |
Il 11/03/2012 02:49, Michael Tokarev ha scritto: > The same as for non-coroutine versions in previous patches: > rename arguments to be more obvious, change type of arguments > from int to size_t where appropriate, and use common code for > send and receive paths (with one extra argument) since these > are exactly the same. Use common qemu_sendv_recvv() directly. > Also constify buf arg of qemu_co_send(). > > qemu_co_sendv(), qemu_co_recvv(), and qemu_co_recv() are now > trivial #define's merely adding one extra arg. qemu_co_send() > is an inline function due to `buf' arg de-constification. Again, I don't see the point in using #defines. Either leave the function static, or you can export it but then inlines are preferrable. > qemu_co_sendv() and qemu_co_recvv() callers are converted to > different argument order. Paolo
On 11.03.2012 19:01, Paolo Bonzini wrote: > Il 11/03/2012 02:49, Michael Tokarev ha scritto: >> The same as for non-coroutine versions in previous patches: >> rename arguments to be more obvious, change type of arguments >> from int to size_t where appropriate, and use common code for >> send and receive paths (with one extra argument) since these >> are exactly the same. Use common qemu_sendv_recvv() directly. >> Also constify buf arg of qemu_co_send(). >> >> qemu_co_sendv(), qemu_co_recvv(), and qemu_co_recv() are now >> trivial #define's merely adding one extra arg. qemu_co_send() >> is an inline function due to `buf' arg de-constification. > > Again, I don't see the point in using #defines. Either leave the > function static, or you can export it but then inlines are preferrable. When you're debugging in gdb, it always enters all inline functions. For a very simple #define there's nothing to enter -- hence the #define. Note that - I still hope - in the end there will be no sendv or recv calls at all, only common sendv_recvv with is_write passed as an argument from upper layer. It will be easier to remove that #define - just two lines of code instead of minimum 5 :) Also, in such cases like these, #defines are more compact and does not clutter the header too much. >> qemu_co_sendv() and qemu_co_recvv() callers are converted to >> different argument order. > > Paolo Thanks, /mjt
Il 11/03/2012 16:26, Michael Tokarev ha scritto: > Note that - I still hope - in the end there will be no sendv or > recv calls at all, only common sendv_recvv with is_write passed > as an argument from upper layer. It will be easier to remove > that #define - just two lines of code instead of minimum 5 :) This is what I don't really like in the second part of these patches. You are doing changes for the sake of other changes which are not upstream yet, for which there is no clear vision, and for which there is no clear benefit. While I agree that there is a lot of duplicated code in block.c and block/*, I don't think that what we need is more parameters to the functions. We have places where we need to know the request flags, for example, but the methods are already quite unwieldy and have a lot of arguments. So I'm not sure that this kind of unification buys anything. On top of this, your patches unify things that are similar but not quite identical, and you do not provide hints in the commit messages that you did so. For example, qemu_co_recvv has handling for receiving 0, but qemu_co_sendv does not. Can you please separate the changes to make the argument order uniform? Those should be easy to get in. Paolo
On 12.03.2012 17:30, Paolo Bonzini wrote: > Il 11/03/2012 16:26, Michael Tokarev ha scritto: >> Note that - I still hope - in the end there will be no sendv or >> recv calls at all, only common sendv_recvv with is_write passed >> as an argument from upper layer. It will be easier to remove >> that #define - just two lines of code instead of minimum 5 :) > > This is what I don't really like in the second part of these patches. > You are doing changes for the sake of other changes which are not > upstream yet, for which there is no clear vision, and for which there is > no clear benefit. I already posted the example of this. I can complete whole thing and send it all in one huge chunk if you prefer that :) > While I agree that there is a lot of duplicated code in block.c and > block/*, I don't think that what we need is more parameters to the > functions. We have places where we need to know the request flags, for > example, but the methods are already quite unwieldy and have a lot of > arguments. So I'm not sure that this kind of unification buys anything. Which places are these? Also, how about dropping nr_sectors? If you need flags, well, the extra argument being added can really be used for that if necessary. > On top of this, your patches unify things that are similar but not quite > identical, and you do not provide hints in the commit messages that you > did so. For example, qemu_co_recvv has handling for receiving 0, but > qemu_co_sendv does not. This is a bug, which I dind't notice, it shouldn't be there. Somehow I overlooked this difference, but I really wondered how they're differ! By using common code here it becomes much more obvious (whith the bug actually fixed). I'll take a look at other similar things here and in my block layer changes. > Can you please separate the changes to make the argument order uniform? > Those should be easy to get in. While at it I found another "library", iov.[ch], which has another implementation of the same thing. So that's where it all should have been started. I'll resend a v3 covering iov_* functions too. Thank you! /mjt
Il 12/03/2012 17:29, Michael Tokarev ha scritto: >> For example, qemu_co_recvv has handling for receiving 0, but >> > qemu_co_sendv does not. > This is a bug, which I dind't notice, it shouldn't > be there. Somehow I overlooked this difference, but > I really wondered how they're differ! By using common > code here it becomes much more obvious (whith the bug > actually fixed). write should never return 0, read does for end-of-file. So your code was actually correct in some sense. >> This is what I don't really like in the second part of these patches. >> You are doing changes for the sake of other changes which are not >> upstream yet, for which there is no clear vision, and for which there is >> no clear benefit. > I already posted the example of this. I can complete whole thing > and send it all in one huge chunk if you prefer that > >> While I agree that there is a lot of duplicated code in block.c and >> block/*, I don't think that what we need is more parameters to the >> functions. We have places where we need to know the request flags, for >> example, but the methods are already quite unwieldy and have a lot of >> arguments. So I'm not sure that this kind of unification buys anything. > Which places are these? If we turn zero-write into a special case of discard, we will need it as a flag in discard. Block mirroring would like to have access to copy-on-read flags. > Also, how about dropping nr_sectors? > > If you need flags, well, the extra argument being > added can really be used for that if necessary. Or we can actually clean up everything, and create a real "request" structure that can be passed along. See how flush and discard are really similar (which do not have all the accumulated stuff for throttling, copy-on-read, bounce buffering, etc.). Perhaps that's the place to start. Paolo
diff --git a/block/nbd.c b/block/nbd.c index 161b299..9919acc 100644 --- a/block/nbd.c +++ b/block/nbd.c @@ -194,7 +194,7 @@ static int nbd_co_send_request(BDRVNBDState *s, struct nbd_request *request, nbd_have_request, NULL, s); rc = nbd_send_request(s->sock, request); if (rc != -1 && iov) { - ret = qemu_co_sendv(s->sock, iov, request->len, offset); + ret = qemu_co_sendv(s->sock, iov, offset, request->len); if (ret != request->len) { errno = -EIO; rc = -1; @@ -221,7 +221,7 @@ static void nbd_co_receive_reply(BDRVNBDState *s, struct nbd_request *request, reply->error = EIO; } else { if (iov && reply->error == 0) { - ret = qemu_co_recvv(s->sock, iov, request->len, offset); + ret = qemu_co_recvv(s->sock, iov, offset, request->len); if (ret != request->len) { reply->error = EIO; } diff --git a/block/sheepdog.c b/block/sheepdog.c index 00276f6f..fed436b 100644 --- a/block/sheepdog.c +++ b/block/sheepdog.c @@ -657,8 +657,8 @@ static void coroutine_fn aio_read_response(void *opaque) } break; case AIOCB_READ_UDATA: - ret = qemu_co_recvv(fd, acb->qiov->iov, rsp.data_length, - aio_req->iov_offset); + ret = qemu_co_recvv(fd, acb->qiov->iov, aio_req->iov_offset, + rsp.data_length); if (ret < 0) { error_report("failed to get the data, %s", strerror(errno)); goto out; @@ -924,7 +924,7 @@ static int coroutine_fn add_aio_request(BDRVSheepdogState *s, AIOReq *aio_req, } if (wlen) { - ret = qemu_co_sendv(s->fd, iov, wlen, aio_req->iov_offset); + ret = qemu_co_sendv(s->fd, iov, aio_req->iov_offset, wlen); if (ret < 0) { qemu_co_mutex_unlock(&s->lock); error_report("failed to send a data, %s", strerror(errno)); diff --git a/qemu-common.h b/qemu-common.h index b8190e9..110ec06 100644 --- a/qemu-common.h +++ b/qemu-common.h @@ -306,31 +306,31 @@ struct qemu_work_item { void qemu_init_vcpu(void *env); #endif -/** - * Sends an iovec (or optionally a part of it) down a socket, yielding - * when the socket is full. - */ -int qemu_co_sendv(int sockfd, struct iovec *iov, - int len, int iov_offset); - -/** - * Receives data into an iovec (or optionally into a part of it) from - * a socket, yielding when there is no data in the socket. - */ -int qemu_co_recvv(int sockfd, struct iovec *iov, - int len, int iov_offset); - /** - * Sends a buffer down a socket, yielding when the socket is full. + * Sends a (part of) iovec down a socket, yielding when the socket is full, or + * Receives data into a (part of) iovec from a socket, + * yielding when there is no data in the socket. + * The same interface as qemu_sendv_recvv(), with added yielding. + * XXX should mark these as coroutine_fn */ -int qemu_co_send(int sockfd, void *buf, int len); +int qemu_co_sendv_recvv(int sockfd, struct iovec *iov, + size_t offset, size_t bytes, bool do_send); +#define qemu_co_recvv(sockfd, iov, offset, bytes) \ + qemu_co_sendv_recvv(sockfd, iov, offset, bytes, false) +#define qemu_co_sendv(sockfd, iov, offset, bytes) \ + qemu_co_sendv_recvv(sockfd, iov, offset, bytes, true) /** - * Receives data into a buffer from a socket, yielding when there - * is no data in the socket. + * The same as above, but with just a single buffer */ -int qemu_co_recv(int sockfd, void *buf, int len); +int qemu_co_send_recv(int sockfd, void *buf, size_t bytes, bool do_send); +#define qemu_co_recv(sockfd, buf, bytes) \ + qemu_co_send_recv(sockfd, buf, bytes, false) +static inline int qemu_co_send(int sockfd, const void *buf, size_t bytes) +{ + return qemu_co_send_recv(sockfd, (void*)buf, bytes, true); +} typedef struct QEMUIOVector { diff --git a/qemu-coroutine-io.c b/qemu-coroutine-io.c index df8ff21..75a4117 100644 --- a/qemu-coroutine-io.c +++ b/qemu-coroutine-io.c @@ -26,71 +26,32 @@ #include "qemu_socket.h" #include "qemu-coroutine.h" -int coroutine_fn qemu_co_recvv(int sockfd, struct iovec *iov, - int len, int iov_offset) +int coroutine_fn +qemu_co_sendv_recvv(int sockfd, struct iovec *iov, + size_t offset, size_t bytes, bool do_send) { - int total = 0; + size_t done = 0; int ret; - while (len) { - ret = qemu_recvv(sockfd, iov, iov_offset + total, len); - if (ret < 0) { - if (errno == EAGAIN) { - qemu_coroutine_yield(); - continue; - } - if (total == 0) { - total = -1; - } - break; - } - if (ret == 0) { - break; - } - total += ret, len -= ret; + while (done < bytes) { + ret = qemu_sendv_recvv(sockfd, iov, offset + done, bytes - done, do_send); + if (ret > 0) { + done += ret; + } else if (!ret) { + break; + } else if (errno == EAGAIN) { + qemu_coroutine_yield(); + } else if (done == 0) { + return -1; + } else { + break; + } } - - return total; -} - -int coroutine_fn qemu_co_sendv(int sockfd, struct iovec *iov, - int len, int iov_offset) -{ - int total = 0; - int ret; - while (len) { - ret = qemu_sendv(sockfd, iov, iov_offset + total, len); - if (ret < 0) { - if (errno == EAGAIN) { - qemu_coroutine_yield(); - continue; - } - if (total == 0) { - total = -1; - } - break; - } - total += ret, len -= ret; - } - - return total; + return done; } -int coroutine_fn qemu_co_recv(int sockfd, void *buf, int len) +int coroutine_fn +qemu_co_send_recv(int sockfd, void *buf, size_t bytes, bool do_send) { - struct iovec iov; - - iov.iov_base = buf; - iov.iov_len = len; - - return qemu_co_recvv(sockfd, &iov, len, 0); -} - -int coroutine_fn qemu_co_send(int sockfd, void *buf, int len) -{ - struct iovec iov; - - iov.iov_base = buf; - iov.iov_len = len; - - return qemu_co_sendv(sockfd, &iov, len, 0); + struct iovec iov = { buf, bytes }; + return qemu_co_sendv_recvv(sockfd, &iov, 0, bytes, do_send); }
The same as for non-coroutine versions in previous patches: rename arguments to be more obvious, change type of arguments from int to size_t where appropriate, and use common code for send and receive paths (with one extra argument) since these are exactly the same. Use common qemu_sendv_recvv() directly. Also constify buf arg of qemu_co_send(). qemu_co_sendv(), qemu_co_recvv(), and qemu_co_recv() are now trivial #define's merely adding one extra arg. qemu_co_send() is an inline function due to `buf' arg de-constification. qemu_co_sendv() and qemu_co_recvv() callers are converted to different argument order. Signed-off-by: Michael Tokarev <mjt@tls.msk.ru> --- block/nbd.c | 4 +- block/sheepdog.c | 6 ++-- qemu-common.h | 38 +++++++++++----------- qemu-coroutine-io.c | 83 +++++++++++++------------------------------------- 4 files changed, 46 insertions(+), 85 deletions(-)