[kernel][ports] Add basic ports functionality

[kernel][ports] Add basic ports functionality

Missing mostly more tests, specifically group tests.

BUG=none
R=travisg@google.com

Committed: 614eaf166a2386cf7a3f5fc98a9f683b3b5b325e

[cpu_(ooo_6.6-7.5), 2015-11-09 03:50:40]

Description was changed from

==========
[kernel][ports] Add basic ports functionality

Port creation, reading and writting on this CL.

Missing:
- port destruction
- buffer management
- debug / introspection
- tests

BUG=
==========

to

==========
[kernel][ports] Add basic ports functionality

Port creation, reading and writting on this CL.

Missing:
- port destruction
- buffer management
- debug / introspection
- tests

BUG=
==========

[cpu_(ooo_6.6-7.5), 2015-11-09 03:50:40]

cpu@chromium.org changed reviewers:
+ travisg@google.com

[cpu_(ooo_6.6-7.5), 2015-11-09 03:54:30]

I tried to make the smallest initial CL that had the basic mechanism in place.
Sorry for 300 lines ...

The other code in kernel/ is terse, I can remove the comments to match the style
of the other code. For now they might help.

I tried to make an effort to not do memory allocation inside the locks and to
only allocate during object creation.

To note:
two threads can write to the same write_port

two threads can read from the same read_port however only one gets the message.

[travisg, 2015-11-09 23:14:16]

Good start. A few little nits here and some formatting tweaks (can be run
through ./scripts/codestyle.space to match the rest of the kernel easily.

https://codereview.chromium.org/1437453002/diff/1/include/kernel/port.h
File include/kernel/port.h (right):

https://codereview.chromium.org/1437453002/diff/1/include/kernel/port.h#newco...
include/kernel/port.h:51: port_mode_broadcast   = 0,
make these ALL_CAPS to make it a bit easier to tell constants from variables.

https://codereview.chromium.org/1437453002/diff/1/kernel/port.c
File kernel/port.c (right):

https://codereview.chromium.org/1437453002/diff/1/kernel/port.c#newcode59
kernel/port.c:59: char name[PORT_NAME_LEN];
should probably put name last, to make sure thumb code can properly use a small
immediate offset into the struct, plus as a general style thing.

https://codereview.chromium.org/1437453002/diff/1/kernel/port.c#newcode91
kernel/port.c:91: return NULL;
weird indent here. Is there a mixture of spaces and tabs in this file? For all
new stuff, use 4 space indents across the file.

https://codereview.chromium.org/1437453002/diff/1/kernel/port.c#newcode116
kernel/port.c:116: {
test for !name and !port and return ERR_INVALID_ARGS

https://codereview.chromium.org/1437453002/diff/1/kernel/port.c#newcode146
kernel/port.c:146: wp->buf = make_buf(size);
test and unroll wp->buf == NULL here.

https://codereview.chromium.org/1437453002/diff/1/kernel/port.c#newcode149
kernel/port.c:149: list_add_tail(&write_port_list, &wp->node);
add comment that there's a bit of a race here and another write port could be
created with the same name.

https://codereview.chromium.org/1437453002/diff/1/kernel/port.c#newcode168
kernel/port.c:168: port_buf_t* buf = make_buf(PORT_BUFF_SIZE);
test for !buf

https://codereview.chromium.org/1437453002/diff/1/kernel/port.c#newcode277
kernel/port.c:277: status_t rc = wait_queue_block(&rp->wait, timeout);
should probably test for timeout == 0 and not bother calling. since this is
inlined and code below calls it with 0 hard coded, it wont emit the entire call
at all.

[cpu_(ooo_6.6-7.5), 2015-11-11 00:31:13]

thanks, I acted on all comments and redid a bit the logic for failure cases, to
properly undo changes.

https://codereview.chromium.org/1437453002/diff/1/include/kernel/port.h
File include/kernel/port.h (right):

https://codereview.chromium.org/1437453002/diff/1/include/kernel/port.h#newco...
include/kernel/port.h:51: port_mode_broadcast   = 0,
On 2015/11/09 23:14:16, travisg wrote:
> make these ALL_CAPS to make it a bit easier to tell constants from variables.

Done.

https://codereview.chromium.org/1437453002/diff/1/kernel/port.c
File kernel/port.c (right):

https://codereview.chromium.org/1437453002/diff/1/kernel/port.c#newcode59
kernel/port.c:59: char name[PORT_NAME_LEN];
On 2015/11/09 23:14:16, travisg wrote:
> should probably put name last, to make sure thumb code can properly use a
small
> immediate offset into the struct, plus as a general style thing.

Done.

https://codereview.chromium.org/1437453002/diff/1/kernel/port.c#newcode91
kernel/port.c:91: return NULL;
On 2015/11/09 23:14:16, travisg wrote:
> weird indent here. Is there a mixture of spaces and tabs in this file? For all
> new stuff, use 4 space indents across the file.

Done.

https://codereview.chromium.org/1437453002/diff/1/kernel/port.c#newcode116
kernel/port.c:116: {
On 2015/11/09 23:14:16, travisg wrote:
> test for !name and !port and return ERR_INVALID_ARGS

Done.

https://codereview.chromium.org/1437453002/diff/1/kernel/port.c#newcode146
kernel/port.c:146: wp->buf = make_buf(size);
On 2015/11/09 23:14:16, travisg wrote:
> test and unroll wp->buf == NULL here.

Done.

https://codereview.chromium.org/1437453002/diff/1/kernel/port.c#newcode149
kernel/port.c:149: list_add_tail(&write_port_list, &wp->node);
On 2015/11/09 23:14:16, travisg wrote:
> add comment that there's a bit of a race here and another write port could be
> created with the same name.

good catch!  I'll do that now and later on I'll fix it.

https://codereview.chromium.org/1437453002/diff/1/kernel/port.c#newcode277
kernel/port.c:277: status_t rc = wait_queue_block(&rp->wait, timeout);
On 2015/11/09 23:14:16, travisg wrote:
> should probably test for timeout == 0 and not bother calling. since this is
> inlined and code below calls it with 0 hard coded, it wont emit the entire
call
> at all.

Done.

[cpu_(ooo_6.6-7.5), 2015-11-11 00:36:31]

https://codereview.chromium.org/1437453002/diff/20001/kernel/port.c
File kernel/port.c (right):

https://codereview.chromium.org/1437453002/diff/20001/kernel/port.c#newcode191
kernel/port.c:191: // found; add read port to write port list.
note that the list_add_tail was not unrolled previously for failure the failure
case.

https://codereview.chromium.org/1437453002/diff/20001/kernel/port.c#newcode206
kernel/port.c:206: // |buf| allocation failed and the buffer was needed.
note the test asked for line 182 is done in line 199

https://codereview.chromium.org/1437453002/diff/20001/kernel/port.c#newcode256
kernel/port.c:256: 
redid this so there are less unlocks and to match the style of port_open, 
technically this is an 'open' function.

[cpu_(ooo_6.6-7.5), 2015-11-15 21:49:20]

patch 3 adds circular buffer impl. and top/main integration. I am not sure what
is the best place to init the global list of write ports.

[cpu_(ooo_6.6-7.5), 2015-11-15 23:00:20]

Patchset #3 (id:40001) has been deleted

[cpu_(ooo_6.6-7.5), 2015-11-17 03:00:16]

patch 4 is the whole thing minus full tests. 

please take a look.

[travisg, 2015-11-19 21:26:11]

A few tweaks and one actual sort of problem, but nothing that can't be fixed.

https://codereview.chromium.org/1437453002/diff/100001/kernel/port.c
File kernel/port.c (right):

https://codereview.chromium.org/1437453002/diff/100001/kernel/port.c#newcode130
kernel/port.c:130: list_initialize(&write_port_list);
if you want, can statically initialize the list with
write_port_list = LIST_INITIAL_VALUE(write_port_list);

would remove the need for the init call.

https://codereview.chromium.org/1437453002/diff/100001/kernel/port.c#newcode171
kernel/port.c:171: strlcpy(wp->name, name, sizeof(wp->name));
to avoid a problem here, might want to strlen name at the top to make sure it
wont overrun the limited port name and return ERR_INVALID_ARGS if it's too long.

https://codereview.chromium.org/1437453002/diff/100001/kernel/port.c#newcode174
kernel/port.c:174: uint size = mode & PORT_MODE_BIG_BUFFER ?  PORT_BUFF_SIZE_BIG
: PORT_BUFF_SIZE;
order of ops here? I always forget but the ternary operator screws me enough
that i always () the first element.

https://codereview.chromium.org/1437453002/diff/100001/kernel/port.c#newcode201
kernel/port.c:201: memset(rp, 0, sizeof(read_port_t));
can use calloc for slightly smaller code

https://codereview.chromium.org/1437453002/diff/100001/kernel/port.c#newcode209
kernel/port.c:209: port_buf_t* buf = make_buf(PORT_BUFF_SIZE);
check for null on buf

https://codereview.chromium.org/1437453002/diff/100001/kernel/port.c#newcode259
kernel/port.c:259: {
check for !ports, !group, and count is some reasonable value

https://codereview.chromium.org/1437453002/diff/100001/kernel/port.c#newcode265
kernel/port.c:265: memset(pg, 0, sizeof(port_group_t));
same re: calloc

https://codereview.chromium.org/1437453002/diff/100001/kernel/port.c#newcode299
kernel/port.c:299: {
check for !pk and !port

https://codereview.chromium.org/1437453002/diff/100001/kernel/port.c#newcode326
kernel/port.c:326: awake_count = wait_queue_wake_one(&rp->gport->wait,
RESCHEDULE_POLICY, NO_ERROR);
potential hazard: the new thread will wake up and modify the rp_list, thus
causing the next iteration of the loop to go off in the weeds. May want to delay
rescheduling till the end of the loop (would be more efficient anyway). Can call
thread_yield() after the lock is released, or if want to get fancy, directly
stick the current thread in the run queue and call thread_resched(). Essentially
the guts of thread_preempt().

https://codereview.chromium.org/1437453002/diff/100001/kernel/port.c#newcode358
kernel/port.c:358: {
test for !port and !result

https://codereview.chromium.org/1437453002/diff/100001/kernel/port.c#newcode391
kernel/port.c:391: {
test for !port

https://codereview.chromium.org/1437453002/diff/100001/kernel/port.c#newcode414
kernel/port.c:414: wait_queue_wake_all(&rp->gport->wait, false, ERR_CANCELLED);
same resched stuff as before. want to not reschedule here (which is what you
did), but then resched at the end of the function if anything got released.

https://codereview.chromium.org/1437453002/diff/100001/kernel/port.c#newcode430
kernel/port.c:430: {
test for !port

https://codereview.chromium.org/1437453002/diff/100001/top/main.c
File top/main.c (right):

https://codereview.chromium.org/1437453002/diff/100001/top/main.c#newcode121
top/main.c:121: port_init();
Should probably just move this inside kernel_init()

[cpu_(ooo_6.6-7.5), 2015-11-20 22:18:46]

changes applied ptal

https://codereview.chromium.org/1437453002/diff/100001/kernel/port.c
File kernel/port.c (right):

https://codereview.chromium.org/1437453002/diff/100001/kernel/port.c#newcode130
kernel/port.c:130: list_initialize(&write_port_list);
On 2015/11/19 21:26:10, travisg wrote:
> if you want, can statically initialize the list with
> write_port_list = LIST_INITIAL_VALUE(write_port_list);
> 
> would remove the need for the init call.

I am a bit afraid of the ordering in global statics. I think we should
let this here.

https://codereview.chromium.org/1437453002/diff/100001/kernel/port.c#newcode171
kernel/port.c:171: strlcpy(wp->name, name, sizeof(wp->name));
On 2015/11/19 21:26:10, travisg wrote:
> to avoid a problem here, might want to strlen name at the top to make sure it
> wont overrun the limited port name and return ERR_INVALID_ARGS if it's too
long.

Done.

https://codereview.chromium.org/1437453002/diff/100001/kernel/port.c#newcode174
kernel/port.c:174: uint size = mode & PORT_MODE_BIG_BUFFER ?  PORT_BUFF_SIZE_BIG
: PORT_BUFF_SIZE;
On 2015/11/19 21:26:10, travisg wrote:
> order of ops here? I always forget but the ternary operator screws me enough
> that i always () the first element.

Done. I usually do put (), kind of surprised I forgot.

https://codereview.chromium.org/1437453002/diff/100001/kernel/port.c#newcode201
kernel/port.c:201: memset(rp, 0, sizeof(read_port_t));
On 2015/11/19 21:26:10, travisg wrote:
> can use calloc for slightly smaller code

Done.

https://codereview.chromium.org/1437453002/diff/100001/kernel/port.c#newcode209
kernel/port.c:209: port_buf_t* buf = make_buf(PORT_BUFF_SIZE);
On 2015/11/19 21:26:10, travisg wrote:
> check for null on buf

Done.

https://codereview.chromium.org/1437453002/diff/100001/kernel/port.c#newcode259
kernel/port.c:259: {
On 2015/11/19 21:26:10, travisg wrote:
> check for !ports, !group, and count is some reasonable value

Done.

https://codereview.chromium.org/1437453002/diff/100001/kernel/port.c#newcode265
kernel/port.c:265: memset(pg, 0, sizeof(port_group_t));
On 2015/11/19 21:26:10, travisg wrote:
> same re: calloc

done for all.

https://codereview.chromium.org/1437453002/diff/100001/kernel/port.c#newcode299
kernel/port.c:299: {
On 2015/11/19 21:26:10, travisg wrote:
> check for !pk and !port

Done.

https://codereview.chromium.org/1437453002/diff/100001/kernel/port.c#newcode326
kernel/port.c:326: awake_count = wait_queue_wake_one(&rp->gport->wait,
RESCHEDULE_POLICY, NO_ERROR);
On 2015/11/19 21:26:10, travisg wrote:
> potential hazard: the new thread will wake up and modify the rp_list, thus
> causing the next iteration of the loop to go off in the weeds. May want to
delay
> rescheduling till the end of the loop (would be more efficient anyway). Can
call
> thread_yield() after the lock is released, or if want to get fancy, directly
> stick the current thread in the run queue and call thread_resched().
Essentially
> the guts of thread_preempt().

Done via yield()

https://codereview.chromium.org/1437453002/diff/100001/kernel/port.c#newcode358
kernel/port.c:358: {
On 2015/11/19 21:26:10, travisg wrote:
> test for !port and !result

Done.

https://codereview.chromium.org/1437453002/diff/100001/kernel/port.c#newcode391
kernel/port.c:391: {
On 2015/11/19 21:26:10, travisg wrote:
> test for !port

Done.

https://codereview.chromium.org/1437453002/diff/100001/kernel/port.c#newcode414
kernel/port.c:414: wait_queue_wake_all(&rp->gport->wait, false, ERR_CANCELLED);
On 2015/11/19 21:26:10, travisg wrote:
> same resched stuff as before. want to not reschedule here (which is what you
> did), but then resched at the end of the function if anything got released.

Acknowledged.

https://codereview.chromium.org/1437453002/diff/100001/kernel/port.c#newcode430
kernel/port.c:430: {
On 2015/11/19 21:26:10, travisg wrote:
> test for !port

Done.

https://codereview.chromium.org/1437453002/diff/100001/top/main.c
File top/main.c (right):

https://codereview.chromium.org/1437453002/diff/100001/top/main.c#newcode121
top/main.c:121: port_init();
On 2015/11/19 21:26:10, travisg wrote:
> Should probably just move this inside kernel_init()

Done.

[travisg, 2015-11-20 22:24:24]

LGTM other than the few nits

https://codereview.chromium.org/1437453002/diff/100001/kernel/port.c
File kernel/port.c (right):

https://codereview.chromium.org/1437453002/diff/100001/kernel/port.c#newcode130
kernel/port.c:130: list_initialize(&write_port_list);
On 2015/11/20 22:18:46, cpu wrote:
> On 2015/11/19 21:26:10, travisg wrote:
> > if you want, can statically initialize the list with
> > write_port_list = LIST_INITIAL_VALUE(write_port_list);
> > 
> > would remove the need for the init call.
> 
> I am a bit afraid of the ordering in global statics. I think we should
> let this here.

They're not ordered. It is simply pre-initialized in the data segment. But can
wait another day.

https://codereview.chromium.org/1437453002/diff/120001/kernel/port.c
File kernel/port.c (right):

https://codereview.chromium.org/1437453002/diff/120001/kernel/port.c#newcode146
kernel/port.c:146: if (strlen(name) > PORT_NAME_LEN)
 - 1 or >= perhaps due to null terminator?

[cpu_(ooo_6.6-7.5), 2015-11-20 22:40:31]

Description was changed from

==========
[kernel][ports] Add basic ports functionality

Port creation, reading and writting on this CL.

Missing:
- port destruction
- buffer management
- debug / introspection
- tests

BUG=
==========

to

==========
[kernel][ports] Add basic ports functionality

Missing mostly more tests, specifically group tests.

BUG=none
==========

[cpu_(ooo_6.6-7.5), 2015-11-20 22:41:26]

done. after I make sure it builds in all I'll commmit

[cpu_(ooo_6.6-7.5), 2015-11-20 22:43:42]

Committed patchset #7 (id:140001) manually as
614eaf166a2386cf7a3f5fc98a9f683b3b5b325e (presubmit successful).