lkml.org 
[lkml]   [2010]   [Apr]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [Patch 3/3] net: reserve ports for applications using fixed port numbers
    Tetsuo Handa wrote:
    > Hello.
    >
    > Amerigo Wang wrote:
    >> Index: linux-2.6/drivers/infiniband/core/cma.c
    >> ===================================================================
    >> --- linux-2.6.orig/drivers/infiniband/core/cma.c
    >> +++ linux-2.6/drivers/infiniband/core/cma.c
    >> @@ -1980,6 +1980,8 @@ retry:
    >> /* FIXME: add proper port randomization per like inet_csk_get_port */
    >> do {
    >> ret = idr_get_new_above(ps, bind_list, next_port, &port);
    >> + if (inet_is_reserved_local_port(port))
    >> + ret = -EAGAIN;
    >
    > You should not overwrite ret with -EAGAIN when idr_get_new_above() returned
    > -ENOSPC. I don't know about idr, thus I don't know whether
    >
    > if (!ret && inet_is_reserved_local_port(port))
    > ret = -EAGAIN;
    >
    > is correct or not.

    Hmm, good catch! I think it is correct.



    >
    >> } while ((ret == -EAGAIN) && idr_pre_get(ps, GFP_KERNEL));
    >>
    >> if (ret)
    >> @@ -2996,10 +2998,13 @@ static int __init cma_init(void)
    >> {
    >> int ret, low, high, remaining;
    >>
    >> - get_random_bytes(&next_port, sizeof next_port);
    >> inet_get_local_port_range(&low, &high);
    >> +again:
    >> + get_random_bytes(&next_port, sizeof next_port);
    >> remaining = (high - low) + 1;
    >> next_port = ((unsigned int) next_port % remaining) + low;
    >> + if (inet_is_reserved_local_port(next_port))
    >> + goto again;
    >>
    >
    > You should not unconditionally "goto again;".
    > If all ports were reserved, it will loop forever (CPU stalls).
    >

    Yeah, how about:

    int tries = 10;
    ...
    again:
    ...
    if (inet_is_reserved_local_port(next_port)) {
    if (tries--)
    goto again;
    else
    return -EBUSY;
    }

    ?


    >> cma_wq = create_singlethread_workqueue("rdma_cm");
    >> if (!cma_wq)
    >
    >
    >> Index: linux-2.6/net/sctp/socket.c
    >> ===================================================================
    >> --- linux-2.6.orig/net/sctp/socket.c
    >> +++ linux-2.6/net/sctp/socket.c
    >> @@ -5436,6 +5436,8 @@ static long sctp_get_port_local(struct s
    >> rover++;
    >> if ((rover < low) || (rover > high))
    >> rover = low;
    >> + if (inet_is_reserved_local_port(rover))
    >> + continue;
    >
    > This one needs to be
    >
    > if (inet_is_reserved_local_port(rover))
    > goto next_nolock;
    >
    >> index = sctp_phashfn(rover);
    >> head = &sctp_port_hashtable[index];
    >> sctp_spin_lock(&head->lock);
    >
    > next:
    > sctp_spin_unlock(&head->lock);
    > +next_nolock:
    > } while (--remaining > 0);
    >
    > otherwise, it will loop forever if all ports were reserved.

    Sorry, doesn't 'continue' jump to exactly where 'next_nolock' is??
    Or I am missing something?


    Thanks for your review!


    \
     
     \ /
      Last update: 2010-04-12 08:51    [W:0.026 / U:0.852 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site