lkml.org 
[lkml]   [2010]   [Mar]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v1 3/3] Let host NIC driver to DMA to guest user space.
    On Sat, Mar 06, 2010 at 05:38:38PM +0800, xiaohui.xin@intel.com wrote:
    > From: Xin Xiaohui <xiaohui.xin@intel.com>
    >
    > The patch let host NIC driver to receive user space skb,
    > then the driver has chance to directly DMA to guest user
    > space buffers thru single ethX interface.
    >
    > Signed-off-by: Xin Xiaohui <xiaohui.xin@intel.com>
    > Signed-off-by: Zhao Yu <yzhao81@gmail.com>
    > Sigend-off-by: Jeff Dike <jdike@c2.user-mode-linux.org>


    I have a feeling I commented on some of the below issues already.
    Do you plan to send a version with comments addressed?

    > ---
    > include/linux/netdevice.h | 76 ++++++++++++++++++++++++++++++++++++++++++-
    > include/linux/skbuff.h | 30 +++++++++++++++--
    > net/core/dev.c | 32 ++++++++++++++++++
    > net/core/skbuff.c | 79 +++++++++++++++++++++++++++++++++++++++++----
    > 4 files changed, 205 insertions(+), 12 deletions(-)
    >
    > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
    > index 94958c1..97bf12c 100644
    > --- a/include/linux/netdevice.h
    > +++ b/include/linux/netdevice.h
    > @@ -485,6 +485,17 @@ struct netdev_queue {
    > unsigned long tx_dropped;
    > } ____cacheline_aligned_in_smp;
    >
    > +#if defined(CONFIG_VHOST_PASSTHRU) || defined(CONFIG_VHOST_PASSTHRU_MODULE)
    > +struct mpassthru_port {
    > + int hdr_len;
    > + int data_len;
    > + int npages;
    > + unsigned flags;
    > + struct socket *sock;
    > + struct skb_user_page *(*ctor)(struct mpassthru_port *,
    > + struct sk_buff *, int);
    > +};
    > +#endif
    >
    > /*
    > * This structure defines the management hooks for network devices.
    > @@ -636,6 +647,10 @@ struct net_device_ops {
    > int (*ndo_fcoe_ddp_done)(struct net_device *dev,
    > u16 xid);
    > #endif
    > +#if defined(CONFIG_VHOST_PASSTHRU) || defined(CONFIG_VHOST_PASSTHRU_MODULE)
    > + int (*ndo_mp_port_prep)(struct net_device *dev,
    > + struct mpassthru_port *port);
    > +#endif
    > };
    >
    > /*
    > @@ -891,7 +906,8 @@ struct net_device
    > struct macvlan_port *macvlan_port;
    > /* GARP */
    > struct garp_port *garp_port;
    > -
    > + /* mpassthru */
    > + struct mpassthru_port *mp_port;
    > /* class/net/name entry */
    > struct device dev;
    > /* space for optional statistics and wireless sysfs groups */
    > @@ -2013,6 +2029,62 @@ static inline u32 dev_ethtool_get_flags(struct net_device *dev)
    > return 0;
    > return dev->ethtool_ops->get_flags(dev);
    > }
    > -#endif /* __KERNEL__ */
    >
    > +#if defined(CONFIG_VHOST_PASSTHRU) || defined(CONFIG_VHOST_PASSTHRU_MODULE)
    > +static inline int netdev_mp_port_prep(struct net_device *dev,
    > + struct mpassthru_port *port)
    > +{

    This function lacks documentation.

    > + int rc;
    > + int npages, data_len;
    > + const struct net_device_ops *ops = dev->netdev_ops;
    > +
    > + /* needed by packet split */
    > + if (ops->ndo_mp_port_prep) {
    > + rc = ops->ndo_mp_port_prep(dev, port);
    > + if (rc)
    > + return rc;
    > + } else { /* should be temp */
    > + port->hdr_len = 128;
    > + port->data_len = 2048;
    > + port->npages = 1;

    where do the numbers come from?

    > + }
    > +
    > + if (port->hdr_len <= 0)
    > + goto err;
    > +
    > + npages = port->npages;
    > + data_len = port->data_len;
    > + if (npages <= 0 || npages > MAX_SKB_FRAGS ||
    > + (data_len < PAGE_SIZE * (npages - 1) ||
    > + data_len > PAGE_SIZE * npages))
    > + goto err;
    > +
    > + return 0;
    > +err:
    > + dev_warn(&dev->dev, "invalid page constructor parameters\n");
    > +
    > + return -EINVAL;
    > +}
    > +
    > +static inline int netdev_mp_port_attach(struct net_device *dev,
    > + struct mpassthru_port *port)
    > +{
    > + if (rcu_dereference(dev->mp_port))
    > + return -EBUSY;
    > +
    > + rcu_assign_pointer(dev->mp_port, port);
    > +
    > + return 0;
    > +}
    > +
    > +static inline void netdev_mp_port_detach(struct net_device *dev)
    > +{
    > + if (!rcu_dereference(dev->mp_port))
    > + return;
    > +
    > + rcu_assign_pointer(dev->mp_port, NULL);
    > + synchronize_rcu();
    > +}

    The above looks wrong, rcu_dereference should be called
    under rcu read side, rcu_assign_pointer usually should not,
    synchronize_rcu definitely should not.

    As I suggested already, these functions are better opencoded,
    rcu is tricky as is without hiding it in inline helpers.

    > +#endif /* CONFIG_VHOST_PASSTHRU */
    > +#endif /* __KERNEL__ */
    > #endif /* _LINUX_NETDEVICE_H */
    > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
    > index df7b23a..e59fa57 100644
    > --- a/include/linux/skbuff.h
    > +++ b/include/linux/skbuff.h
    > @@ -209,6 +209,13 @@ struct skb_shared_info {
    > void * destructor_arg;
    > };
    >
    > +struct skb_user_page {
    > + u8 *start;
    > + int size;
    > + struct skb_frag_struct *frags;
    > + struct skb_shared_info *ushinfo;
    > + void (*dtor)(struct skb_user_page *);
    > +};
    > /* We divide dataref into two halves. The higher 16 bits hold references
    > * to the payload part of skb->data. The lower 16 bits hold references to
    > * the entire skb->data. A clone of a headerless skb holds the length of
    > @@ -441,17 +448,18 @@ extern void kfree_skb(struct sk_buff *skb);
    > extern void consume_skb(struct sk_buff *skb);
    > extern void __kfree_skb(struct sk_buff *skb);
    > extern struct sk_buff *__alloc_skb(unsigned int size,
    > - gfp_t priority, int fclone, int node);
    > + gfp_t priority, int fclone,
    > + int node, struct net_device *dev);
    > static inline struct sk_buff *alloc_skb(unsigned int size,
    > gfp_t priority)
    > {
    > - return __alloc_skb(size, priority, 0, -1);
    > + return __alloc_skb(size, priority, 0, -1, NULL);
    > }
    >
    > static inline struct sk_buff *alloc_skb_fclone(unsigned int size,
    > gfp_t priority)
    > {
    > - return __alloc_skb(size, priority, 1, -1);
    > + return __alloc_skb(size, priority, 1, -1, NULL);
    > }
    >
    > extern int skb_recycle_check(struct sk_buff *skb, int skb_size);
    > @@ -1509,6 +1517,22 @@ static inline void netdev_free_page(struct net_device *dev, struct page *page)
    > __free_page(page);
    > }
    >
    > +extern struct skb_user_page *netdev_alloc_user_pages(struct net_device *dev,
    > + struct sk_buff *skb, int npages);
    > +
    > +static inline struct skb_user_page *netdev_alloc_user_page(
    > + struct net_device *dev,
    > + struct sk_buff *skb, unsigned int size)
    > +{
    > + struct skb_user_page *user;
    > + int npages = (size < PAGE_SIZE) ? 1 : (size / PAGE_SIZE);

    Should round up to full pages?

    > +
    > + user = netdev_alloc_user_pages(dev, skb, npages);
    > + if (likely(user))
    > + return user;
    > + return NULL;
    > +}
    > +
    > /**
    > * skb_clone_writable - is the header of a clone writable
    > * @skb: buffer to check
    > diff --git a/net/core/dev.c b/net/core/dev.c
    > index b8f74cf..ab8b082 100644
    > --- a/net/core/dev.c
    > +++ b/net/core/dev.c
    > @@ -2265,6 +2265,30 @@ void netif_nit_deliver(struct sk_buff *skb)
    > rcu_read_unlock();
    > }
    >
    > +#if defined(CONFIG_VHOST_PASSTHRU) || defined(CONFIG_VHOST_PASSTHRU_MODULE)
    > +static inline struct sk_buff *handle_mpassthru(struct sk_buff *skb,
    > + struct packet_type **pt_prev,
    > + int *ret, struct net_device *orig_dev)

    please document

    pt_prev, orig_dev and ret seem unused?

    > +{
    > + struct mpassthru_port *ctor = NULL;

    Why do you call the port "ctor"?

    > + struct sock *sk = NULL;
    > +
    > + if (skb->dev)
    > + ctor = skb->dev->mp_port;
    > + if (!ctor)
    > + return skb;
    > +
    > + sk = ctor->sock->sk;
    > +
    > + skb_queue_tail(&sk->sk_receive_queue, skb);
    > +
    > + sk->sk_data_ready(sk, skb->len);
    > + return NULL;
    > +}
    > +#else
    > +#define handle_mpassthru(skb, pt_prev, ret, orig_dev) (skb)
    > +#endif
    > +
    > /**
    > * netif_receive_skb - process receive buffer from network
    > * @skb: buffer to process
    > @@ -2342,6 +2366,9 @@ int netif_receive_skb(struct sk_buff *skb)
    > goto out;
    > ncls:
    > #endif
    > + skb = handle_mpassthru(skb, &pt_prev, &ret, orig_dev);
    > + if (!skb)
    > + goto out;
    >
    > skb = handle_bridge(skb, &pt_prev, &ret, orig_dev);
    > if (!skb)
    > @@ -2455,6 +2482,11 @@ int dev_gro_receive(struct napi_struct *napi, struct sk_buff *skb)
    > if (skb_is_gso(skb) || skb_has_frags(skb))
    > goto normal;
    >
    > +#if defined(CONFIG_VHOST_PASSTHRU) || defined(CONFIG_VHOST_PASSTHRU_MODULE)
    > + if (skb->dev && skb->dev->mp_port)
    > + goto normal;
    > +#endif
    > +
    > rcu_read_lock();
    > list_for_each_entry_rcu(ptype, head, list) {
    > if (ptype->type != type || ptype->dev || !ptype->gro_receive)
    > diff --git a/net/core/skbuff.c b/net/core/skbuff.c
    > index 80a9616..6510e5b 100644
    > --- a/net/core/skbuff.c
    > +++ b/net/core/skbuff.c
    > @@ -170,13 +170,15 @@ EXPORT_SYMBOL(skb_under_panic);
    > * %GFP_ATOMIC.
    > */
    > struct sk_buff *__alloc_skb(unsigned int size, gfp_t gfp_mask,
    > - int fclone, int node)
    > + int fclone, int node, struct net_device *dev)
    > {
    > struct kmem_cache *cache;
    > struct skb_shared_info *shinfo;
    > struct sk_buff *skb;
    > u8 *data;
    > -
    > +#if defined(CONFIG_VHOST_PASSTHRU) || defined(CONFIG_VHOST_PASSTHRU_MODULE)
    > + struct skb_user_page *user = NULL;
    > +#endif
    > cache = fclone ? skbuff_fclone_cache : skbuff_head_cache;
    >
    > /* Get the HEAD */
    > @@ -185,8 +187,26 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t gfp_mask,
    > goto out;
    >
    > size = SKB_DATA_ALIGN(size);
    > - data = kmalloc_node_track_caller(size + sizeof(struct skb_shared_info),
    > - gfp_mask, node);
    > +#if defined(CONFIG_VHOST_PASSTHRU) || defined(CONFIG_VHOST_PASSTHRU_MODULE)
    > + if (!dev || !dev->mp_port) { /* Legacy alloc func */
    > +#endif
    > + data = kmalloc_node_track_caller(
    > + size + sizeof(struct skb_shared_info),
    > + gfp_mask, node);
    > +#if defined(CONFIG_VHOST_PASSTHRU) || defined(CONFIG_VHOST_PASSTHRU_MODULE)
    > + } else { /* Allocation may from page constructor of device */

    what does the comment mean?

    > + user = netdev_alloc_user_page(dev, skb, size);
    > + if (!user) {
    > + data = kmalloc_node_track_caller(
    > + size + sizeof(struct skb_shared_info),
    > + gfp_mask, node);
    > + printk(KERN_INFO "can't alloc user buffer.\n");
    > + } else {
    > + data = user->start;
    > + size = SKB_DATA_ALIGN(user->size);
    > + }
    > + }
    > +#endif
    > if (!data)
    > goto nodata;
    >
    > @@ -208,6 +228,11 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t gfp_mask,
    > skb->mac_header = ~0U;
    > #endif
    >
    > +#if defined(CONFIG_VHOST_PASSTHRU) || defined(CONFIG_VHOST_PASSTHRU_MODULE)
    > + if (user)
    > + memcpy(user->ushinfo, skb_shinfo(skb),
    > + sizeof(struct skb_shared_info));
    > +#endif
    > /* make sure we initialize shinfo sequentially */
    > shinfo = skb_shinfo(skb);
    > atomic_set(&shinfo->dataref, 1);
    > @@ -231,6 +256,10 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t gfp_mask,
    >
    > child->fclone = SKB_FCLONE_UNAVAILABLE;
    > }
    > +#if defined(CONFIG_VHOST_PASSTHRU) || defined(CONFIG_VHOST_PASSTHRU_MODULE)
    > + shinfo->destructor_arg = user;
    > +#endif
    > +
    > out:
    > return skb;
    > nodata:
    > @@ -259,7 +288,7 @@ struct sk_buff *__netdev_alloc_skb(struct net_device *dev,
    > int node = dev->dev.parent ? dev_to_node(dev->dev.parent) : -1;
    > struct sk_buff *skb;
    >
    > - skb = __alloc_skb(length + NET_SKB_PAD, gfp_mask, 0, node);
    > + skb = __alloc_skb(length + NET_SKB_PAD, gfp_mask, 0, node, dev);
    > if (likely(skb)) {
    > skb_reserve(skb, NET_SKB_PAD);
    > skb->dev = dev;
    > @@ -278,6 +307,29 @@ struct page *__netdev_alloc_page(struct net_device *dev, gfp_t gfp_mask)
    > }
    > EXPORT_SYMBOL(__netdev_alloc_page);
    >
    > +#if defined(CONFIG_VHOST_PASSTHRU) || defined(CONFIG_VHOST_PASSTHRU_MODULE)
    > +struct skb_user_page *netdev_alloc_user_pages(struct net_device *dev,
    > + struct sk_buff *skb, int npages)
    > +{
    > + struct mpassthru_port *ctor;
    > + struct skb_user_page *user = NULL;
    > +
    > + rcu_read_lock();
    > + ctor = rcu_dereference(dev->mp_port);
    > + if (!ctor)
    > + goto out;
    > +
    > + BUG_ON(npages > ctor->npages);
    > +
    > + user = ctor->ctor(ctor, skb, npages);

    With the assumption that "ctor" pins userspace pages,
    can't it sleep? If yes you can't call it under rcu read side
    critical section.

    > +out:
    > + rcu_read_unlock();
    > +
    > + return user;
    > +}
    > +EXPORT_SYMBOL(netdev_alloc_user_pages);
    > +#endif
    > +
    > void skb_add_rx_frag(struct sk_buff *skb, int i, struct page *page, int off,
    > int size)
    > {
    > @@ -338,6 +390,10 @@ static void skb_clone_fraglist(struct sk_buff *skb)
    >
    > static void skb_release_data(struct sk_buff *skb)
    > {
    > +#if defined(CONFIG_VHOST_PASSTHRU) || defined(CONFIG_VHOST_PASSTHRU_MODULE)
    > + struct skb_user_page *user = skb_shinfo(skb)->destructor_arg;
    > +#endif
    > +
    > if (!skb->cloned ||
    > !atomic_sub_return(skb->nohdr ? (1 << SKB_DATAREF_SHIFT) + 1 : 1,
    > &skb_shinfo(skb)->dataref)) {
    > @@ -349,7 +405,10 @@ static void skb_release_data(struct sk_buff *skb)
    >
    > if (skb_has_frags(skb))
    > skb_drop_fraglist(skb);
    > -
    > +#if defined(CONFIG_VHOST_PASSTHRU) || defined(CONFIG_VHOST_PASSTHRU_MODULE)
    > + if (skb->dev && skb->dev->mp_port && user && user->dtor)
    > + user->dtor(user);
    > +#endif
    > kfree(skb->head);
    > }
    > }
    > @@ -503,8 +562,14 @@ int skb_recycle_check(struct sk_buff *skb, int skb_size)
    > if (skb_shared(skb) || skb_cloned(skb))
    > return 0;
    >
    > - skb_release_head_state(skb);
    > +#if defined(CONFIG_VHOST_PASSTHRU) || defined(CONFIG_VHOST_PASSTHRU_MODULE)
    > + if (skb->dev && skb->dev->mp_port)
    > + return 0;
    > +#endif
    > +
    > shinfo = skb_shinfo(skb);
    > +
    > + skb_release_head_state(skb);
    > atomic_set(&shinfo->dataref, 1);
    > shinfo->nr_frags = 0;
    > shinfo->gso_size = 0;
    > --
    > 1.5.4.4


    \
     
     \ /
      Last update: 2010-03-08 12:25    [W:5.187 / U:0.036 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site