lkml.org 
[lkml]   [2015]   [Jul]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
From
SubjectRE: [PATCH V3 3/7] Drivers: hv: vmbus: add APIs to send/recv hvsock packet and get the r/w-ability
Date
> From: Vitaly Kuznetsov
> Sent: Tuesday, July 21, 2015 22:07
> > diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
>> ...
> > +int vmbus_recvpacket_hvsock(struct vmbus_channel *channel, void *buffer,
> > + u32 bufferlen, u32 *buffer_actual_len)
> > +{
> > + struct vmpipe_proto_header *pipe_hdr;
> > + struct vmpacket_descriptor *desc;
> > + u32 packet_len, payload_len;
> > + bool signal = false;
> > + int ret;
> > +
> > + *buffer_actual_len = 0;
> > +
> > + if (bufferlen < HVSOCK_HEADER_LEN)
> > + return -ENOBUFS;
> > +
> > + ret = hv_ringbuffer_peek(&channel->inbound, buffer,
> > + HVSOCK_HEADER_LEN);
> > + if (ret != 0)
> > + return 0;
>
> I'd suggest you do something like
>
> if (ret == -EAGIAIN)
> return 0;
> else if (ret)
> return ret;
>
> to make it future-proof (e.g. when a new error is returned by
> hv_ringbuffer_peek). And a comment would also be useful as it is unclear
> why we silence errors here.
Hi Vitaly,
Thanks!
I think I made a mistake here:
the "if (ret != 0) return 0;" should be changed
to "if (ret != 0) return ret;"

vmbus_recvpacket_hvsock() is only invoked in hvsock_recvmsg() in the
case of can_read == true && need_refill == true, which means
the bytes-available-to-read in the ringbuffer is bigger than
HVSOCK_HEADER_LEN, so here hv_ringbuffer_peek() can only return 0.

I'll fix this in V4.

> > +void vmbus_get_hvsock_rw_status(struct vmbus_channel *channel,
> > + bool *can_read, bool *can_write)
> > +{
> > + u32 avl_read_bytes, avl_write_bytes, dummy;
> > +
> > + if (can_read != NULL) {
> > + hv_get_ringbuffer_available_space(&channel->inbound,
> > + &avl_read_bytes,
> > + &dummy);
> > + *can_read = avl_read_bytes >= HVSOCK_MIN_PKT_LEN;
> > + }
> > +
> > + /* We write into the ringbuffer only when we're able to write a
>
> Not sure why checkpatch.pl doesn't complain but according to the
> CodingStyle multi-line comments outside of drivers/net and net/ are
> supposed to start with and empty '/*' line.
Yeah, you're correct. I'll fix this in V4.

BTW, it seems the rule is not strictly obeyed: :-)
kernel/sched/fair.c:7290: /* don't kick the active_load_balance_cpu_stop,
kernel/pid.c:273: /* When all that is left in the pid namespace
kernel/watchdog.c:293: /* check for a hardlockup e
kernel/signal.c:2376: /* A signal was successfully delivered, and the

> > diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> > @@ -885,6 +885,9 @@ extern int vmbus_sendpacket_ctl(struct
> vmbus_channel *channel,
> > u32 flags,
> > bool kick_q);
> >
> > +extern int vmbus_sendpacket_hvsock(struct vmbus_channel *channel,
> > + void *buf, u32 len);
> > +
>
> I *think* if your argument list makes it to the second line it is supposed
> to be alligned with the first one (e.g. 'void' should start at the same
> position as 'struct' on the previous line).

I think it's indeed aligned in the patch, i.e., here the 's' and the 'v' are at the
same column.

If I use Vim's ":set list" command, it will like like

extern int vmbus_sendpacket_hvsock(struct vmbus_channel *channel,$
^I^I^I^I void *buf, u32 len);$

Here I use 4 Tabs and 3 white spaces to make sure the 's' and the 'v' are at
the same column.
In the patch, due to the leading '+', they doesn't look like at the same column.

Please let me know if I'm not using the correct aligning method.
To be frank, I might depend too much on scripts/checkpatch.pl, which didn't
find obvious issues in the patch. :-)

> > extern int vmbus_sendpacket_pagebuffer(struct vmbus_channel *channel,
> > struct hv_page_buffer pagebuffers[],
> > u32 pagecount,
> > @@ -934,6 +937,11 @@ extern int vmbus_recvpacket_raw(struct
> vmbus_channel *channel,
> > u32 *buffer_actual_len,
> > u64 *requestid);
> >
> > +extern int vmbus_recvpacket_hvsock(struct vmbus_channel *channel, void
> *buffer,
> > + u32 bufferlen, u32 *buffer_actual_len);
> > +
>
> the same.
ditto

> > +extern void vmbus_get_hvsock_rw_status(struct vmbus_channel *channel,
> > + bool *can_read, bool *can_write);
> >
>
> the same.
ditto.

Thanks,
-- Dexuan


\
 
 \ /
  Last update: 2015-07-22 12:41    [W:0.113 / U:0.032 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site