lkml.org 
[lkml]   [2014]   [Mar]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    SubjectRe: [PATCH net-next v7 4/9] xen-netback: Introduce TX grant mapping
    From
    Date
    On Thu, 2014-03-06 at 21:48 +0000, Zoltan Kiss wrote:
    > @@ -135,13 +146,31 @@ struct xenvif {
    > pending_ring_idx_t pending_cons;
    > u16 pending_ring[MAX_PENDING_REQS];
    > struct pending_tx_info pending_tx_info[MAX_PENDING_REQS];
    > + grant_handle_t grant_tx_handle[MAX_PENDING_REQS];
    >
    > /* Coalescing tx requests before copying makes number of grant
    > * copy ops greater or equal to number of slots required. In
    > * worst case a tx request consumes 2 gnttab_copy.
    > */
    > struct gnttab_copy tx_copy_ops[2*MAX_PENDING_REQS];
    > -
    > + struct gnttab_map_grant_ref tx_map_ops[MAX_PENDING_REQS];
    > + struct gnttab_unmap_grant_ref tx_unmap_ops[MAX_PENDING_REQS];

    I wonder if we should break some of these arrays into separate
    allocations? Wasn't there a problem with sizeof(struct xenvif) at one
    point?

    > diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c
    > index bc32627..1fe9fe5 100644
    > --- a/drivers/net/xen-netback/interface.c
    > +++ b/drivers/net/xen-netback/interface.c
    > @@ -493,6 +533,23 @@ void xenvif_disconnect(struct xenvif *vif)
    >
    > void xenvif_free(struct xenvif *vif)
    > {
    > + int i, unmap_timeout = 0;
    > +
    > + for (i = 0; i < MAX_PENDING_REQS; ++i) {
    > + if (vif->grant_tx_handle[i] != NETBACK_INVALID_HANDLE) {
    > + unmap_timeout++;
    > + schedule_timeout(msecs_to_jiffies(1000));
    > + if (unmap_timeout > 9 &&
    > + net_ratelimit())

    Does this really reach 80 columns when unwrapped?

    (there seems to my eye to be a lot of overaggressive wrapping in this
    patch, but nevermind)

    > + netdev_err(vif->dev,
    > + "Page still granted! Index: %x\n",
    > + i);
    > + i = -1;

    Should there not be a break here? Otherwise don't we restart the for
    loop from 0 again? If that is intentional then a comment would be very
    useful.

    > @@ -919,11 +873,38 @@ err:
    > return NULL;
    > }
    >
    > +static inline void xenvif_grant_handle_set(struct xenvif *vif,
    > + u16 pending_idx,
    > + grant_handle_t handle)
    > +{
    > + if (unlikely(vif->grant_tx_handle[pending_idx] !=
    > + NETBACK_INVALID_HANDLE)) {
    > + netdev_err(vif->dev,

    Is this in any way guest triggerable? Needs to be ratelimited in that
    case (and arguably even if not?)

    > + "Trying to overwrite active handle! pending_idx: %x\n",
    > + pending_idx);
    > + BUG();
    > + }
    > + vif->grant_tx_handle[pending_idx] = handle;
    > +}
    > +
    > +static inline void xenvif_grant_handle_reset(struct xenvif *vif,
    > + u16 pending_idx)
    > +{
    > + if (unlikely(vif->grant_tx_handle[pending_idx] ==
    > + NETBACK_INVALID_HANDLE)) {
    > + netdev_err(vif->dev,

    Likewise.

    > + "Trying to unmap invalid handle! pending_idx: %x\n",
    > + pending_idx);
    > + BUG();
    > + }
    > + vif->grant_tx_handle[pending_idx] = NETBACK_INVALID_HANDLE;
    > +}
    > +
    > @@ -1001,6 +982,17 @@ static void xenvif_fill_frags(struct xenvif *vif, struct sk_buff *skb)
    >
    > pending_idx = frag_get_pending_idx(frag);
    >
    > + /* If this is not the first frag, chain it to the previous*/
    > + if (unlikely(prev_pending_idx == INVALID_PENDING_IDX))
    > + skb_shinfo(skb)->destructor_arg =
    > + &vif->pending_tx_info[pending_idx].callback_struct;
    > + else if (likely(pending_idx != prev_pending_idx))
    > + vif->pending_tx_info[prev_pending_idx].callback_struct.ctx =
    > + &(vif->pending_tx_info[pending_idx].callback_struct);

    #define callback_for(vif, pending_idx) .... would make this and a bunch
    of other places a lot less verbose IMHO.

    > + index = pending_index(vif->pending_prod);
    > + vif->pending_ring[index] = pending_idx;
    > + /* TX shouldn't use the index before we give it back here */

    I hope this comment refers to the pending_prod++ and not the mb(), since
    the barrier only guarantees visibility after that point, but not
    invisibility before this point.

    [...]
    > + /* Btw. already unmapped? */

    What does this comment mean? Is it a fixme? An indicator that
    xenvif_grant_handle_reset is supposed to handle this case or something
    else?

    I think there was another such comment earlier too.
    > + xenvif_grant_handle_reset(vif, pending_idx);
    > +
    > + ret = gnttab_unmap_refs(&tx_unmap_op, NULL,
    > + &vif->mmap_pages[pending_idx], 1);
    > + BUG_ON(ret);
    > +
    > + xenvif_idx_release(vif, pending_idx, XEN_NETIF_RSP_OKAY);
    > +}
    > +
    > static inline int rx_work_todo(struct xenvif *vif)
    > {
    > return !skb_queue_empty(&vif->rx_queue) &&




    \
     
     \ /
      Last update: 2014-03-14 00:21    [W:3.528 / U:0.048 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site