[Xen-devel] Re: [PATCH] libxl: reimplement buffer for bootloading and drop data if buffer is full

[Roger Pau Monnà <roger.pau@xxxxxxxxxxxxx>]

2011/10/13 Ian Campbell <Ian.Campbell@xxxxxxxxxx>:
> On Thu, 2011-10-13 at 09:43 +0100, Roger Pau Monnà wrote:
>
>> >> Â Â Â Â ÂFD_ZERO(&rsel);
>> >> Â Â Â Â ÂFD_SET(fifo_fd, &rsel);
>> >> Â Â Â Â Ânfds = fifo_fd + 1;
>> >> - Â Â Â Âif (xenconsoled_prod == 0 || (xenconsoled_prod < 
>> >> BOOTLOADER_BUF_SIZE && xenconsoled_cons == 0)) {
>> >> + Â Â Â Âif (xenconsoled_prod == 0 || (xenconsoled_prod < 
>> >> BOOTLOADER_BUF_SIZE && xenconsoled_cons == 0) || timeout) {
>> >
>> > If you've had a timeout there will be data pending on this FD won't
>> > there, so this change essentially means that after the first timeout the
>> > timeout on the select becomes effectively zero?
>>
>> Well, I cannot know this for sure, but it's the most likely situation.
>>
>> > Can you just make this:
>> > Â Â Â Â Â if (xenconsoled_prod == 0 || xenconsoled_cons == 0) {
>>
>> I don't think there's any need for this pair of "ifs", since they will
>> always be hit, because xenconsoled_cons and bootloader_cons is always
>> 0 at this point, I agree with you that this might be dropping data to
>> aggressively, since the timeout will not be needed. Something like
>> this might be more suitable:
>>
>> if (bootloader_prod == 0 || ret == 0) {
>>
>> The "timeout" variable is not really needed, I've just added it to
>> make the code cleaner and easier to understand,
>
> Actually, the timeout variable was ok my concern was more the use of
> continue and the code flow it seemed to create which wasn't easy to
> follow.

Ok, I think I've managed to write it in a more friendly way.

> If the buffer is full and we therefore don't add it to the select then
> either
> Â Â Â* some other file descriptor in the select becomes read for
> Â Â Â Âread/write as appropriate, in which case some progress will be
> Â Â Â Âmade, - or -
> Â Â Â* the timeout will fire and we will discard some data, such that
> Â Â Â Ânext time round the loop we can make progress.
>
> Do we need to care about the case where no fd's get added to the select?

I don't think it's possible, since read fd's get added when there's
space in the buffer, and write fd's gets added when there's data in
the buffer, so either way there will be fd's in select.

buffer empty: read fd's are added
buffer contains data but it's not full: read & write fd's are added
buffer full: write fd's are added.

> Doesn't dropping any part of the data stream have that same property? Is
> dropping the tail somehow worse?

Dropping from the head and adding data to the tail makes the data
stream in the buffer contiguous, there are no breaks inside it. On the
other hand, rewriting data in the tail (or the head) introduces one
(or possible more) breaks in the data stream. What I mean with "break"
is that the data is no longer in the same order as it came from the
fd.

>>
>> > Hmm. that might discard a bit too aggressively, perhaps leaving this
>> > check as it was and doing the discard if (ret == 0 && xenconsoled_prod
>> > == BOOTLOADER_BUF_SIZE) in the else clause if the FD_ISSET works?
>>
>> This might make the select loop too fast, since we add all the
>> descriptors to the set, and it's a strong possibility that the timeout
>> will never be hit, so ret will never be 0 and we will be looping
>> forever.
>
> That's why we should not add fd's to the set which we cannot make
> progress with if they become/are ready.
>
> Ian.
>
>>
>> >> Â Â Â Â Â Â ÂFD_SET(xenconsoled_fd, &rsel);
>> >> Â Â Â Â Â Â Ânfds = xenconsoled_fd + 1 > nfds ? xenconsoled_fd + 1 : nfds;
>> >> Â Â Â Â Â}
>> >> - Â Â Â Âif (bootloader_prod == 0 || (bootloader_prod < 
>> >> BOOTLOADER_BUF_SIZE && bootloader_cons == 0)) {
>> >> + Â Â Â Âif (bootloader_prod == 0 || (bootloader_prod < 
>> >> BOOTLOADER_BUF_SIZE && bootloader_cons == 0) || timeout) {
>> >> Â Â Â Â Â Â ÂFD_SET(bootloader_fd, &rsel);
>> >> Â Â Â Â Â Â Ânfds = bootloader_fd + 1 > nfds ? bootloader_fd + 1 : nfds;
>> >> Â Â Â Â Â}
>> >> @@ -208,12 +226,23 @@ static char * bootloader_interact(libxl_
>> >> Â Â Â Â Â Â Ânfds = bootloader_fd + 1 > nfds ? bootloader_fd + 1 : nfds;
>> >> Â Â Â Â Â}
>> >>
>> >> - Â Â Â Âret = select(nfds, &rsel, &wsel, NULL, NULL);
>> >> + Â Â Â Âret = select(nfds, &rsel, &wsel, NULL, &wait);
>> >> Â Â Â Â Âif (ret < 0)
>> >> Â Â Â Â Â Â Âgoto out_err;
>> >> + Â Â Â Âif (ret == 0) {
>> >> + Â Â Â Â Â Âtimeout = 1;
>> >> + Â Â Â Â Â Âcontinue;
>> >> + Â Â Â Â}
>> >> + Â Â Â Âtimeout = 0;
>> >>
>> >> Â Â Â Â Â/* Input from xenconsole, read xenconsoled_fd, write 
>> >> bootloader_fd */
>> >> Â Â Â Â Âif (FD_ISSET(xenconsoled_fd, &rsel)) {
>> >> + Â Â Â Â Â Âif (xenconsoled_prod == XENCONSOLED_BUF_SIZE) {
>> >> + Â Â Â Â Â Â Â Âif (ioctl(xenconsoled_fd, FIONREAD, &read_ahead) < 0)
>> >
>> > I think we can avoid this as described above, but if not then you need
>> > to be wary of the case where read_ahead > XENCONSOLED_BUF_SIZE.
>>
>> Yes, I've also missed that one, this should be checked to avoid
>> passing a negative integer to read or moving memory regions outside of
>> the buffer.
>>
>> >> + Â Â Â Â Â Â Â Â Â Âgoto out_err;
>> >> + Â Â Â Â Â Â Â Âmemmove(xenconsoled_buf, &xenconsoled_buf[read_ahead], 
>> >> XENCONSOLED_BUF_SIZE - read_ahead);
>> >> + Â Â Â Â Â Â Â Âxenconsoled_prod -= read_ahead;
>> >> + Â Â Â Â Â Â}
>> >> Â Â Â Â Â Â Âret = read(xenconsoled_fd, 
>> >> &xenconsoled_buf[xenconsoled_prod], XENCONSOLED_BUF_SIZE - 
>> >> xenconsoled_prod);
>> >> Â Â Â Â Â Â Âif (ret < 0 && errno != EIO && errno != EAGAIN)
>> >> Â Â Â Â Â Â Â Â Âgoto out_err;
>> >> @@ -230,6 +259,12 @@ static char * bootloader_interact(libxl_
>> >>
>> >> Â Â Â Â Â/* Input from bootloader, read bootloader_fd, write 
>> >> xenconsoled_fd */
>> >> Â Â Â Â Âif (FD_ISSET(bootloader_fd, &rsel)) {
>> >> + Â Â Â Â Â Âif (bootloader_prod == BOOTLOADER_BUF_SIZE) {
>> >> + Â Â Â Â Â Â Â Âif (ioctl(bootloader_fd, FIONREAD, &read_ahead) < 0)
>> >> + Â Â Â Â Â Â Â Â Â Âgoto out_err;
>> >> + Â Â Â Â Â Â Â Âmemmove(bootloader_buf, &bootloader_buf[read_ahead], 
>> >> BOOTLOADER_BUF_SIZE - read_ahead);
>> >> + Â Â Â Â Â Â Â Âbootloader_prod -= read_ahead;
>> >> + Â Â Â Â Â Â}
>> >> Â Â Â Â Â Â Âret = read(bootloader_fd, &bootloader_buf[bootloader_prod], 
>> >> BOOTLOADER_BUF_SIZE - bootloader_prod);
>> >> Â Â Â Â Â Â Âif (ret < 0 && errno != EIO && errno != EAGAIN)
>> >> Â Â Â Â Â Â Â Â Âgoto out_err;
>> >
>> >
>> >
>>
>> I'm wrapping another patch which I will send right now.
>
>
>

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel