lkml.org 
[lkml]   [2007]   [Sep]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2] iw_cxgb3: Support "iwarp-only" interfaces to avoid 4-tuple conflicts.
    Hi Steve.

    On Sat, Sep 15, 2007 at 10:56:46AM -0500, Steve Wise (swise@opengridcomputing.com) wrote:
    > >>The iWARP driver must translate all listens on address 0.0.0.0 to the
    > >>set of rdma-only ip addresses for the device in question. This prevents
    > >>incoming connect requests to the TCP ipaddresses from going up the
    > >>rdma stack.
    > >
    > >If the only solutions to solve a problem with hardware are to steal
    > >packets or became a real device, then real device is much more
    > >appropriate. Is that correct?
    > >
    >
    > This is a real device. I don't understand your question? Packets
    > aren't being stolen.

    I meant port from main network stack. Sorry for confusion.

    > >>+static void insert_ifa(struct iwch_dev *rnicp, struct in_ifaddr *ifa)
    > >>+{
    > >>+ struct iwch_addrlist *addr;
    > >>+
    > >>+ addr = kmalloc(sizeof *addr, GFP_KERNEL);
    > >
    > >As a small nitpick: this wants to be sizeof(struct in_ifaddr)
    > >
    >
    > No, insert_ifa() allocates a struct iwch_addrlist, which has 2 fields: a
    > list_head for linking, and a struct in_ifaddr pointer.

    sizeof(struct iwch_addrlist) of course, not (*addr).
    To simplify grep.

    > >>+ if (!addr) {
    > >>+ printk(KERN_ERR MOD "%s - failed to alloc memory!\n",
    > >>+ __FUNCTION__);
    > >>+ return;
    > >>+ }
    > >>+ addr->ifa = ifa;
    > >>+ mutex_lock(&rnicp->mutex);
    > >>+ list_add_tail(&addr->entry, &rnicp->addrlist);
    > >>+ mutex_unlock(&rnicp->mutex);
    > >>+}
    > >
    > >What about providing error back to caller and fail to register?
    > >
    >
    > There are two causes where this is called: 1) during module init to
    > populate the list of iwarp addresses. If we failed in that case then, I
    > _could_ then not register. 2) we get called via the notifier mechanism
    > when an address is added. If that fails, the caller doesn't care (since
    > we're on the notifier callout thread). But the code could perhaps
    > unregister the device. I prefer just logging an error in case 2. I'll
    > look into not registering if we cannot get any address due to lack of
    > memory. But there's another case: we load the module and the admin
    > hasn't yet created the ethX:iw interface.
    >
    > Perhaps I should change the code to only register as a working rdma
    > device _when_ we get at least one ethX:iwY interface created? Whatchathink?

    Does second case ends up with problem you described in the initial
    e-mail not being fixed?

    > >>+static inline int is_iwarp_label(char *label)
    > >>+{
    > >>+ char *colon;
    > >>+
    > >>+ colon = strchr(label, ':');
    > >>+ if (colon && !strncmp(colon+1, "iw", 2))
    > >>+ return 1;
    > >>+ return 0;
    > >>+}
    > >
    > >I.e. it is not allowed to create ':iw' alias for anyone else?
    > >Well, looks crappy, but if it is the only solution...
    > >
    >
    > It is kinda crappy. But I don't see a better solution. Any ideas?

    Does creating the whole new netdevice is a too big overhead, or is it
    considered bad idea?

    > >>+static struct iwch_listen_entry *alloc_listener(struct iwch_listen_ep
    > >>*ep,
    > >>+ __be32 addr)
    > >
    > >Do you know, that cxgb3 function names suck? :)
    > >Especially get_skb().
    > >
    > >>+{
    > >>+ struct iwch_dev *h = to_iwch_dev(ep->com.cm_id->device);
    > >>+ struct iwch_listen_entry *le;
    > >>+
    > >>+ le = kmalloc(sizeof *le, GFP_KERNEL);
    > >
    > >Wants to be sizeof(struct iwch_listen_entry) and in other places too.
    > >
    >
    > Do you mean I shouldn't use sizeof *le, but rather sizeof(struct
    > iwch_listen_entry)? Is that the preferred coding style?

    Yes, exactly.

    > >I skipped rdma internals of the patch, since I do not know it enough
    > >to judge, but your approach looks good from core network point of view.
    > >Maybe you should automatically create an alias each time new interface
    > >is added so that admin would not care about proper aliases?
    > >
    >
    > That would be much better IMO, but the problem is that I cannot create
    > an alias without an actual ip address. Unless we change the core
    > services to allow it.
    >
    > Thanks for reviewing!
    >
    > Steve.
    >

    --
    Evgeniy Polyakov
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2007-09-16 16:25    [W:0.029 / U:33.084 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site