* RE: [ofa-general] [PATCH v3] iw_cxgb3: Support "iwarp-only" interfacesto avoid 4-tuple conflicts.
From: Kanevsky, Arkady @ 2007-09-27 18:56 UTC (permalink / raw)
To: Sean Hefty, Steve Wise; +Cc: netdev, rdreier, linux-kernel, general
In-Reply-To: <46FBF8AF.9040700@ichips.intel.com>
Sean,
What is the model on how client connects, say for iSCSI,
when client and server both support, iWARP and 10GbE or 1GbE,
and would like to setup "most" performant "connection" for ULP?
Thanks,
Arkady Kanevsky email: arkady@netapp.com
Network Appliance Inc. phone: 781-768-5395
1601 Trapelo Rd. - Suite 16. Fax: 781-895-1195
Waltham, MA 02451 central phone: 781-768-5300
> -----Original Message-----
> From: Sean Hefty [mailto:mshefty@ichips.intel.com]
> Sent: Thursday, September 27, 2007 2:39 PM
> To: Steve Wise
> Cc: netdev@vger.kernel.org; rdreier@cisco.com;
> general@lists.openfabrics.org; linux-kernel@vger.kernel.org
> Subject: Re: [ofa-general] [PATCH v3] iw_cxgb3: Support
> "iwarp-only" interfacesto avoid 4-tuple conflicts.
>
> > The sysadmin creates "for iwarp use only" alias interfaces
> of the form
> > "devname:iw*" where devname is the native interface name
> (eg eth0) for
> > the iwarp netdev device. The alias label can be anything
> starting with "iw".
> > The "iw" immediately after the ':' is the key used by the
> iw_cxgb3 driver.
>
> I'm still not sure about this, but haven't come up with
> anything better myself. And if there's a good chance of
> other rnic's needing the same support, I'd rather see the
> common code separated out, even if just encapsulated within
> this module for easy re-use.
>
> As for the code, I have a couple of questions about whether
> deadlock and a race condition are possible, plus a few minor comments.
>
> > +static void insert_ifa(struct iwch_dev *rnicp, struct
> in_ifaddr *ifa)
> > +{
> > + struct iwch_addrlist *addr;
> > +
> > + addr = kmalloc(sizeof *addr, GFP_KERNEL);
> > + 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);
> > +}
>
> Should this return success/failure?
>
> > +static int nb_callback(struct notifier_block *self,
> unsigned long event,
> > + void *ctx)
> > +{
> > + struct in_ifaddr *ifa = ctx;
> > + struct iwch_dev *rnicp = container_of(self, struct
> iwch_dev, nb);
> > +
> > + PDBG("%s rnicp %p event %lx\n", __FUNCTION__, rnicp, event);
> > +
> > + switch (event) {
> > + case NETDEV_UP:
> > + if (netdev_is_ours(rnicp, ifa->ifa_dev->dev) &&
> > + is_iwarp_label(ifa->ifa_label)) {
> > + PDBG("%s label %s addr 0x%x added\n",
> > + __FUNCTION__, ifa->ifa_label,
> ifa->ifa_address);
> > + insert_ifa(rnicp, ifa);
> > + iwch_listeners_add_addr(rnicp,
> ifa->ifa_address);
>
> If insert_ifa() fails, what will iwch_listeners_add_addr()
> do? (I'm not easily seeing the relationship between the
> address list and the listen list at this point.)
>
> > + }
> > + break;
> > + case NETDEV_DOWN:
> > + if (netdev_is_ours(rnicp, ifa->ifa_dev->dev) &&
> > + is_iwarp_label(ifa->ifa_label)) {
> > + PDBG("%s label %s addr 0x%x deleted\n",
> > + __FUNCTION__, ifa->ifa_label,
> ifa->ifa_address);
> > + iwch_listeners_del_addr(rnicp,
> ifa->ifa_address);
> > + remove_ifa(rnicp, ifa);
> > + }
> > + break;
> > + default:
> > + break;
> > + }
> > + return 0;
> > +}
> > +
> > +static void delete_addrlist(struct iwch_dev *rnicp) {
> > + struct iwch_addrlist *addr, *tmp;
> > +
> > + mutex_lock(&rnicp->mutex);
> > + list_for_each_entry_safe(addr, tmp, &rnicp->addrlist, entry) {
> > + list_del(&addr->entry);
> > + kfree(addr);
> > + }
> > + mutex_unlock(&rnicp->mutex);
> > +}
> > +
> > +static void populate_addrlist(struct iwch_dev *rnicp) {
> > + int i;
> > + struct in_device *indev;
> > +
> > + for (i = 0; i < rnicp->rdev.port_info.nports; i++) {
> > + indev = in_dev_get(rnicp->rdev.port_info.lldevs[i]);
> > + if (!indev)
> > + continue;
> > + for_ifa(indev)
> > + if (is_iwarp_label(ifa->ifa_label)) {
> > + PDBG("%s label %s addr 0x%x added\n",
> > + __FUNCTION__, ifa->ifa_label,
> > + ifa->ifa_address);
> > + insert_ifa(rnicp, ifa);
> > + }
> > + endfor_ifa(indev);
> > + }
> > +}
> > +
> > static void rnic_init(struct iwch_dev *rnicp) {
> > PDBG("%s iwch_dev %p\n", __FUNCTION__, rnicp); @@
> -70,6 +187,12 @@
> > static void rnic_init(struct iwch_dev *r
> > idr_init(&rnicp->qpidr);
> > idr_init(&rnicp->mmidr);
> > spin_lock_init(&rnicp->lock);
> > + INIT_LIST_HEAD(&rnicp->addrlist);
> > + INIT_LIST_HEAD(&rnicp->listen_eps);
> > + mutex_init(&rnicp->mutex);
> > + rnicp->nb.notifier_call = nb_callback;
> > + populate_addrlist(rnicp);
> > + register_inetaddr_notifier(&rnicp->nb);
> >
> > rnicp->attr.vendor_id = 0x168;
> > rnicp->attr.vendor_part_id = 7;
> > @@ -148,6 +271,8 @@ static void close_rnic_dev(struct t3cdev
> > mutex_lock(&dev_mutex);
> > list_for_each_entry_safe(dev, tmp, &dev_list, entry) {
> > if (dev->rdev.t3cdev_p == tdev) {
> > + unregister_inetaddr_notifier(&dev->nb);
> > + delete_addrlist(dev);
> > list_del(&dev->entry);
> > iwch_unregister_device(dev);
> > cxio_rdev_close(&dev->rdev);
> > diff --git a/drivers/infiniband/hw/cxgb3/iwch.h
> > b/drivers/infiniband/hw/cxgb3/iwch.h
> > index caf4e60..7fa0a47 100644
> > --- a/drivers/infiniband/hw/cxgb3/iwch.h
> > +++ b/drivers/infiniband/hw/cxgb3/iwch.h
> > @@ -36,6 +36,8 @@ #include <linux/mutex.h> #include
> <linux/list.h>
> > #include <linux/spinlock.h> #include <linux/idr.h>
> > +#include <linux/notifier.h>
> > +#include <linux/inetdevice.h>
> >
> > #include <rdma/ib_verbs.h>
> >
> > @@ -101,6 +103,11 @@ struct iwch_rnic_attributes {
> > u32 cq_overflow_detection;
> > };
> >
> > +struct iwch_addrlist {
> > + struct list_head entry;
> > + struct in_ifaddr *ifa;
> > +};
> > +
> > struct iwch_dev {
> > struct ib_device ibdev;
> > struct cxio_rdev rdev;
> > @@ -111,6 +118,10 @@ struct iwch_dev {
> > struct idr mmidr;
> > spinlock_t lock;
> > struct list_head entry;
> > + struct notifier_block nb;
> > + struct list_head addrlist;
> > + struct list_head listen_eps;
>
> The behavior of the listen lists is similar to what's done in the
> rdma_cm: Wildcard listens are stored in a listen_any_list.
> When new devices are added, associated listens are added to
> each device. This is similar, except we're dealing with
> devices and addresses. I'm wondering if we shouldn't mimic
> the same behavior and track listens in iwch_addrlist
> directly. (I don't see anything wrong with this approach
> though.)
>
> What happens if an address changes between iwarp only and non-iwarp?
>
> How are listens on specific addresses handled from an rdma_cm level?
> Does the rdma_cm map the address to the device, call the
> iw_cm to listen, which in turn calls the device listen
> function? The device then checks that the address has been
> marked as iwarp only? (I'm being too lazy to trace this
> through the code, but if you don't know off the top of your
> head, I will do that.)
>
> > + struct mutex mutex;
> > };
> >
> > static inline struct iwch_dev *to_iwch_dev(struct
> ib_device *ibdev)
> > diff --git a/drivers/infiniband/hw/cxgb3/iwch_cm.c
> > b/drivers/infiniband/hw/cxgb3/iwch_cm.c
> > index 1cdfcd4..afc8a48 100644
> > --- a/drivers/infiniband/hw/cxgb3/iwch_cm.c
> > +++ b/drivers/infiniband/hw/cxgb3/iwch_cm.c
> > @@ -1127,23 +1127,149 @@ static int act_open_rpl(struct t3cdev *t
> > return CPL_RET_BUF_DONE;
> > }
> >
> > -static int listen_start(struct iwch_listen_ep *ep)
> > +static int wait_for_reply(struct iwch_ep_common *epc) {
> > + PDBG("%s ep %p waiting\n", __FUNCTION__, epc);
> > + wait_event(epc->waitq, epc->rpl_done);
> > + PDBG("%s ep %p done waiting err %d\n", __FUNCTION__,
> epc, epc->rpl_err);
> > + return epc->rpl_err;
> > +}
>
> What thread is being blocked here, and what sets the event?
>
> > +
> > +static struct iwch_listen_entry *alloc_listener(struct
> iwch_listen_ep *ep,
> > + __be32 addr)
> > +{
> > + struct iwch_dev *h = to_iwch_dev(ep->com.cm_id->device);
> > + struct iwch_listen_entry *le;
> > +
> > + le = kmalloc(sizeof *le, GFP_KERNEL);
> > + if (!le) {
> > + printk(KERN_ERR MOD "%s - failed to alloc memory!\n",
> > + __FUNCTION__);
> > + return NULL;
> > + }
> > + le->stid = cxgb3_alloc_stid(h->rdev.t3cdev_p,
> > + &t3c_client, ep);
> > + if (le->stid == -1) {
> > + printk(KERN_ERR MOD "%s - cannot alloc stid.\n",
> > + __FUNCTION__);
> > + kfree(le);
> > + return NULL;
> > + }
> > + le->addr = addr;
> > + PDBG("%s stid %u addr %x port %x\n", __FUNCTION__, le->stid,
> > + ntohl(le->addr), ntohs(ep->com.local_addr.sin_port));
> > + return le;
> > +}
> > +
> > +static void dealloc_listener(struct iwch_listen_ep *ep,
> > + struct iwch_listen_entry *le) {
> > + PDBG("%s stid %u addr %x port %x\n", __FUNCTION__, le->stid,
> > + ntohl(le->addr), ntohs(ep->com.local_addr.sin_port));
> > + cxgb3_free_stid(ep->com.tdev, le->stid);
> > + kfree(le);
> > +}
> > +
> > +static void dealloc_listener_list(struct iwch_listen_ep *ep) {
> > + struct iwch_listen_entry *le, *tmp;
> > + struct iwch_dev *h = to_iwch_dev(ep->com.cm_id->device);
> > +
> > + mutex_lock(&h->mutex);
> > + list_for_each_entry_safe(le, tmp, &ep->listeners, entry) {
> > + list_del(&le->entry);
> > + dealloc_listener(ep, le);
> > + }
> > + mutex_unlock(&h->mutex);
> > +}
> > +
> > +static int alloc_listener_list(struct iwch_listen_ep *ep) {
> > + struct iwch_dev *h = to_iwch_dev(ep->com.cm_id->device);
>
> nit: in place of 'h' in several places, how about 'rnicp',
> which is also used?
>
> > + struct iwch_addrlist *addr;
> > + struct iwch_listen_entry *le;
> > + int err = 0;
> > + int added=0;
> > + mutex_lock(&h->mutex);
> > + list_for_each_entry(addr, &h->addrlist, entry) {
> > + if (ep->com.local_addr.sin_addr.s_addr == 0 ||
> > + ep->com.local_addr.sin_addr.s_addr ==
> > + addr->ifa->ifa_address) {
> > + le = alloc_listener(ep, addr->ifa->ifa_address);
> > + if (!le)
> > + break;
> > + list_add_tail(&le->entry, &ep->listeners);
> > + added++;
> > + }
> > + }
> > + mutex_unlock(&h->mutex);
> > + if (ep->com.local_addr.sin_addr.s_addr != 0 && !added)
> > + err = -EADDRNOTAVAIL;
> > + if (!err && !added)
> > + printk(KERN_WARNING MOD
> > + "No RDMA interface found for device %s\n",
> > + pci_name(h->rdev.rnic_info.pdev));
> > + return err;
> > +}
>
> Adding some white space would improve readability.
>
> > +
> > +static int listen_stop_one(struct iwch_listen_ep *ep,
> unsigned int
> > +stid)
> > {
> > struct sk_buff *skb;
> > - struct cpl_pass_open_req *req;
> > + struct cpl_close_listserv_req *req;
> > +
> > + PDBG("%s stid %u\n", __FUNCTION__, stid);
> > + skb = get_skb(NULL, sizeof(*req), GFP_KERNEL);
> > + if (!skb) {
> > + printk(KERN_ERR MOD "%s - failed to alloc
> skb\n", __FUNCTION__);
> > + return -ENOMEM;
> > + }
> > + req = (struct cpl_close_listserv_req *) skb_put(skb,
> sizeof(*req));
> > + req->wr.wr_hi = htonl(V_WR_OP(FW_WROPCODE_FORWARD));
> > + req->cpu_idx = 0;
> > + OPCODE_TID(req) =
> htonl(MK_OPCODE_TID(CPL_CLOSE_LISTSRV_REQ, stid));
> > + skb->priority = 1;
> > + ep->com.rpl_err = 0;
> > + ep->com.rpl_done = 0;
> > + cxgb3_ofld_send(ep->com.tdev, skb);
> > + return wait_for_reply(&ep->com);
> > +}
> > +
> > +static int listen_stop(struct iwch_listen_ep *ep) {
> > + struct iwch_listen_entry *le;
> > + struct iwch_dev *h = to_iwch_dev(ep->com.cm_id->device);
> > + int err = 0;
> >
> > PDBG("%s ep %p\n", __FUNCTION__, ep);
> > + mutex_lock(&h->mutex);
> > + list_for_each_entry(le, &ep->listeners, entry) {
> > + err = listen_stop_one(ep, le->stid);
>
> This ends up blocking while holding a mutex, which looks like
> deadlock potential.
>
> > + if (err)
> > + break;
> > + }
> > + mutex_unlock(&h->mutex);
> > + return err;
> > +}
> > +
> > +static int listen_start_one(struct iwch_listen_ep *ep,
> unsigned int stid,
> > + __be32 addr, __be16 port)
> > +{
> > + struct sk_buff *skb;
> > + struct cpl_pass_open_req *req;
> > +
> > + PDBG("%s stid %u addr %x port %x\n", __FUNCTION__,
> stid, ntohl(addr),
> > + ntohs(port));
> > skb = get_skb(NULL, sizeof(*req), GFP_KERNEL);
> > if (!skb) {
> > - printk(KERN_ERR MOD "t3c_listen_start failed to
> alloc skb!\n");
> > + printk(KERN_ERR MOD "%s - failed to alloc
> skb\n", __FUNCTION__);
> > return -ENOMEM;
> > }
> >
> > req = (struct cpl_pass_open_req *) skb_put(skb, sizeof(*req));
> > req->wr.wr_hi = htonl(V_WR_OP(FW_WROPCODE_FORWARD));
> > - OPCODE_TID(req) =
> htonl(MK_OPCODE_TID(CPL_PASS_OPEN_REQ, ep->stid));
> > - req->local_port = ep->com.local_addr.sin_port;
> > - req->local_ip = ep->com.local_addr.sin_addr.s_addr;
> > + OPCODE_TID(req) = htonl(MK_OPCODE_TID(CPL_PASS_OPEN_REQ, stid));
> > + req->local_port = port;
> > + req->local_ip = addr;
> > req->peer_port = 0;
> > req->peer_ip = 0;
> > req->peer_netmask = 0;
> > @@ -1152,8 +1278,32 @@ static int listen_start(struct iwch_list
> > req->opt1 = htonl(V_CONN_POLICY(CPL_CONN_POLICY_ASK));
> >
> > skb->priority = 1;
> > + ep->com.rpl_err = 0;
> > + ep->com.rpl_done = 0;
> > cxgb3_ofld_send(ep->com.tdev, skb);
> > - return 0;
> > + return wait_for_reply(&ep->com);
> > +}
> > +
> > +static int listen_start(struct iwch_listen_ep *ep) {
> > + struct iwch_listen_entry *le;
> > + struct iwch_dev *h = to_iwch_dev(ep->com.cm_id->device);
> > + int err = 0;
> > +
> > + PDBG("%s ep %p\n", __FUNCTION__, ep);
> > + mutex_lock(&h->mutex);
> > + list_for_each_entry(le, &ep->listeners, entry) {
> > + err = listen_start_one(ep, le->stid, le->addr,
> > + ep->com.local_addr.sin_port);
>
> Similar to above - blocking while holding a mutex. There are
> a couple of other places where this also occurs.
>
> > + if (err)
> > + goto fail;
> > + }
> > + mutex_unlock(&h->mutex);
> > + return err;
> > +fail:
> > + mutex_unlock(&h->mutex);
> > + listen_stop(ep);
> > + return err;
> > }
> >
> > static int pass_open_rpl(struct t3cdev *tdev, struct sk_buff *skb,
> > void *ctx) @@ -1170,39 +1320,59 @@ static int
> pass_open_rpl(struct t3cdev *
> > return CPL_RET_BUF_DONE;
> > }
> >
> > -static int listen_stop(struct iwch_listen_ep *ep) -{
> > - struct sk_buff *skb;
> > - struct cpl_close_listserv_req *req;
> > -
> > - PDBG("%s ep %p\n", __FUNCTION__, ep);
> > - skb = get_skb(NULL, sizeof(*req), GFP_KERNEL);
> > - if (!skb) {
> > - printk(KERN_ERR MOD "%s - failed to alloc
> skb\n", __FUNCTION__);
> > - return -ENOMEM;
> > - }
> > - req = (struct cpl_close_listserv_req *) skb_put(skb,
> sizeof(*req));
> > - req->wr.wr_hi = htonl(V_WR_OP(FW_WROPCODE_FORWARD));
> > - req->cpu_idx = 0;
> > - OPCODE_TID(req) =
> htonl(MK_OPCODE_TID(CPL_CLOSE_LISTSRV_REQ, ep->stid));
> > - skb->priority = 1;
> > - cxgb3_ofld_send(ep->com.tdev, skb);
> > - return 0;
> > -}
> > -
> > static int close_listsrv_rpl(struct t3cdev *tdev, struct
> sk_buff *skb,
> > void *ctx)
> > {
> > struct iwch_listen_ep *ep = ctx;
> > struct cpl_close_listserv_rpl *rpl = cplhdr(skb);
> >
> > - PDBG("%s ep %p\n", __FUNCTION__, ep);
> > + PDBG("%s ep %p stid %u\n", __FUNCTION__, ep, GET_TID(rpl));
> > +
> > ep->com.rpl_err = status2errno(rpl->status);
> > ep->com.rpl_done = 1;
> > wake_up(&ep->com.waitq);
> > return CPL_RET_BUF_DONE;
> > }
> >
> > +void iwch_listeners_add_addr(struct iwch_dev *rnicp, __be32 addr) {
> > + struct iwch_listen_ep *listen_ep;
> > + struct iwch_listen_entry *le;
> > +
> > + mutex_lock(&rnicp->mutex);
> > + list_for_each_entry(listen_ep, &rnicp->listen_eps, entry) {
> > + if (listen_ep->com.local_addr.sin_addr.s_addr)
> > + continue;
> > + le = alloc_listener(listen_ep, addr);
> > + if (le) {
> > + list_add_tail(&le->entry,
> &listen_ep->listeners);
> > + listen_start_one(listen_ep, le->stid, addr,
> > +
> listen_ep->com.local_addr.sin_port);
> > + }
> > + }
> > + mutex_unlock(&rnicp->mutex);
> > +}
> > +
> > +void iwch_listeners_del_addr(struct iwch_dev *rnicp, __be32 addr) {
> > + struct iwch_listen_ep *listen_ep;
> > + struct iwch_listen_entry *le, *tmp;
> > +
> > + mutex_lock(&rnicp->mutex);
> > + list_for_each_entry(listen_ep, &rnicp->listen_eps, entry) {
> > + if (listen_ep->com.local_addr.sin_addr.s_addr)
> > + continue;
> > + list_for_each_entry_safe(le, tmp, &listen_ep->listeners,
> > + entry)
> > + if (le->addr == addr) {
> > + listen_stop_one(listen_ep, le->stid);
> > + list_del(&le->entry);
> > + dealloc_listener(listen_ep, le);
> > + }
> > + }
> > + mutex_unlock(&rnicp->mutex);
> > +}
> > +
> > static void accept_cr(struct iwch_ep *ep, __be32 peer_ip, struct
> > sk_buff *skb) {
> > struct cpl_pass_accept_rpl *rpl;
> > @@ -1767,8 +1937,7 @@ int iwch_accept_cr(struct iw_cm_id *cm_i
> > goto err;
> >
> > /* wait for wr_ack */
> > - wait_event(ep->com.waitq, ep->com.rpl_done);
> > - err = ep->com.rpl_err;
> > + err = wait_for_reply(&ep->com);
> > if (err)
> > goto err;
> >
> > @@ -1887,31 +2056,23 @@ int iwch_create_listen(struct iw_cm_id *
> > ep->com.cm_id = cm_id;
> > ep->backlog = backlog;
> > ep->com.local_addr = cm_id->local_addr;
> > + INIT_LIST_HEAD(&ep->listeners);
> >
> > - /*
> > - * Allocate a server TID.
> > - */
> > - ep->stid = cxgb3_alloc_stid(h->rdev.t3cdev_p, &t3c_client, ep);
> > - if (ep->stid == -1) {
> > - printk(KERN_ERR MOD "%s - cannot alloc
> atid.\n", __FUNCTION__);
> > - err = -ENOMEM;
> > + err = alloc_listener_list(ep);
> > + if (err)
> > goto fail2;
> > - }
> >
> > state_set(&ep->com, LISTEN);
> > err = listen_start(ep);
> > - if (err)
> > - goto fail3;
> >
> > - /* wait for pass_open_rpl */
> > - wait_event(ep->com.waitq, ep->com.rpl_done);
> > - err = ep->com.rpl_err;
> > if (!err) {
> > cm_id->provider_data = ep;
> > + mutex_lock(&h->mutex);
> > + list_add_tail(&ep->entry, &h->listen_eps);
> > + mutex_unlock(&h->mutex);
>
> Is there a race between listen_start() being called and
> inserting the ep into the list? Could anything try to find
> the ep on the list after listen_start returns?
>
> > goto out;
> > }
> > -fail3:
> > - cxgb3_free_stid(ep->com.tdev, ep->stid);
> > + dealloc_listener_list(ep);
> > fail2:
> > cm_id->rem_ref(cm_id);
> > put_ep(&ep->com);
> > @@ -1923,18 +2084,20 @@ out:
> > int iwch_destroy_listen(struct iw_cm_id *cm_id) {
> > int err;
> > + struct iwch_dev *h = to_iwch_dev(cm_id->device);
> > struct iwch_listen_ep *ep = to_listen_ep(cm_id);
> >
> > PDBG("%s ep %p\n", __FUNCTION__, ep);
> >
> > might_sleep();
> > + mutex_lock(&h->mutex);
> > + list_del(&ep->entry);
> > + mutex_unlock(&h->mutex);
> > state_set(&ep->com, DEAD);
> > ep->com.rpl_done = 0;
> > ep->com.rpl_err = 0;
> > err = listen_stop(ep);
> > - wait_event(ep->com.waitq, ep->com.rpl_done);
> > - cxgb3_free_stid(ep->com.tdev, ep->stid);
> > - err = ep->com.rpl_err;
> > + dealloc_listener_list(ep);
> > cm_id->rem_ref(cm_id);
> > put_ep(&ep->com);
> > return err;
> > diff --git a/drivers/infiniband/hw/cxgb3/iwch_cm.h
> > b/drivers/infiniband/hw/cxgb3/iwch_cm.h
> > index 6107e7c..23e5a22 100644
> > --- a/drivers/infiniband/hw/cxgb3/iwch_cm.h
> > +++ b/drivers/infiniband/hw/cxgb3/iwch_cm.h
> > @@ -162,10 +162,19 @@ struct iwch_ep_common {
> > int rpl_err;
> > };
> >
> > -struct iwch_listen_ep {
> > - struct iwch_ep_common com;
> > +struct iwch_listen_entry {
> > + struct list_head entry;
> > unsigned int stid;
> > + __be32 addr;
> > +};
> > +
> > +struct iwch_listen_ep {
> > + struct iwch_ep_common com; /* Must be first entry! */
> > + struct list_head entry;
> > + struct list_head listeners;
> > int backlog;
> > + int listen_count;
>
> I didn't notice where this was used.
>
> > + int listen_rpls;
>
> or this.
>
> > };
> >
> > struct iwch_ep {
> > @@ -222,6 +231,8 @@ int iwch_resume_tid(struct iwch_ep *ep); void
> > __free_ep(struct kref *kref); void iwch_rearp(struct
> iwch_ep *ep);
> > int iwch_ep_redirect(void *ctx, struct dst_entry *old, struct
> > dst_entry *new, struct l2t_entry *l2t);
> > +void iwch_listeners_add_addr(struct iwch_dev *rnicp, __be32 addr);
> > +void iwch_listeners_del_addr(struct iwch_dev *rnicp, __be32 addr);
> >
> > int __init iwch_cm_init(void);
> > void __exit iwch_cm_term(void);
> > _______________________________________________
> > general mailing list
> > general@lists.openfabrics.org
> > http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general
> >
> > To unsubscribe, please visit
> > http://openib.org/mailman/listinfo/openib-general
> >
> _______________________________________________
> general mailing list
> general@lists.openfabrics.org
> http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general
>
> To unsubscribe, please visit
> http://openib.org/mailman/listinfo/openib-general
>
^ permalink raw reply
* Re: [RFC/PATCH 2/3] UDP memory usage accounting: accounting unit and variable
From: Hideo AOKI @ 2007-09-27 18:54 UTC (permalink / raw)
To: Andi Kleen; +Cc: Satoshi OSHIMA, netdev, yoshfuji
In-Reply-To: <p734pho9nb9.fsf@bingen.suse.de>
Hello,
I apologize for not replying sooner.
Andi Kleen wrote:
> Satoshi OSHIMA <satoshi.oshima.fk@hitachi.com> writes:
>
>> This patch introduces global variable for UDP memory accounting.
>> The unit is page.
>
> The global variable doesn't seem to be very MP scalable, especially
> if you change it for each packet. This will be a very hot cache line,
> in the worst case bouncing around a large machine.
>
> Possible alternatives:
> - Per CPU variables
> - You only change the global on socket creation time (by pre allocating a large
> amount) or when the system comes under memory pressure.
> - Batching of the global updates for multiple packets [that's a variant
> of the previous one, might be still too costly though]
>
> Also for such variables it's usually good to cache line pad them on SMP
> to avoid false sharing with something else.
>
> -Andi
Thank you so much for your suggestions.
The implementation of the patch basically followed implementation of
tcp_memory_allocated. However, I should agree that the patch introduces
atomic operations too much. Then, I'll try to use the batching to reduce
the number of atomic operations.
Best regards,
Hideo Aoki
--
Hitachi Computer Products (America) Inc.
^ permalink raw reply
* Re: [PATCH 1/4] net: Dynamically allocate the per cpu counters for the loopback device.
From: David Miller @ 2007-09-27 18:52 UTC (permalink / raw)
To: ebiederm; +Cc: netdev, containers
In-Reply-To: <m1myv8r1lb.fsf@ebiederm.dsl.xmission.com>
From: ebiederm@xmission.com (Eric W. Biederman)
Date: Thu, 27 Sep 2007 01:48:00 -0600
> I'm not doing get_cpu/put_cpu so does the comment make sense
> in relationship to per_cpu_ptr?
It is possible. But someone would need to go check for
sure.
^ permalink raw reply
* Re: [RFC/PATCH 0/3] UDP memory usage accounting
From: Hideo AOKI @ 2007-09-27 18:51 UTC (permalink / raw)
To: Evgeniy Polyakov; +Cc: Satoshi OSHIMA, netdev, yoshfuji
In-Reply-To: <20070921125843.GA15810@2ka.mipt.ru>
Hello,
Apologies for late response.
Evgeniy Polyakov wrote:
> Hi.
>
> On Fri, Sep 21, 2007 at 09:18:07PM +0900, Satoshi OSHIMA (satoshi.oshima.fk@hitachi.com) wrote:
>> This patch set try to introduce memory usage accounting for
>> UDP(currently ipv4 only).
>>
>> Currently, memory usage of UDP can be observed as the sam of
>> usage of tx_queue and rx_queue. But I believe that the system
>> wide accounting is usefull when heavy loaded condition.
>>
>> In the next step, I would like to add memory usage quota
>> for UDP to avoid unlimited memory consumption problem
>> under DDOS attack.
>
> Could you please desribed such attack in more details?
> Each UDP socket has its queue length which can not be exceeded
> (roughly), no new sockets are created when remote side sends a packet
> (like after special steps in TCP), so where is possibility to eat all
> the mem?
I think Satoshi will answer this question soon.
>> This patch set is for 2.6.23-rc7.
>
> I seriously doubt you want to put udp specific hacks and zillions of
> atomic ops all around the code just to know exact number of bytes eaten
> for UDP.
I'll revise the patch to reduce the number of atomic operations.
> Please use udp specific code (like udp_sendmsg()) for proper accounting
> if you need that, but not hacks in generic ip code.
As far as I know, Satoshi is improving this part right now. Please wait his
response.
Many thanks for your comments.
Best regards,
Hideo Aoki
--
Hitachi Computer Products (America) Inc.
^ permalink raw reply
* Re: [ofa-general] [PATCH v3] iw_cxgb3: Support "iwarp-only" interfaces to avoid 4-tuple conflicts.
From: Sean Hefty @ 2007-09-27 18:38 UTC (permalink / raw)
To: Steve Wise; +Cc: rdreier, sean.hefty, netdev, linux-kernel, general
In-Reply-To: <20070923203649.8324.64524.stgit@dell3.ogc.int>
> The sysadmin creates "for iwarp use only" alias interfaces of the form
> "devname:iw*" where devname is the native interface name (eg eth0) for the
> iwarp netdev device. The alias label can be anything starting with "iw".
> The "iw" immediately after the ':' is the key used by the iw_cxgb3 driver.
I'm still not sure about this, but haven't come up with anything better
myself. And if there's a good chance of other rnic's needing the same
support, I'd rather see the common code separated out, even if just
encapsulated within this module for easy re-use.
As for the code, I have a couple of questions about whether deadlock and
a race condition are possible, plus a few minor comments.
> +static void insert_ifa(struct iwch_dev *rnicp, struct in_ifaddr *ifa)
> +{
> + struct iwch_addrlist *addr;
> +
> + addr = kmalloc(sizeof *addr, GFP_KERNEL);
> + 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);
> +}
Should this return success/failure?
> +static int nb_callback(struct notifier_block *self, unsigned long event,
> + void *ctx)
> +{
> + struct in_ifaddr *ifa = ctx;
> + struct iwch_dev *rnicp = container_of(self, struct iwch_dev, nb);
> +
> + PDBG("%s rnicp %p event %lx\n", __FUNCTION__, rnicp, event);
> +
> + switch (event) {
> + case NETDEV_UP:
> + if (netdev_is_ours(rnicp, ifa->ifa_dev->dev) &&
> + is_iwarp_label(ifa->ifa_label)) {
> + PDBG("%s label %s addr 0x%x added\n",
> + __FUNCTION__, ifa->ifa_label, ifa->ifa_address);
> + insert_ifa(rnicp, ifa);
> + iwch_listeners_add_addr(rnicp, ifa->ifa_address);
If insert_ifa() fails, what will iwch_listeners_add_addr() do? (I'm not
easily seeing the relationship between the address list and the listen
list at this point.)
> + }
> + break;
> + case NETDEV_DOWN:
> + if (netdev_is_ours(rnicp, ifa->ifa_dev->dev) &&
> + is_iwarp_label(ifa->ifa_label)) {
> + PDBG("%s label %s addr 0x%x deleted\n",
> + __FUNCTION__, ifa->ifa_label, ifa->ifa_address);
> + iwch_listeners_del_addr(rnicp, ifa->ifa_address);
> + remove_ifa(rnicp, ifa);
> + }
> + break;
> + default:
> + break;
> + }
> + return 0;
> +}
> +
> +static void delete_addrlist(struct iwch_dev *rnicp)
> +{
> + struct iwch_addrlist *addr, *tmp;
> +
> + mutex_lock(&rnicp->mutex);
> + list_for_each_entry_safe(addr, tmp, &rnicp->addrlist, entry) {
> + list_del(&addr->entry);
> + kfree(addr);
> + }
> + mutex_unlock(&rnicp->mutex);
> +}
> +
> +static void populate_addrlist(struct iwch_dev *rnicp)
> +{
> + int i;
> + struct in_device *indev;
> +
> + for (i = 0; i < rnicp->rdev.port_info.nports; i++) {
> + indev = in_dev_get(rnicp->rdev.port_info.lldevs[i]);
> + if (!indev)
> + continue;
> + for_ifa(indev)
> + if (is_iwarp_label(ifa->ifa_label)) {
> + PDBG("%s label %s addr 0x%x added\n",
> + __FUNCTION__, ifa->ifa_label,
> + ifa->ifa_address);
> + insert_ifa(rnicp, ifa);
> + }
> + endfor_ifa(indev);
> + }
> +}
> +
> static void rnic_init(struct iwch_dev *rnicp)
> {
> PDBG("%s iwch_dev %p\n", __FUNCTION__, rnicp);
> @@ -70,6 +187,12 @@ static void rnic_init(struct iwch_dev *r
> idr_init(&rnicp->qpidr);
> idr_init(&rnicp->mmidr);
> spin_lock_init(&rnicp->lock);
> + INIT_LIST_HEAD(&rnicp->addrlist);
> + INIT_LIST_HEAD(&rnicp->listen_eps);
> + mutex_init(&rnicp->mutex);
> + rnicp->nb.notifier_call = nb_callback;
> + populate_addrlist(rnicp);
> + register_inetaddr_notifier(&rnicp->nb);
>
> rnicp->attr.vendor_id = 0x168;
> rnicp->attr.vendor_part_id = 7;
> @@ -148,6 +271,8 @@ static void close_rnic_dev(struct t3cdev
> mutex_lock(&dev_mutex);
> list_for_each_entry_safe(dev, tmp, &dev_list, entry) {
> if (dev->rdev.t3cdev_p == tdev) {
> + unregister_inetaddr_notifier(&dev->nb);
> + delete_addrlist(dev);
> list_del(&dev->entry);
> iwch_unregister_device(dev);
> cxio_rdev_close(&dev->rdev);
> diff --git a/drivers/infiniband/hw/cxgb3/iwch.h b/drivers/infiniband/hw/cxgb3/iwch.h
> index caf4e60..7fa0a47 100644
> --- a/drivers/infiniband/hw/cxgb3/iwch.h
> +++ b/drivers/infiniband/hw/cxgb3/iwch.h
> @@ -36,6 +36,8 @@ #include <linux/mutex.h>
> #include <linux/list.h>
> #include <linux/spinlock.h>
> #include <linux/idr.h>
> +#include <linux/notifier.h>
> +#include <linux/inetdevice.h>
>
> #include <rdma/ib_verbs.h>
>
> @@ -101,6 +103,11 @@ struct iwch_rnic_attributes {
> u32 cq_overflow_detection;
> };
>
> +struct iwch_addrlist {
> + struct list_head entry;
> + struct in_ifaddr *ifa;
> +};
> +
> struct iwch_dev {
> struct ib_device ibdev;
> struct cxio_rdev rdev;
> @@ -111,6 +118,10 @@ struct iwch_dev {
> struct idr mmidr;
> spinlock_t lock;
> struct list_head entry;
> + struct notifier_block nb;
> + struct list_head addrlist;
> + struct list_head listen_eps;
The behavior of the listen lists is similar to what's done in the
rdma_cm: Wildcard listens are stored in a listen_any_list. When new
devices are added, associated listens are added to each device. This is
similar, except we're dealing with devices and addresses. I'm wondering
if we shouldn't mimic the same behavior and track listens in
iwch_addrlist directly. (I don't see anything wrong with this approach
though.)
What happens if an address changes between iwarp only and non-iwarp?
How are listens on specific addresses handled from an rdma_cm level?
Does the rdma_cm map the address to the device, call the iw_cm to
listen, which in turn calls the device listen function? The device then
checks that the address has been marked as iwarp only? (I'm being too
lazy to trace this through the code, but if you don't know off the top
of your head, I will do that.)
> + struct mutex mutex;
> };
>
> static inline struct iwch_dev *to_iwch_dev(struct ib_device *ibdev)
> diff --git a/drivers/infiniband/hw/cxgb3/iwch_cm.c b/drivers/infiniband/hw/cxgb3/iwch_cm.c
> index 1cdfcd4..afc8a48 100644
> --- a/drivers/infiniband/hw/cxgb3/iwch_cm.c
> +++ b/drivers/infiniband/hw/cxgb3/iwch_cm.c
> @@ -1127,23 +1127,149 @@ static int act_open_rpl(struct t3cdev *t
> return CPL_RET_BUF_DONE;
> }
>
> -static int listen_start(struct iwch_listen_ep *ep)
> +static int wait_for_reply(struct iwch_ep_common *epc)
> +{
> + PDBG("%s ep %p waiting\n", __FUNCTION__, epc);
> + wait_event(epc->waitq, epc->rpl_done);
> + PDBG("%s ep %p done waiting err %d\n", __FUNCTION__, epc, epc->rpl_err);
> + return epc->rpl_err;
> +}
What thread is being blocked here, and what sets the event?
> +
> +static struct iwch_listen_entry *alloc_listener(struct iwch_listen_ep *ep,
> + __be32 addr)
> +{
> + struct iwch_dev *h = to_iwch_dev(ep->com.cm_id->device);
> + struct iwch_listen_entry *le;
> +
> + le = kmalloc(sizeof *le, GFP_KERNEL);
> + if (!le) {
> + printk(KERN_ERR MOD "%s - failed to alloc memory!\n",
> + __FUNCTION__);
> + return NULL;
> + }
> + le->stid = cxgb3_alloc_stid(h->rdev.t3cdev_p,
> + &t3c_client, ep);
> + if (le->stid == -1) {
> + printk(KERN_ERR MOD "%s - cannot alloc stid.\n",
> + __FUNCTION__);
> + kfree(le);
> + return NULL;
> + }
> + le->addr = addr;
> + PDBG("%s stid %u addr %x port %x\n", __FUNCTION__, le->stid,
> + ntohl(le->addr), ntohs(ep->com.local_addr.sin_port));
> + return le;
> +}
> +
> +static void dealloc_listener(struct iwch_listen_ep *ep,
> + struct iwch_listen_entry *le)
> +{
> + PDBG("%s stid %u addr %x port %x\n", __FUNCTION__, le->stid,
> + ntohl(le->addr), ntohs(ep->com.local_addr.sin_port));
> + cxgb3_free_stid(ep->com.tdev, le->stid);
> + kfree(le);
> +}
> +
> +static void dealloc_listener_list(struct iwch_listen_ep *ep)
> +{
> + struct iwch_listen_entry *le, *tmp;
> + struct iwch_dev *h = to_iwch_dev(ep->com.cm_id->device);
> +
> + mutex_lock(&h->mutex);
> + list_for_each_entry_safe(le, tmp, &ep->listeners, entry) {
> + list_del(&le->entry);
> + dealloc_listener(ep, le);
> + }
> + mutex_unlock(&h->mutex);
> +}
> +
> +static int alloc_listener_list(struct iwch_listen_ep *ep)
> +{
> + struct iwch_dev *h = to_iwch_dev(ep->com.cm_id->device);
nit: in place of 'h' in several places, how about 'rnicp', which is also
used?
> + struct iwch_addrlist *addr;
> + struct iwch_listen_entry *le;
> + int err = 0;
> + int added=0;
> + mutex_lock(&h->mutex);
> + list_for_each_entry(addr, &h->addrlist, entry) {
> + if (ep->com.local_addr.sin_addr.s_addr == 0 ||
> + ep->com.local_addr.sin_addr.s_addr ==
> + addr->ifa->ifa_address) {
> + le = alloc_listener(ep, addr->ifa->ifa_address);
> + if (!le)
> + break;
> + list_add_tail(&le->entry, &ep->listeners);
> + added++;
> + }
> + }
> + mutex_unlock(&h->mutex);
> + if (ep->com.local_addr.sin_addr.s_addr != 0 && !added)
> + err = -EADDRNOTAVAIL;
> + if (!err && !added)
> + printk(KERN_WARNING MOD
> + "No RDMA interface found for device %s\n",
> + pci_name(h->rdev.rnic_info.pdev));
> + return err;
> +}
Adding some white space would improve readability.
> +
> +static int listen_stop_one(struct iwch_listen_ep *ep, unsigned int stid)
> {
> struct sk_buff *skb;
> - struct cpl_pass_open_req *req;
> + struct cpl_close_listserv_req *req;
> +
> + PDBG("%s stid %u\n", __FUNCTION__, stid);
> + skb = get_skb(NULL, sizeof(*req), GFP_KERNEL);
> + if (!skb) {
> + printk(KERN_ERR MOD "%s - failed to alloc skb\n", __FUNCTION__);
> + return -ENOMEM;
> + }
> + req = (struct cpl_close_listserv_req *) skb_put(skb, sizeof(*req));
> + req->wr.wr_hi = htonl(V_WR_OP(FW_WROPCODE_FORWARD));
> + req->cpu_idx = 0;
> + OPCODE_TID(req) = htonl(MK_OPCODE_TID(CPL_CLOSE_LISTSRV_REQ, stid));
> + skb->priority = 1;
> + ep->com.rpl_err = 0;
> + ep->com.rpl_done = 0;
> + cxgb3_ofld_send(ep->com.tdev, skb);
> + return wait_for_reply(&ep->com);
> +}
> +
> +static int listen_stop(struct iwch_listen_ep *ep)
> +{
> + struct iwch_listen_entry *le;
> + struct iwch_dev *h = to_iwch_dev(ep->com.cm_id->device);
> + int err = 0;
>
> PDBG("%s ep %p\n", __FUNCTION__, ep);
> + mutex_lock(&h->mutex);
> + list_for_each_entry(le, &ep->listeners, entry) {
> + err = listen_stop_one(ep, le->stid);
This ends up blocking while holding a mutex, which looks like deadlock
potential.
> + if (err)
> + break;
> + }
> + mutex_unlock(&h->mutex);
> + return err;
> +}
> +
> +static int listen_start_one(struct iwch_listen_ep *ep, unsigned int stid,
> + __be32 addr, __be16 port)
> +{
> + struct sk_buff *skb;
> + struct cpl_pass_open_req *req;
> +
> + PDBG("%s stid %u addr %x port %x\n", __FUNCTION__, stid, ntohl(addr),
> + ntohs(port));
> skb = get_skb(NULL, sizeof(*req), GFP_KERNEL);
> if (!skb) {
> - printk(KERN_ERR MOD "t3c_listen_start failed to alloc skb!\n");
> + printk(KERN_ERR MOD "%s - failed to alloc skb\n", __FUNCTION__);
> return -ENOMEM;
> }
>
> req = (struct cpl_pass_open_req *) skb_put(skb, sizeof(*req));
> req->wr.wr_hi = htonl(V_WR_OP(FW_WROPCODE_FORWARD));
> - OPCODE_TID(req) = htonl(MK_OPCODE_TID(CPL_PASS_OPEN_REQ, ep->stid));
> - req->local_port = ep->com.local_addr.sin_port;
> - req->local_ip = ep->com.local_addr.sin_addr.s_addr;
> + OPCODE_TID(req) = htonl(MK_OPCODE_TID(CPL_PASS_OPEN_REQ, stid));
> + req->local_port = port;
> + req->local_ip = addr;
> req->peer_port = 0;
> req->peer_ip = 0;
> req->peer_netmask = 0;
> @@ -1152,8 +1278,32 @@ static int listen_start(struct iwch_list
> req->opt1 = htonl(V_CONN_POLICY(CPL_CONN_POLICY_ASK));
>
> skb->priority = 1;
> + ep->com.rpl_err = 0;
> + ep->com.rpl_done = 0;
> cxgb3_ofld_send(ep->com.tdev, skb);
> - return 0;
> + return wait_for_reply(&ep->com);
> +}
> +
> +static int listen_start(struct iwch_listen_ep *ep)
> +{
> + struct iwch_listen_entry *le;
> + struct iwch_dev *h = to_iwch_dev(ep->com.cm_id->device);
> + int err = 0;
> +
> + PDBG("%s ep %p\n", __FUNCTION__, ep);
> + mutex_lock(&h->mutex);
> + list_for_each_entry(le, &ep->listeners, entry) {
> + err = listen_start_one(ep, le->stid, le->addr,
> + ep->com.local_addr.sin_port);
Similar to above - blocking while holding a mutex. There are a couple
of other places where this also occurs.
> + if (err)
> + goto fail;
> + }
> + mutex_unlock(&h->mutex);
> + return err;
> +fail:
> + mutex_unlock(&h->mutex);
> + listen_stop(ep);
> + return err;
> }
>
> static int pass_open_rpl(struct t3cdev *tdev, struct sk_buff *skb, void *ctx)
> @@ -1170,39 +1320,59 @@ static int pass_open_rpl(struct t3cdev *
> return CPL_RET_BUF_DONE;
> }
>
> -static int listen_stop(struct iwch_listen_ep *ep)
> -{
> - struct sk_buff *skb;
> - struct cpl_close_listserv_req *req;
> -
> - PDBG("%s ep %p\n", __FUNCTION__, ep);
> - skb = get_skb(NULL, sizeof(*req), GFP_KERNEL);
> - if (!skb) {
> - printk(KERN_ERR MOD "%s - failed to alloc skb\n", __FUNCTION__);
> - return -ENOMEM;
> - }
> - req = (struct cpl_close_listserv_req *) skb_put(skb, sizeof(*req));
> - req->wr.wr_hi = htonl(V_WR_OP(FW_WROPCODE_FORWARD));
> - req->cpu_idx = 0;
> - OPCODE_TID(req) = htonl(MK_OPCODE_TID(CPL_CLOSE_LISTSRV_REQ, ep->stid));
> - skb->priority = 1;
> - cxgb3_ofld_send(ep->com.tdev, skb);
> - return 0;
> -}
> -
> static int close_listsrv_rpl(struct t3cdev *tdev, struct sk_buff *skb,
> void *ctx)
> {
> struct iwch_listen_ep *ep = ctx;
> struct cpl_close_listserv_rpl *rpl = cplhdr(skb);
>
> - PDBG("%s ep %p\n", __FUNCTION__, ep);
> + PDBG("%s ep %p stid %u\n", __FUNCTION__, ep, GET_TID(rpl));
> +
> ep->com.rpl_err = status2errno(rpl->status);
> ep->com.rpl_done = 1;
> wake_up(&ep->com.waitq);
> return CPL_RET_BUF_DONE;
> }
>
> +void iwch_listeners_add_addr(struct iwch_dev *rnicp, __be32 addr)
> +{
> + struct iwch_listen_ep *listen_ep;
> + struct iwch_listen_entry *le;
> +
> + mutex_lock(&rnicp->mutex);
> + list_for_each_entry(listen_ep, &rnicp->listen_eps, entry) {
> + if (listen_ep->com.local_addr.sin_addr.s_addr)
> + continue;
> + le = alloc_listener(listen_ep, addr);
> + if (le) {
> + list_add_tail(&le->entry, &listen_ep->listeners);
> + listen_start_one(listen_ep, le->stid, addr,
> + listen_ep->com.local_addr.sin_port);
> + }
> + }
> + mutex_unlock(&rnicp->mutex);
> +}
> +
> +void iwch_listeners_del_addr(struct iwch_dev *rnicp, __be32 addr)
> +{
> + struct iwch_listen_ep *listen_ep;
> + struct iwch_listen_entry *le, *tmp;
> +
> + mutex_lock(&rnicp->mutex);
> + list_for_each_entry(listen_ep, &rnicp->listen_eps, entry) {
> + if (listen_ep->com.local_addr.sin_addr.s_addr)
> + continue;
> + list_for_each_entry_safe(le, tmp, &listen_ep->listeners,
> + entry)
> + if (le->addr == addr) {
> + listen_stop_one(listen_ep, le->stid);
> + list_del(&le->entry);
> + dealloc_listener(listen_ep, le);
> + }
> + }
> + mutex_unlock(&rnicp->mutex);
> +}
> +
> static void accept_cr(struct iwch_ep *ep, __be32 peer_ip, struct sk_buff *skb)
> {
> struct cpl_pass_accept_rpl *rpl;
> @@ -1767,8 +1937,7 @@ int iwch_accept_cr(struct iw_cm_id *cm_i
> goto err;
>
> /* wait for wr_ack */
> - wait_event(ep->com.waitq, ep->com.rpl_done);
> - err = ep->com.rpl_err;
> + err = wait_for_reply(&ep->com);
> if (err)
> goto err;
>
> @@ -1887,31 +2056,23 @@ int iwch_create_listen(struct iw_cm_id *
> ep->com.cm_id = cm_id;
> ep->backlog = backlog;
> ep->com.local_addr = cm_id->local_addr;
> + INIT_LIST_HEAD(&ep->listeners);
>
> - /*
> - * Allocate a server TID.
> - */
> - ep->stid = cxgb3_alloc_stid(h->rdev.t3cdev_p, &t3c_client, ep);
> - if (ep->stid == -1) {
> - printk(KERN_ERR MOD "%s - cannot alloc atid.\n", __FUNCTION__);
> - err = -ENOMEM;
> + err = alloc_listener_list(ep);
> + if (err)
> goto fail2;
> - }
>
> state_set(&ep->com, LISTEN);
> err = listen_start(ep);
> - if (err)
> - goto fail3;
>
> - /* wait for pass_open_rpl */
> - wait_event(ep->com.waitq, ep->com.rpl_done);
> - err = ep->com.rpl_err;
> if (!err) {
> cm_id->provider_data = ep;
> + mutex_lock(&h->mutex);
> + list_add_tail(&ep->entry, &h->listen_eps);
> + mutex_unlock(&h->mutex);
Is there a race between listen_start() being called and inserting the ep
into the list? Could anything try to find the ep on the list after
listen_start returns?
> goto out;
> }
> -fail3:
> - cxgb3_free_stid(ep->com.tdev, ep->stid);
> + dealloc_listener_list(ep);
> fail2:
> cm_id->rem_ref(cm_id);
> put_ep(&ep->com);
> @@ -1923,18 +2084,20 @@ out:
> int iwch_destroy_listen(struct iw_cm_id *cm_id)
> {
> int err;
> + struct iwch_dev *h = to_iwch_dev(cm_id->device);
> struct iwch_listen_ep *ep = to_listen_ep(cm_id);
>
> PDBG("%s ep %p\n", __FUNCTION__, ep);
>
> might_sleep();
> + mutex_lock(&h->mutex);
> + list_del(&ep->entry);
> + mutex_unlock(&h->mutex);
> state_set(&ep->com, DEAD);
> ep->com.rpl_done = 0;
> ep->com.rpl_err = 0;
> err = listen_stop(ep);
> - wait_event(ep->com.waitq, ep->com.rpl_done);
> - cxgb3_free_stid(ep->com.tdev, ep->stid);
> - err = ep->com.rpl_err;
> + dealloc_listener_list(ep);
> cm_id->rem_ref(cm_id);
> put_ep(&ep->com);
> return err;
> diff --git a/drivers/infiniband/hw/cxgb3/iwch_cm.h b/drivers/infiniband/hw/cxgb3/iwch_cm.h
> index 6107e7c..23e5a22 100644
> --- a/drivers/infiniband/hw/cxgb3/iwch_cm.h
> +++ b/drivers/infiniband/hw/cxgb3/iwch_cm.h
> @@ -162,10 +162,19 @@ struct iwch_ep_common {
> int rpl_err;
> };
>
> -struct iwch_listen_ep {
> - struct iwch_ep_common com;
> +struct iwch_listen_entry {
> + struct list_head entry;
> unsigned int stid;
> + __be32 addr;
> +};
> +
> +struct iwch_listen_ep {
> + struct iwch_ep_common com; /* Must be first entry! */
> + struct list_head entry;
> + struct list_head listeners;
> int backlog;
> + int listen_count;
I didn't notice where this was used.
> + int listen_rpls;
or this.
> };
>
> struct iwch_ep {
> @@ -222,6 +231,8 @@ int iwch_resume_tid(struct iwch_ep *ep);
> void __free_ep(struct kref *kref);
> void iwch_rearp(struct iwch_ep *ep);
> int iwch_ep_redirect(void *ctx, struct dst_entry *old, struct dst_entry *new, struct l2t_entry *l2t);
> +void iwch_listeners_add_addr(struct iwch_dev *rnicp, __be32 addr);
> +void iwch_listeners_del_addr(struct iwch_dev *rnicp, __be32 addr);
>
> int __init iwch_cm_init(void);
> void __exit iwch_cm_term(void);
> _______________________________________________
> general mailing list
> general@lists.openfabrics.org
> http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general
>
> To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
>
^ permalink raw reply
* Re: RTL8111 PCI Express Gigabit driver r8169 produces slow file transfers
From: Francois Romieu @ 2007-09-27 17:24 UTC (permalink / raw)
To: Achim Frase; +Cc: netdev
In-Reply-To: <1190898841.7919.14.camel@BeeB1t>
Achim Frase <achim.frase@googlemail.com> :
> [...]
> but I hope you could help me.
Yes. Please try any of:
- current 2.6.23-git
- 2.6.23-rc8 + patch below
diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
index b85ab4a..c921ec3 100644
--- a/drivers/net/r8169.c
+++ b/drivers/net/r8169.c
@@ -1228,7 +1228,10 @@ static void rtl8169_hw_phy_config(struct net_device *dev)
return;
}
- /* phy config for RTL8169s mac_version C chip */
+ if ((tp->mac_version != RTL_GIGA_MAC_VER_02) &&
+ (tp->mac_version != RTL_GIGA_MAC_VER_03))
+ return;
+
mdio_write(ioaddr, 31, 0x0001); //w 31 2 0 1
mdio_write(ioaddr, 21, 0x1000); //w 21 15 0 1000
mdio_write(ioaddr, 24, 0x65c7); //w 24 15 0 65c7
@@ -2567,6 +2570,15 @@ static void rtl8169_tx_interrupt(struct net_device *dev,
(TX_BUFFS_AVAIL(tp) >= MAX_SKB_FRAGS)) {
netif_wake_queue(dev);
}
+ /*
+ * 8168 hack: TxPoll requests are lost when the Tx packets are
+ * too close. Let's kick an extra TxPoll request when a burst
+ * of start_xmit activity is detected (if it is not detected,
+ * it is slow enough). -- FR
+ */
+ smp_rmb();
+ if (tp->cur_tx != dirty_tx)
+ RTL_W8(TxPoll, NPQ);
}
}
--
Ueimor
^ permalink raw reply related
* Re: [PATCH] net: Add network namespace clone & unshare support.
From: Eric W. Biederman @ 2007-09-27 17:14 UTC (permalink / raw)
To: Cedric Le Goater; +Cc: David Miller, Linux Containers, netdev, Andrew Morton
In-Reply-To: <46FB6F1E.8070105@fr.ibm.com>
Cedric Le Goater <clg@fr.ibm.com> writes:
>> diff --git a/include/linux/sched.h b/include/linux/sched.h
>> index a01ac6d..e10a0a8 100644
>> --- a/include/linux/sched.h
>> +++ b/include/linux/sched.h
>> @@ -27,6 +27,7 @@
>> #define CLONE_NEWUTS 0x04000000 /* New utsname group? */
>> #define CLONE_NEWIPC 0x08000000 /* New ipcs */
>> #define CLONE_NEWUSER 0x10000000 /* New user namespace */
>> +#define CLONE_NEWNET 0x20000000 /* New network namespace */
>
> This new flag is going to conflict with the pid namespace flag
> CLONE_NEWPID in -mm. It might be worth changing it to:
>
> #define CLONE_NEWNET 0x40000000
Interesting, it would have been nice if someone had caught this
detail earlier. Oh well.
Thanks for pointing this out, it's on my todo list to look into,
and ensure we resolve.
I'm confused because my notes have 0x80000000 for the pid namespace,
and 0x40000000 for the time namespace.
> The changes in nxproxy.c and fork.c will also conflict but I don't
> think we can do much about it for now.
They should also be fairly easy conflicts to resolve.
I guess we are likely to hit this conflict in the next -mm or the
merge window, which ever comes first.
Eric
^ permalink raw reply
* Re: [Devel] [PATCH 4/4] net: Make the loopback device per network namespace
From: Eric W. Biederman @ 2007-09-27 16:48 UTC (permalink / raw)
To: Denis V. Lunev; +Cc: David Miller, Linux Containers, netdev
In-Reply-To: <46FB9EB1.5070908@sw.ru>
"Denis V. Lunev" <den@sw.ru> writes:
> Eric W. Biederman wrote:
>> This patch makes loopback_dev per network namespace. Adding
>> code to create a different loopback device for each network
>> namespace and adding the code to free a loopback device
>> when a network namespace exits.
>>
>> This patch modifies all users the loopback_dev so they
>> access it as init_net.loopback_dev, keeping all of the
>> code compiling and working. A later pass will be needed to
>> update the users to use something other than the initial network
>> namespace.
>
> A pity that an important bit of explanation is missed. The
> initialization of loopback_dev is moved from a chain of devices
> (init_module) to a subsystem initialization to keep proper order, i.e.
> we must be sure that the initialization order is correct.
That didn't happen in the patch you mentioned. That happened
when we started dynamically allocating the loopback device.
That was the patch Daniel sent out a bit ago.
There are certainly some ordering issues and it may have helped
to talk about them. But they are because things assume the
loopback device is present. We have various bits of code that
is around such as the dst_ifdown case that assumes if another
network device is present the loopback device is present. To
fulfill that assumption I guess that means we have both an
initialization order dependency and a destruction order dependency.
The fact we were using module_init before actually appears to
me to have been racy, but we got away with it because the actual
data structure was statically allocated.
Since it appears that for a dynamically allocated loopback
registering it first and unregistering it last is necessary
for routing. It is likely worth looking at this a little
more closely and making a guarantee. So we can make it easier
for networking layers like ipv6, that want to memorize which
device is the loopback device.
Eric
^ permalink raw reply
* Re: [PATCH] various dst_ifdown routines to catch refcounting bugs
From: Eric W. Biederman @ 2007-09-27 16:27 UTC (permalink / raw)
To: Denis V. Lunev; +Cc: davem, devel, netdev, containers, ebiederm
In-Reply-To: <20070927134724.GA11531@iris.sw.ru>
"Denis V. Lunev" <den@openvz.org> writes:
> Moving dst entries into init_net.loopback_dev is not a good thing.
> This hides obvious and non-obvious ref-counting bugs.
Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>
To be clear using init_net.loopback is currently safe because we don't
have any destination cache entries for anything except the initial
network namespace.
I have not yet made this change simply because I haven't gotten around
to this part in my patches.
I do have a question I would like to bring up, because I like avoiding
explicit references to loopback_dev when I can.
/* Dirty hack. We did it in 2.2 (in __dst_free),
* we have _very_ good reasons not to repeat
* this mistake in 2.3, but we have no choice
* now. _It_ _is_ _explicit_ _deliberate_
* _race_ _condition_.
*
* Commented and originally written by Alexey.
*/
What is the race that is talked about in that comment. Can we just
assign NULL instead of the loopback device when we bring a route down.
My gut feeling is that something like:
dst->input = dst->output = dst_discard;
may be enough. But I don't know where the deliberate race is.
I haven't traced this all of the way through but from the obvious
parts I just get this nagging feeling that something isn't quite
right.
Eric
^ permalink raw reply
* Re: [PATCH 5/7] CAN: Add virtual CAN netdevice driver
From: Eric W. Biederman @ 2007-09-27 16:16 UTC (permalink / raw)
To: Urs Thuermann
Cc: netdev, David Miller, Patrick McHardy, Thomas Gleixner,
Oliver Hartkopp, Oliver Hartkopp, Urs Thuermann, Daniel Lezcano
In-Reply-To: <m1hclgm7dr.fsf@ebiederm.dsl.xmission.com>
I guess in particular IFF_LOOPBACK means that all packets from
a device will come right back to the current machine, and go
nowhere else.
That usage sounds completely different then the CAN usage which
appears to mean. Broadcast packets will be returned to this machine
as well as being sent out onto the wire.
Eric
^ permalink raw reply
* Re: [PATCH 3/4] net ipv4: When possible test for IFF_LOOPBACK and not dev == loopback_dev
From: Eric W. Biederman @ 2007-09-27 16:10 UTC (permalink / raw)
To: Daniel Lezcano; +Cc: David Miller, Linux Containers, netdev, urs
In-Reply-To: <46FB873C.6060200@fr.ibm.com>
Daniel Lezcano <dlezcano@fr.ibm.com> writes:
> Eric W. Biederman wrote:
>> Now that multiple loopback devices are becoming possible it makes
>> the code a little cleaner and more maintainable to test if a deivice
>> is th a loopback device by testing dev->flags & IFF_LOOPBACK instead
>> of dev == loopback_dev.
>>
>> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
>
>
> Urs Thuermann posted the patch:
>
> [PATCH 5/7] CAN: Add virtual CAN netdevice driver
>
> This network driver set its flag to IFF_LOOPBACK for testing.
> Is it possible this can be a collision with your patch ?
I have brought it up on that thread. As best as I tell the CAN usage
of IFF_LOOPBACK will be a problem even without my patch. Assuming
something other then the CAN layer will see the CAN devices.
The CAN documentations IFF_LOOPBACK should be set on all CAN devices.
It seems that the people who want high performance predictable CAN
don't want this and the people who want something they can trace
easily want this.
It sounds to me like CAN routers don't exist.
Anyway hopefully that usage can be resolved as that code is reviewed,
and made ready to merge.
Eric
^ permalink raw reply
* Re: [PATCH 5/7] CAN: Add virtual CAN netdevice driver
From: Eric W. Biederman @ 2007-09-27 15:54 UTC (permalink / raw)
To: Urs Thuermann
Cc: netdev, David Miller, Patrick McHardy, Thomas Gleixner,
Oliver Hartkopp, Oliver Hartkopp, Urs Thuermann, Daniel Lezcano
In-Reply-To: <20070920184533.3795.5@janus.isnogud.escape.de>
Urs Thuermann <urs@isnogud.escape.de> writes:
> This patch adds the virtual CAN bus (vcan) network driver.
> The vcan device is just a loopback device for CAN frames, no
> real CAN hardware is involved.
I'm trying to wrap my head around the CAN use of IFF_LOOPBACK.
> 6.2 loopback
>
> As described in chapter 3.2 the CAN network device driver should
> support a local loopback functionality. In this case the driver flag
> IFF_LOOPBACK has to be set to cause the PF_CAN core to not perform the
> loopback as fallback solution:
>
> dev->flags = (IFF_NOARP | IFF_LOOPBACK);
>
Currently IFF_LOOPBACK set in dev->flags means we are dealing
with drivers/net/loopback.c.
In other networking layers loopback functionality (i.e. for broadcast)
is never expected to be provided by the drivers and is instead
always provided by the networking layer. Keeping the drivers
simpler. Further you already have this functionality in the
generic CAN layer for doing loopback without driver support.
So at a first glance the CAN usage of IFF_LOOPBACK looks completely
broken, and likely to confuse other networking layers if they see
a CAN device. Say if someone attempts to run IP over CAN or
something like that.
Do you think you can remove this incompatible usage of IFF_LOOPBACK
from the can code?
If I have read your documentation properly the only reason you are
doing this is so that the timing of frames to cansniffer more
accurately reflects when the frame hits the wire. If CAN runs over a
very slow medium I guess I can see where that can be a concern. But
the usage of IFF_LOOPBACK to do this still feels fairly hackish
to me.
Eric
^ permalink raw reply
* Re: IPSec on Linux Kernel
From: Rami Rosen @ 2007-09-27 15:23 UTC (permalink / raw)
To: netdev
Hi,Fabio,
- Assuming that you intend to deal with IPV4, I suggest that you will
start by looking at the ah4.ko module sources, which are in net/ipv4/ah.c,
especially at the ah_output() and the ah_input() methods.
(for ipv6 there are the ah6.c in net/ipv6).
- May I ask: are you aware that the Authentication Header protocol deals
only with authentication and not with encryption? and as a result, the ESP
protocol, which supports authentication and also, when needed, encryption, is
much more widely used?
Regards,
Rosen Rami
^ permalink raw reply
* Re: [PATCH] sky2: sky2 FE+ receive status workaround
From: Jochen Voss @ 2007-09-27 15:23 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: Jeff Garzik, netdev
In-Reply-To: <20070927065807.7a71ae89@freepuppy.rosehill>
[-- Attachment #1: Type: text/plain, Size: 931 bytes --]
Hi,
On Thu, Sep 27, 2007 at 06:58:07AM -0700, Stephen Hemminger wrote:
> On Thu, 27 Sep 2007 09:14:11 +0100 Jochen Voß <voss@seehuhn.de> wrote:
> > On 27 Sep 2007, at 01:58, Stephen Hemminger wrote:
> > > + /* This chip has hardware problems that generates bogus status.
> > > + * So do only marginal checking and expect higher level protocols
> > > + * to handle crap frames.
> > > + */
> > > + if (sky2->hw->chip_id == CHIP_ID_YUKON_FE_P &&
> > > + sky2->hw->chip_rev == CHIP_REV_YU_FE2_A0 &&
> > > + length != count)
> > > + goto okay;
> >
> > Shouldn't the condition be "length == count"?
>
> No, the code is correct as is. Basically if length == count, then
> the status field is correct, and the driver can go ahead and use it.
> If length != count, then the status is bogus but the data is okay.
Oh, I see. Thanks for the explanation.
All the best,
Jochen
--
http://seehuhn.de/
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply
* Re: [RFC] af_packet: allow disabling timestamps
From: Unai Uribarri @ 2007-09-27 14:08 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: Eric Dumazet, David S. Miller, Evgeniy Polyakov, netdev
In-Reply-To: <20070914122642.5343b128@oldman>
On vie, 2007-09-14 at 12:26 +0200, Stephen Hemminger wrote:
> On Thu, 13 Sep 2007 14:24:06 +0200
> Eric Dumazet <dada1@cosmosbay.com> wrote:
>
> > On Thu, 13 Sep 2007 12:42:53 +0200
> > Stephen Hemminger <shemminger@linux-foundation.org> wrote:
> >
> > > Currently, af_packet does not allow disabling timestamps. This patch changes
> > > that but doesn't force global timestamps on.
> > >
> > > This shows up in bugzilla as:
> > > http://bugzilla.kernel.org/show_bug.cgi?id=4809
> > >
> > > Patch against net-2.6.24 tree.
> > >
> >
> > I am not sure I understood this patch.
> >
> > This means that tcpdump/ethereal wont get precise timestamps
> > (gathered when packet is received), but imprecise ones (gathered when the sniffer reads the packet)
> >
> > I added some time ago ktime infrastructure to eventually get nanosecond
> > precision in libpcap, so I would prefer a step in the right direction :)
> >
> > Should'nt we use something like :
> >
> > [PATCH] af_packet : allow disabling timestamps, or requesting nanosecond precision.
> >
> > Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>
> >
> > diff --git a/net/core/sock.c b/net/core/sock.c
> > index 5a16e38..1c10b9d 100644
> > --- a/net/core/sock.c
> > +++ b/net/core/sock.c
> > @@ -563,6 +563,7 @@ set_rcvbuf:
> > } else {
> > sock_reset_flag(sk, SOCK_RCVTSTAMP);
> > sock_reset_flag(sk, SOCK_RCVTSTAMPNS);
> > + sock_disable_timestamp(sk);
> > }
> > break;
> >
> > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> > index 745e2cb..409de44 100644
> > --- a/net/packet/af_packet.c
> > +++ b/net/packet/af_packet.c
> > @@ -650,12 +650,27 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev, struct packe
> > h->tp_snaplen = snaplen;
> > h->tp_mac = macoff;
> > h->tp_net = netoff;
> > - if (skb->tstamp.tv64)
> > - tv = ktime_to_timeval(skb->tstamp);
> > - else
> > - do_gettimeofday(&tv);
> > - h->tp_sec = tv.tv_sec;
> > - h->tp_usec = tv.tv_usec;
> > + h->tp_sec = 0;
> > + h->tp_usec = 0;
> > + if ((sock_flag(sk, SOCK_TIMESTAMP))) {
> > + if (sock_flag(sk, SOCK_RCVTSTAMPNS)) {
> > + struct timespec ts;
> > + if (skb->tstamp.tv64)
> > + ts = ktime_to_timespec(skb->tstamp);
> > + else
> > + getnstimeofday(&ts);
> > + h->tp_sec = ts.tv_sec;
> > + h->tp_usec = ts.tv_nsec; /* cheat a litle bit */
> > + }
> > + else {
> > + if (skb->tstamp.tv64)
> > + tv = ktime_to_timeval(skb->tstamp);
> > + else
> > + do_gettimeofday(&tv);
> > + h->tp_sec = tv.tv_sec;
> > + h->tp_usec = tv.tv_usec;
> > + }
> > + }
> >
> > sll = (struct sockaddr_ll*)((u8*)h + TPACKET_ALIGN(sizeof(*h)));
> > sll->sll_halen = 0;
> > @@ -1014,6 +1029,7 @@ static int packet_create(struct net *net, struct socket *sock, int protocol)
> > sock->ops = &packet_ops_spkt;
> >
> > sock_init_data(sock, sk);
> > + sock_enable_timestamp(sk);
> >
> > po = pkt_sk(sk);
> > sk->sk_family = PF_PACKET;
>
> No, then we end up timestamping all the packets, even if they get dropped by
> packet filter. The change in 2.6.24 allows dhclient (and rstp) to only call
> hires clock source for packets they want, not all packets.
>
> Perhaps the timestamping needs to change into a tristate flag?
> -
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
Eric's patch has a feature your previous patch hasn't: a way to disable
timestamping from userspace (the changes at net/core/sock.c). But it
changes the userspace API.
I really think that any developer that sets SO_TIMESTAMP to 0 and still
expect to receive valid timestamp is terminally insane and doesn't
deserve any mercy. But we should take pity of these poor souls that uses
(suffers) closed software and found another way that doesn't changes the
API.
Bye.
^ permalink raw reply
* Re: [RFC] af_packet: allow disabling timestamps
From: Unai Uribarri @ 2007-09-27 14:34 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: David S. Miller, Evgeniy Polyakov, netdev
In-Reply-To: <20070913124253.60da52f2@oldman>
This small modification to Stephen's patch timestamps the skb when
needed, so the timestamp can be reused by other af_packet sockets.
Signed-off-by: Unai Uribarri <unai.uribarri@optenet.com>
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -259,7 +259,8 @@ static void sock_disable_timestamp(struct sock *sk)
{
if (sock_flag(sk, SOCK_TIMESTAMP)) {
sock_reset_flag(sk, SOCK_TIMESTAMP);
- net_disable_timestamp();
+ if (sk->sk_family != PF_PACKET)
+ net_disable_timestamp();
}
}
@@ -1655,7 +1656,8 @@ void sock_enable_timestamp(struct sock *sk)
{
if (!sock_flag(sk, SOCK_TIMESTAMP)) {
sock_set_flag(sk, SOCK_TIMESTAMP);
- net_enable_timestamp();
+ if (sk->sk_family != PF_PACKET)
+ net_enable_timestamp();
}
}
EXPORT_SYMBOL(sock_enable_timestamp);
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -570,7 +570,6 @@ static int tpacket_rcv(struct sk_buff *skb, struct
net_device *dev, struct packe
unsigned long status = TP_STATUS_LOSING|TP_STATUS_USER;
unsigned short macoff, netoff;
struct sk_buff *copy_skb = NULL;
- struct timeval tv;
if (dev->nd_net != &init_net)
goto drop;
@@ -648,12 +647,18 @@ static int tpacket_rcv(struct sk_buff *skb, struct
net_device *dev, struct packe
h->tp_snaplen = snaplen;
h->tp_mac = macoff;
h->tp_net = netoff;
- if (skb->tstamp.tv64)
+
+ if (sock_flag(sk, SOCK_TIMESTAMP)) {
+ struct timeval tv;
+ if (skb->tstamp.tv64 == 0)
+ __net_timestamp(skb);
tv = ktime_to_timeval(skb->tstamp);
- else
- do_gettimeofday(&tv);
- h->tp_sec = tv.tv_sec;
- h->tp_usec = tv.tv_usec;
+ h->tp_sec = tv.tv_sec;
+ h->tp_usec = tv.tv_usec;
+ } else {
+ h->tp_sec = 0;
+ h->tp_usec = 0;
+ }
sll = (struct sockaddr_ll*)((u8*)h + TPACKET_ALIGN(sizeof(*h)));
sll->sll_halen = dev_parse_header(skb, sll->sll_addr);
@@ -1004,6 +1009,7 @@ static int packet_create(struct net *net, struct
socket *sock, int protocol)
sock->ops = &packet_ops_spkt;
sock_init_data(sock, sk);
+ sock_set_flag(sk, SOCK_TIMESTAMP);
po = pkt_sk(sk);
sk->sk_family = PF_PACKET;
On jue, 2007-09-13 at 12:42 +0200, Stephen Hemminger wrote:
> Currently, af_packet does not allow disabling timestamps. This patch changes
> that but doesn't force global timestamps on.
>
> This shows up in bugzilla as:
> http://bugzilla.kernel.org/show_bug.cgi?id=4809
>
> Patch against net-2.6.24 tree.
>
> Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>
>
> --- a/net/core/sock.c 2007-09-12 15:08:43.000000000 +0200
> +++ b/net/core/sock.c 2007-09-13 12:10:19.000000000 +0200
> @@ -259,7 +259,8 @@ static void sock_disable_timestamp(struc
> {
> if (sock_flag(sk, SOCK_TIMESTAMP)) {
> sock_reset_flag(sk, SOCK_TIMESTAMP);
> - net_disable_timestamp();
> + if (sk->sk_family != PF_PACKET)
> + net_disable_timestamp();
> }
> }
>
> @@ -1645,7 +1646,8 @@ void sock_enable_timestamp(struct sock *
> {
> if (!sock_flag(sk, SOCK_TIMESTAMP)) {
> sock_set_flag(sk, SOCK_TIMESTAMP);
> - net_enable_timestamp();
> + if (sk->sk_family != PF_PACKET)
> + net_enable_timestamp();
> }
> }
> EXPORT_SYMBOL(sock_enable_timestamp);
> --- a/net/packet/af_packet.c 2007-09-12 17:07:00.000000000 +0200
> +++ b/net/packet/af_packet.c 2007-09-13 12:09:10.000000000 +0200
> @@ -572,7 +572,6 @@ static int tpacket_rcv(struct sk_buff *s
> unsigned long status = TP_STATUS_LOSING|TP_STATUS_USER;
> unsigned short macoff, netoff;
> struct sk_buff *copy_skb = NULL;
> - struct timeval tv;
>
> if (dev->nd_net != &init_net)
> goto drop;
> @@ -650,12 +649,19 @@ static int tpacket_rcv(struct sk_buff *s
> h->tp_snaplen = snaplen;
> h->tp_mac = macoff;
> h->tp_net = netoff;
> - if (skb->tstamp.tv64)
> - tv = ktime_to_timeval(skb->tstamp);
> - else
> - do_gettimeofday(&tv);
> - h->tp_sec = tv.tv_sec;
> - h->tp_usec = tv.tv_usec;
> +
> + if (sock_flag(sk, SOCK_TIMESTAMP)) {
> + struct timeval tv;
> + if (skb->tstamp.tv64)
> + tv = ktime_to_timeval(skb->tstamp);
> + else
> + do_gettimeofday(&tv);
> + h->tp_sec = tv.tv_sec;
> + h->tp_usec = tv.tv_usec;
> + } else {
> + h->tp_sec = 0;
> + h->tp_usec = 0;
> + }
>
> sll = (struct sockaddr_ll*)((u8*)h + TPACKET_ALIGN(sizeof(*h)));
> sll->sll_halen = 0;
> @@ -1014,6 +1020,7 @@ static int packet_create(struct net *net
> sock->ops = &packet_ops_spkt;
>
> sock_init_data(sock, sk);
> + sock_set_flag(sk, SOCK_TIMESTAMP);
>
> po = pkt_sk(sk);
> sk->sk_family = PF_PACKET;
> -
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: TCP Spike
From: Stephen Hemminger @ 2007-09-27 14:33 UTC (permalink / raw)
To: Majumder, Rajib
Cc: 'netdev@vger.kernel.org',
'linux-kernel@vger.kernel.org'
In-Reply-To: <F444CAE5E62A714C9F45AA292785BED3223F7283@esng11p33001.sg.csfb.com>
On Thu, 27 Sep 2007 11:58:01 +0800
"Majumder, Rajib" <rajib.majumder@credit-suisse.com> wrote:
> Hi,
>
> We have observed 40ms latency spikes in TCP connections in "burst" type of traffic. This affects regular TCP sockets. We observed this issue in kernels of 2.4.21 and kernel 2.6.5.
Unfortunately, 2.6.5 is out of my short term memory at this point. I do remember that 2.6.5
used BIC for congestion control, and there were some math errors in the congestion control
logic that caused it to be way to aggressive.
>
> Aparently, this seems to be fixed in 2.6.19.
>
> Can someone throw some light on this?
My guess is that the addition of the SACK hinting might be the major win. The code
takes 3 passes over the SACK list, so with large outstanding data that was a major
bottleneck, not sure if it was 4ms worth though.
>
> Is this a congestion control/avoidance issue? What congestion control algorithm is used before 2.6.8?
Default congestion control in early 2.6 was BIC, then after CUBIC stabilized it was made the default in 2.6.19.
Another thing that may cause changes in latency is Appropriate Byte Counting (ABC).
It was added in 2.6.14, but then turned off by default in 2.6.18. The problem is
that ABC caused performance problems with some applications that sent messages
as many small writes.
--
Stephen Hemminger <shemminger@linux-foundation.org>
^ permalink raw reply
* IPSec on Linux Kernel
From: Fabio Souto @ 2007-09-27 14:04 UTC (permalink / raw)
To: netdev
Hi,
I'm currently doing some research work and I thought that maybe you guys
could help me out on this.
I'm currently trying to find where can I understand more about the IPSec
implementation on the current Linux Kernel (2.6.22). I need to find where
the AH calls are made so I can reroute those functions calls to an external
module, for a safer AH generation.
It would be helpful to find the source code files where I can study the
IPSec stack in detail, and reroute the function call.
Any hints on these topics?
Thanks in advance
Fabio Souto
Portugal
--
View this message in context: http://www.nabble.com/IPSec-on-Linux-Kernel-tf4528613.html#a12922013
Sent from the netdev mailing list archive at Nabble.com.
^ permalink raw reply
* Re: [PATCH] sky2: sky2 FE+ receive status workaround
From: Stephen Hemminger @ 2007-09-27 13:58 UTC (permalink / raw)
To: Jochen Voß; +Cc: Jeff Garzik, netdev
In-Reply-To: <4C6E8545-ADDC-456E-B17B-B3512A6047D1@seehuhn.de>
On Thu, 27 Sep 2007 09:14:11 +0100
Jochen Voß <voss@seehuhn.de> wrote:
> Hi Stephen,
>
> On 27 Sep 2007, at 01:58, Stephen Hemminger wrote:
> > + /* This chip has hardware problems that generates bogus status.
> > + * So do only marginal checking and expect higher level protocols
> > + * to handle crap frames.
> > + */
> > + if (sky2->hw->chip_id == CHIP_ID_YUKON_FE_P &&
> > + sky2->hw->chip_rev == CHIP_REV_YU_FE2_A0 &&
> > + length != count)
> > + goto okay;
>
> Shouldn't the condition be "length == count"?
>
No, the code is correct as is. Basically if length == count, then
the status field is correct, and the driver can go ahead and use it.
If length != count, then the status is bogus but the data is okay.
--
Stephen Hemminger <shemminger@linux-foundation.org>
^ permalink raw reply
* [PATCH] various dst_ifdown routines to catch refcounting bugs
From: Denis V. Lunev @ 2007-09-27 13:47 UTC (permalink / raw)
To: davem; +Cc: devel, netdev, containers, ebiederm
Moving dst entries into init_net.loopback_dev is not a good thing.
This hides obvious and non-obvious ref-counting bugs.
This patch uses net_ns loopback instead of init_net loopback.
This allowes to catch various bugs like recent one in IPv6 DAD handling.
Signed-off-by: Denis V. Lunev <den@openvz.org>
--- ./net/core/dst.c.loop 2007-08-26 19:30:38.000000000 +0400
+++ ./net/core/dst.c 2007-08-26 19:30:38.000000000 +0400
@@ -279,11 +279,11 @@ static inline void dst_ifdown(struct dst
if (!unregister) {
dst->input = dst->output = dst_discard;
} else {
- dst->dev = init_net.loopback_dev;
+ dst->dev = dst->dev->nd_net->loopback_dev;
dev_hold(dst->dev);
dev_put(dev);
if (dst->neighbour && dst->neighbour->dev == dev) {
- dst->neighbour->dev = init_net.loopback_dev;
+ dst->neighbour->dev = dst->dev;
dev_put(dev);
dev_hold(dst->neighbour->dev);
}
--- ./net/ipv4/route.c.loop 2007-08-26 19:30:38.000000000 +0400
+++ ./net/ipv4/route.c 2007-08-26 19:30:38.000000000 +0400
@@ -1402,8 +1402,9 @@ static void ipv4_dst_ifdown(struct dst_e
{
struct rtable *rt = (struct rtable *) dst;
struct in_device *idev = rt->idev;
- if (dev != init_net.loopback_dev && idev && idev->dev == dev) {
- struct in_device *loopback_idev = in_dev_get(init_net.loopback_dev);
+ if (dev != dev->nd_net->loopback_dev && idev && idev->dev == dev) {
+ struct in_device *loopback_idev =
+ in_dev_get(dev->nd_net->loopback_dev);
if (loopback_idev) {
rt->idev = loopback_idev;
in_dev_put(idev);
--- ./net/ipv4/xfrm4_policy.c.loop 2007-08-26 19:30:38.000000000 +0400
+++ ./net/ipv4/xfrm4_policy.c 2007-08-26 19:30:38.000000000 +0400
@@ -306,7 +306,8 @@ static void xfrm4_dst_ifdown(struct dst_
xdst = (struct xfrm_dst *)dst;
if (xdst->u.rt.idev->dev == dev) {
- struct in_device *loopback_idev = in_dev_get(init_net.loopback_dev);
+ struct in_device *loopback_idev =
+ in_dev_get(dev->nd_net->loopback_dev);
BUG_ON(!loopback_idev);
do {
--- ./net/ipv6/route.c.loop 2007-08-26 19:30:38.000000000 +0400
+++ ./net/ipv6/route.c 2007-08-26 19:30:38.000000000 +0400
@@ -220,9 +220,12 @@ static void ip6_dst_ifdown(struct dst_en
{
struct rt6_info *rt = (struct rt6_info *)dst;
struct inet6_dev *idev = rt->rt6i_idev;
+ struct net_device *loopback_dev =
+ dev->nd_net->loopback_dev;
- if (dev != init_net.loopback_dev && idev != NULL && idev->dev == dev) {
- struct inet6_dev *loopback_idev = in6_dev_get(init_net.loopback_dev);
+ if (dev != loopback_dev && idev != NULL && idev->dev == dev) {
+ struct inet6_dev *loopback_idev =
+ in6_dev_get(loopback_dev);
if (loopback_idev != NULL) {
rt->rt6i_idev = loopback_idev;
in6_dev_put(idev);
@@ -1185,12 +1188,12 @@ int ip6_route_add(struct fib6_config *cf
if ((cfg->fc_flags & RTF_REJECT) ||
(dev && (dev->flags&IFF_LOOPBACK) && !(addr_type&IPV6_ADDR_LOOPBACK))) {
/* hold loopback dev/idev if we haven't done so. */
- if (dev != init_net.loopback_dev) {
+ if (dev != dev->nd_net->loopback_dev) {
if (dev) {
dev_put(dev);
in6_dev_put(idev);
}
- dev = init_net.loopback_dev;
+ dev = dev->nd_net->loopback_dev;
dev_hold(dev);
idev = in6_dev_get(dev);
if (!idev) {
@@ -1894,13 +1897,13 @@ struct rt6_info *addrconf_dst_alloc(stru
if (rt == NULL)
return ERR_PTR(-ENOMEM);
- dev_hold(init_net.loopback_dev);
+ dev_hold(idev->dev->nd_net->loopback_dev);
in6_dev_hold(idev);
rt->u.dst.flags = DST_HOST;
rt->u.dst.input = ip6_input;
rt->u.dst.output = ip6_output;
- rt->rt6i_dev = init_net.loopback_dev;
+ rt->rt6i_dev = idev->dev->nd_net->loopback_dev;
rt->rt6i_idev = idev;
rt->u.dst.metrics[RTAX_MTU-1] = ipv6_get_mtu(rt->rt6i_dev);
rt->u.dst.metrics[RTAX_ADVMSS-1] = ipv6_advmss(dst_mtu(&rt->u.dst));
--- ./net/ipv6/xfrm6_policy.c.loop 2007-08-26 19:30:38.000000000 +0400
+++ ./net/ipv6/xfrm6_policy.c 2007-08-26 19:30:38.000000000 +0400
@@ -375,7 +375,8 @@ static void xfrm6_dst_ifdown(struct dst_
xdst = (struct xfrm_dst *)dst;
if (xdst->u.rt6.rt6i_idev->dev == dev) {
- struct inet6_dev *loopback_idev = in6_dev_get(init_net.loopback_dev);
+ struct inet6_dev *loopback_idev =
+ in6_dev_get(dev->nd_net->loopback_dev);
BUG_ON(!loopback_idev);
do {
--- ./net/xfrm/xfrm_policy.c.loop 2007-08-26 19:30:38.000000000 +0400
+++ ./net/xfrm/xfrm_policy.c 2007-08-26 19:30:38.000000000 +0400
@@ -1949,7 +1949,7 @@ static int stale_bundle(struct dst_entry
void xfrm_dst_ifdown(struct dst_entry *dst, struct net_device *dev)
{
while ((dst = dst->child) && dst->xfrm && dst->dev == dev) {
- dst->dev = init_net.loopback_dev;
+ dst->dev = dev->nd_net->loopback_dev;
dev_hold(dst->dev);
dev_put(dev);
}
^ permalink raw reply
* Re: [PKT_SCHED]: Add stateless NAT
From: Patrick McHardy @ 2007-09-27 13:39 UTC (permalink / raw)
To: Evgeniy Polyakov
Cc: Herbert Xu, David S. Miller, netdev, Alexey Kuznetsov, jamal
In-Reply-To: <20070927132941.GB25353@2ka.mipt.ru>
Evgeniy Polyakov wrote:
> On Thu, Sep 27, 2007 at 09:20:37PM +0800, Herbert Xu (herbert@gondor.apana.org.au) wrote:
>
>>How about putting it in net/core/utils.c?
>
>
> I knew, that was a bad idea to try to fix netfilter dependency :)
>
> diff --git a/include/linux/netfilter.h b/include/linux/netfilter.h
This looks good to me.
^ permalink raw reply
* Re: [PKT_SCHED]: Add stateless NAT
From: jamal @ 2007-09-27 13:34 UTC (permalink / raw)
To: Patrick McHardy
Cc: Evgeniy Polyakov, Herbert Xu, David S. Miller, netdev,
Alexey Kuznetsov
In-Reply-To: <46FBB064.6070508@trash.net>
On Thu, 2007-27-09 at 15:30 +0200, Patrick McHardy wrote:
>
> I like Herbert's suggestion of net/core/utils.c better (and without the
> nf_ prefix please).
me too. Evgeniy, you are the man if you "finish the whole cow" as some
wise Africans would say;->
cheers,
jamal
^ permalink raw reply
* Re: [PKT_SCHED]: Add stateless NAT
From: Evgeniy Polyakov @ 2007-09-27 13:33 UTC (permalink / raw)
To: Patrick McHardy
Cc: Herbert Xu, David S. Miller, netdev, Alexey Kuznetsov, jamal
In-Reply-To: <46FBB064.6070508@trash.net>
On Thu, Sep 27, 2007 at 03:30:12PM +0200, Patrick McHardy (kaber@trash.net) wrote:
> Evgeniy Polyakov wrote:
> > On Thu, Sep 27, 2007 at 03:16:48PM +0200, Patrick McHardy (kaber@trash.net) wrote:
> >
> >>
> >>These are way too large to get inlined, please move somewhere below
> >>net/core.
> >
> >
> > I knew that... :)
> > I'm pretty sure new files called net/core/helpers.c which will host that
> > helper is not a good solution too?
>
>
> I like Herbert's suggestion of net/core/utils.c better (and without the
> nf_ prefix please).
I've put it there without nf_ prefix and updated netfilter header to
create new inlune function with that prefix for private netfilter usage.
--
Evgeniy Polyakov
^ permalink raw reply
* Re: [PKT_SCHED]: Add stateless NAT
From: Patrick McHardy @ 2007-09-27 13:30 UTC (permalink / raw)
To: Evgeniy Polyakov
Cc: Herbert Xu, David S. Miller, netdev, Alexey Kuznetsov, jamal
In-Reply-To: <20070927132507.GA25353@2ka.mipt.ru>
Evgeniy Polyakov wrote:
> On Thu, Sep 27, 2007 at 03:16:48PM +0200, Patrick McHardy (kaber@trash.net) wrote:
>
>>
>>These are way too large to get inlined, please move somewhere below
>>net/core.
>
>
> I knew that... :)
> I'm pretty sure new files called net/core/helpers.c which will host that
> helper is not a good solution too?
I like Herbert's suggestion of net/core/utils.c better (and without the
nf_ prefix please).
^ permalink raw reply
* Re: [PKT_SCHED]: Add stateless NAT
From: Evgeniy Polyakov @ 2007-09-27 13:29 UTC (permalink / raw)
To: Herbert Xu; +Cc: David S. Miller, netdev, Alexey Kuznetsov, jamal
In-Reply-To: <20070927132037.GA17056@gondor.apana.org.au>
On Thu, Sep 27, 2007 at 09:20:37PM +0800, Herbert Xu (herbert@gondor.apana.org.au) wrote:
> How about putting it in net/core/utils.c?
I knew, that was a bad idea to try to fix netfilter dependency :)
diff --git a/include/linux/netfilter.h b/include/linux/netfilter.h
index 1dd075e..51b5a22 100644
--- a/include/linux/netfilter.h
+++ b/include/linux/netfilter.h
@@ -40,6 +40,35 @@
#endif
#ifdef __KERNEL__
+
+static inline void nf_csum_replace4(__sum16 *sum, __be32 from, __be32 to)
+{
+ __be32 diff[] = { ~from, to };
+
+ *sum = csum_fold(csum_partial((char *)diff, sizeof(diff), ~csum_unfold(*sum)));
+}
+
+static inline void nf_csum_replace2(__sum16 *sum, __be16 from, __be16 to)
+{
+ nf_csum_replace4(sum, (__force __be32)from, (__force __be32)to);
+}
+
+extern void proto_csum_replace(__sum16 *sum, struct sk_buff *skb,
+ __be32 from, __be32 to, int pseudohdr);
+
+static inline void nf_proto_csum_replace4(__sum16 *sum, struct sk_buff *skb,
+ __be32 from, __be32 to, int pseudohdr)
+{
+ proto_csum_replace(sum, skb, from, to, pseudohdr);
+}
+
+static inline void nf_proto_csum_replace2(__sum16 *sum, struct sk_buff *skb,
+ __be16 from, __be16 to, int pseudohdr)
+{
+ nf_proto_csum_replace4(sum, skb, (__force __be32)from,
+ (__force __be32)to, pseudohdr);
+}
+
#ifdef CONFIG_NETFILTER
extern void netfilter_init(void);
@@ -289,28 +318,6 @@ extern void nf_invalidate_cache(int pf);
Returns true or false. */
extern int skb_make_writable(struct sk_buff **pskb, unsigned int writable_len);
-static inline void nf_csum_replace4(__sum16 *sum, __be32 from, __be32 to)
-{
- __be32 diff[] = { ~from, to };
-
- *sum = csum_fold(csum_partial((char *)diff, sizeof(diff), ~csum_unfold(*sum)));
-}
-
-static inline void nf_csum_replace2(__sum16 *sum, __be16 from, __be16 to)
-{
- nf_csum_replace4(sum, (__force __be32)from, (__force __be32)to);
-}
-
-extern void nf_proto_csum_replace4(__sum16 *sum, struct sk_buff *skb,
- __be32 from, __be32 to, int pseudohdr);
-
-static inline void nf_proto_csum_replace2(__sum16 *sum, struct sk_buff *skb,
- __be16 from, __be16 to, int pseudohdr)
-{
- nf_proto_csum_replace4(sum, skb, (__force __be32)from,
- (__force __be32)to, pseudohdr);
-}
-
struct nf_afinfo {
unsigned short family;
__sum16 (*checksum)(struct sk_buff *skb, unsigned int hook,
diff --git a/net/core/utils.c b/net/core/utils.c
index 0bf17da..2f6d4d2 100644
--- a/net/core/utils.c
+++ b/net/core/utils.c
@@ -293,3 +293,20 @@ out:
}
EXPORT_SYMBOL(in6_pton);
+
+void proto_csum_replace(__sum16 *sum, struct sk_buff *skb,
+ __be32 from, __be32 to, int pseudohdr)
+{
+ __be32 diff[] = { ~from, to };
+ if (skb->ip_summed != CHECKSUM_PARTIAL) {
+ *sum = csum_fold(csum_partial(diff, sizeof(diff),
+ ~csum_unfold(*sum)));
+ if (skb->ip_summed == CHECKSUM_COMPLETE && pseudohdr)
+ skb->csum = ~csum_partial(diff, sizeof(diff),
+ ~skb->csum);
+ } else if (pseudohdr)
+ *sum = ~csum_fold(csum_partial(diff, sizeof(diff),
+ csum_unfold(*sum)));
+}
+
+EXPORT_SYMBOL(proto_csum_replace);
diff --git a/net/netfilter/core.c b/net/netfilter/core.c
index 381a77c..9ffbbe2 100644
--- a/net/netfilter/core.c
+++ b/net/netfilter/core.c
@@ -226,22 +226,6 @@ copy_skb:
}
EXPORT_SYMBOL(skb_make_writable);
-void nf_proto_csum_replace4(__sum16 *sum, struct sk_buff *skb,
- __be32 from, __be32 to, int pseudohdr)
-{
- __be32 diff[] = { ~from, to };
- if (skb->ip_summed != CHECKSUM_PARTIAL) {
- *sum = csum_fold(csum_partial(diff, sizeof(diff),
- ~csum_unfold(*sum)));
- if (skb->ip_summed == CHECKSUM_COMPLETE && pseudohdr)
- skb->csum = ~csum_partial(diff, sizeof(diff),
- ~skb->csum);
- } else if (pseudohdr)
- *sum = ~csum_fold(csum_partial(diff, sizeof(diff),
- csum_unfold(*sum)));
-}
-EXPORT_SYMBOL(nf_proto_csum_replace4);
-
#if defined(CONFIG_NF_CONNTRACK) || defined(CONFIG_NF_CONNTRACK_MODULE)
/* This does not belong here, but locally generated errors need it if connection
tracking in use: without this, connection may not be in hash table, and hence
--
Evgeniy Polyakov
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox