Message ID | 1395343961-4744-2-git-send-email-zoltan.kiss@citrix.com |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
On Thu, 2014-03-20 at 19:32 +0000, Zoltan Kiss wrote: > Ian made some late comments about the grant mapping series, I incorporated the > outcomes into this patch. Additional comments, refactoring etc. I don't know how davem feels about merging these kinds of thing into one patch but from my point of view the least thing to do is to enumerate the cleanups -- so that people can verify that the patch does infact do exactly what it says it does, no more, no less. > Signed-off-by: Zoltan Kiss <zoltan.kiss@citrix.com> > --- > diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c > index 6837bfc..fa19538 100644 > --- a/drivers/net/xen-netback/interface.c > +++ b/drivers/net/xen-netback/interface.c > @@ -576,15 +576,15 @@ void xenvif_disconnect(struct xenvif *vif) > void xenvif_free(struct xenvif *vif) > { > int i, unmap_timeout = 0; > - /* Here we want to avoid timeout messages if an skb can be legitimatly > - * stucked somewhere else. Realisticly this could be an another vif's > + /* Here we want to avoid timeout messages if an skb can be legitimately > + * stuck somewhere else. Realistically this could be an another vif's > * internal or QDisc queue. That another vif also has this > * rx_drain_timeout_msecs timeout, but the timer only ditches the > * internal queue. After that, the QDisc queue can put in worst case > * XEN_NETIF_RX_RING_SIZE / MAX_SKB_FRAGS skbs into that another vif's > * internal queue, so we need several rounds of such timeouts until we > * can be sure that no another vif should have skb's from us. We are > - * not sending more skb's, so newly stucked packets are not interesting > + * not sending more skb's, so newly stuck packets are not interesting > * for us here. > */ > unsigned int worst_case_skb_lifetime = (rx_drain_timeout_msecs/1000) * > @@ -599,6 +599,13 @@ void xenvif_free(struct xenvif *vif) > netdev_err(vif->dev, > "Page still granted! Index: %x\n", > i); > + /* If there are still unmapped pages, reset the loop to > + * start mchecking again. We shouldn't exit here until "checking" > + * dealloc thread and NAPI instance release all the > + * pages. If a kernel bug cause the skbs stall "causes the skbs to stall" > + * somewhere, the interface couldn't be brought down > + * properly. I'm not sure what you are saying here exactly. Perhaps you means s/couldn't/cannot/? > + */ > i = -1; > } > } > diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c > index 1c336f6..0e46184b 100644 > --- a/drivers/net/xen-netback/netback.c > +++ b/drivers/net/xen-netback/netback.c > @@ -56,7 +56,7 @@ bool separate_tx_rx_irq = 1; > module_param(separate_tx_rx_irq, bool, 0644); > > /* When guest ring is filled up, qdisc queues the packets for us, but we have > - * to timeout them, otherwise other guests' packets can get stucked there > + * to timeout them, otherwise other guests' packets can get stuck there > */ > unsigned int rx_drain_timeout_msecs = 10000; > module_param(rx_drain_timeout_msecs, uint, 0444); > @@ -99,6 +99,9 @@ static inline unsigned long idx_to_kaddr(struct xenvif *vif, > return (unsigned long)pfn_to_kaddr(idx_to_pfn(vif, idx)); > } > > +#define callback_param(vif, pending_idx) \ > + (vif->pending_tx_info[pending_idx].callback_struct) > + > /* Find the containing VIF's structure from a pointer in pending_tx_info array > */ > static inline struct xenvif* ubuf_to_vif(struct ubuf_info *ubuf) > @@ -1025,12 +1028,12 @@ static void xenvif_fill_frags(struct xenvif *vif, struct sk_buff *skb) > /* 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; > + &callback_param(vif, pending_idx); > 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); > + callback_param(vif, prev_pending_idx).ctx = > + &callback_param(vif, pending_idx); > > - vif->pending_tx_info[pending_idx].callback_struct.ctx = NULL; > + callback_param(vif, pending_idx).ctx = NULL; > prev_pending_idx = pending_idx; > > txp = &vif->pending_tx_info[pending_idx].req; > @@ -1400,13 +1403,13 @@ static int xenvif_tx_submit(struct xenvif *vif) > memcpy(skb->data, > (void *)(idx_to_kaddr(vif, pending_idx)|txp->offset), > data_len); > - vif->pending_tx_info[pending_idx].callback_struct.ctx = NULL; > + callback_param(vif, pending_idx).ctx = NULL; > if (data_len < txp->size) { > /* Append the packet payload as a fragment. */ > txp->offset += data_len; > txp->size -= data_len; > skb_shinfo(skb)->destructor_arg = > - &vif->pending_tx_info[pending_idx].callback_struct; > + &callback_param(vif, pending_idx); > } else { > /* Schedule a response immediately. */ > xenvif_idx_unmap(vif, pending_idx); > @@ -1550,7 +1553,6 @@ static inline void xenvif_tx_dealloc_action(struct xenvif *vif) > idx_to_kaddr(vif, pending_idx), > GNTMAP_host_map, > vif->grant_tx_handle[pending_idx]); > - /* Btw. already unmapped? */ > xenvif_grant_handle_reset(vif, pending_idx); > ++gop; > } > @@ -1683,12 +1685,20 @@ void xenvif_idx_unmap(struct xenvif *vif, u16 pending_idx) > idx_to_kaddr(vif, pending_idx), > GNTMAP_host_map, > vif->grant_tx_handle[pending_idx]); > - /* Btw. already unmapped? */ > xenvif_grant_handle_reset(vif, pending_idx); > > ret = gnttab_unmap_refs(&tx_unmap_op, NULL, > &vif->mmap_pages[pending_idx], 1); > - BUG_ON(ret); > + if (ret) { > + netdev_err(vif->dev, > + "Unmap fail: ret: %d pending_idx: %d host_addr: %llx handle: %x status: %d\n", > + ret, > + pending_idx, > + tx_unmap_op.host_addr, > + tx_unmap_op.handle, > + tx_unmap_op.status); > + BUG(); > + } > > xenvif_idx_release(vif, pending_idx, XEN_NETIF_RSP_OKAY); > } -- 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
From: Zoltan Kiss <zoltan.kiss@citrix.com> Date: Thu, 20 Mar 2014 19:32:41 +0000 > Ian made some late comments about the grant mapping series, I incorporated the > outcomes into this patch. Additional comments, refactoring etc. > > Signed-off-by: Zoltan Kiss <zoltan.kiss@citrix.com> I would really prefer that pure non-functional cleanup changes happen separately from real changes. Thanks. -- 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
On 21/03/14 09:41, Ian Campbell wrote: > On Thu, 2014-03-20 at 19:32 +0000, Zoltan Kiss wrote: >> Ian made some late comments about the grant mapping series, I incorporated the >> outcomes into this patch. Additional comments, refactoring etc. > > I don't know how davem feels about merging these kinds of thing into one > patch but from my point of view the least thing to do is to enumerate > the cleanups -- so that people can verify that the patch does infact do > exactly what it says it does, no more, no less. Ok, I'll make a detailed list in the commit message about what are the changes, and fix up the typos you mentioned below. > >> Signed-off-by: Zoltan Kiss <zoltan.kiss@citrix.com> >> --- >> diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c >> index 6837bfc..fa19538 100644 >> --- a/drivers/net/xen-netback/interface.c >> +++ b/drivers/net/xen-netback/interface.c >> @@ -576,15 +576,15 @@ void xenvif_disconnect(struct xenvif *vif) >> void xenvif_free(struct xenvif *vif) >> { >> int i, unmap_timeout = 0; >> - /* Here we want to avoid timeout messages if an skb can be legitimatly >> - * stucked somewhere else. Realisticly this could be an another vif's >> + /* Here we want to avoid timeout messages if an skb can be legitimately >> + * stuck somewhere else. Realistically this could be an another vif's >> * internal or QDisc queue. That another vif also has this >> * rx_drain_timeout_msecs timeout, but the timer only ditches the >> * internal queue. After that, the QDisc queue can put in worst case >> * XEN_NETIF_RX_RING_SIZE / MAX_SKB_FRAGS skbs into that another vif's >> * internal queue, so we need several rounds of such timeouts until we >> * can be sure that no another vif should have skb's from us. We are >> - * not sending more skb's, so newly stucked packets are not interesting >> + * not sending more skb's, so newly stuck packets are not interesting >> * for us here. >> */ >> unsigned int worst_case_skb_lifetime = (rx_drain_timeout_msecs/1000) * >> @@ -599,6 +599,13 @@ void xenvif_free(struct xenvif *vif) >> netdev_err(vif->dev, >> "Page still granted! Index: %x\n", >> i); >> + /* If there are still unmapped pages, reset the loop to >> + * start mchecking again. We shouldn't exit here until > > "checking" > >> + * dealloc thread and NAPI instance release all the >> + * pages. If a kernel bug cause the skbs stall > > "causes the skbs to stall" > >> + * somewhere, the interface couldn't be brought down >> + * properly. > > I'm not sure what you are saying here exactly. Perhaps you means > s/couldn't/cannot/? > >> + */ >> i = -1; >> } >> } >> diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c >> index 1c336f6..0e46184b 100644 >> --- a/drivers/net/xen-netback/netback.c >> +++ b/drivers/net/xen-netback/netback.c >> @@ -56,7 +56,7 @@ bool separate_tx_rx_irq = 1; >> module_param(separate_tx_rx_irq, bool, 0644); >> >> /* When guest ring is filled up, qdisc queues the packets for us, but we have >> - * to timeout them, otherwise other guests' packets can get stucked there >> + * to timeout them, otherwise other guests' packets can get stuck there >> */ >> unsigned int rx_drain_timeout_msecs = 10000; >> module_param(rx_drain_timeout_msecs, uint, 0444); >> @@ -99,6 +99,9 @@ static inline unsigned long idx_to_kaddr(struct xenvif *vif, >> return (unsigned long)pfn_to_kaddr(idx_to_pfn(vif, idx)); >> } >> >> +#define callback_param(vif, pending_idx) \ >> + (vif->pending_tx_info[pending_idx].callback_struct) >> + >> /* Find the containing VIF's structure from a pointer in pending_tx_info array >> */ >> static inline struct xenvif* ubuf_to_vif(struct ubuf_info *ubuf) >> @@ -1025,12 +1028,12 @@ static void xenvif_fill_frags(struct xenvif *vif, struct sk_buff *skb) >> /* 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; >> + &callback_param(vif, pending_idx); >> 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); >> + callback_param(vif, prev_pending_idx).ctx = >> + &callback_param(vif, pending_idx); >> >> - vif->pending_tx_info[pending_idx].callback_struct.ctx = NULL; >> + callback_param(vif, pending_idx).ctx = NULL; >> prev_pending_idx = pending_idx; >> >> txp = &vif->pending_tx_info[pending_idx].req; >> @@ -1400,13 +1403,13 @@ static int xenvif_tx_submit(struct xenvif *vif) >> memcpy(skb->data, >> (void *)(idx_to_kaddr(vif, pending_idx)|txp->offset), >> data_len); >> - vif->pending_tx_info[pending_idx].callback_struct.ctx = NULL; >> + callback_param(vif, pending_idx).ctx = NULL; >> if (data_len < txp->size) { >> /* Append the packet payload as a fragment. */ >> txp->offset += data_len; >> txp->size -= data_len; >> skb_shinfo(skb)->destructor_arg = >> - &vif->pending_tx_info[pending_idx].callback_struct; >> + &callback_param(vif, pending_idx); >> } else { >> /* Schedule a response immediately. */ >> xenvif_idx_unmap(vif, pending_idx); >> @@ -1550,7 +1553,6 @@ static inline void xenvif_tx_dealloc_action(struct xenvif *vif) >> idx_to_kaddr(vif, pending_idx), >> GNTMAP_host_map, >> vif->grant_tx_handle[pending_idx]); >> - /* Btw. already unmapped? */ >> xenvif_grant_handle_reset(vif, pending_idx); >> ++gop; >> } >> @@ -1683,12 +1685,20 @@ void xenvif_idx_unmap(struct xenvif *vif, u16 pending_idx) >> idx_to_kaddr(vif, pending_idx), >> GNTMAP_host_map, >> vif->grant_tx_handle[pending_idx]); >> - /* Btw. already unmapped? */ >> xenvif_grant_handle_reset(vif, pending_idx); >> >> ret = gnttab_unmap_refs(&tx_unmap_op, NULL, >> &vif->mmap_pages[pending_idx], 1); >> - BUG_ON(ret); >> + if (ret) { >> + netdev_err(vif->dev, >> + "Unmap fail: ret: %d pending_idx: %d host_addr: %llx handle: %x status: %d\n", >> + ret, >> + pending_idx, >> + tx_unmap_op.host_addr, >> + tx_unmap_op.handle, >> + tx_unmap_op.status); >> + BUG(); >> + } >> >> xenvif_idx_release(vif, pending_idx, XEN_NETIF_RSP_OKAY); >> } > > -- 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
On Fri, 2014-03-21 at 19:31 +0000, Zoltan Kiss wrote: > On 21/03/14 09:41, Ian Campbell wrote: > > On Thu, 2014-03-20 at 19:32 +0000, Zoltan Kiss wrote: > >> Ian made some late comments about the grant mapping series, I incorporated the > >> outcomes into this patch. Additional comments, refactoring etc. > > > > I don't know how davem feels about merging these kinds of thing into one > > patch but from my point of view the least thing to do is to enumerate > > the cleanups -- so that people can verify that the patch does infact do > > exactly what it says it does, no more, no less. > Ok, I'll make a detailed list in the commit message about what are the > changes, and fix up the typos you mentioned below. Thanks. Also note Davem's request to split out any real changes from the pure non-functional cleanups. Ian. -- 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
diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c index 6837bfc..fa19538 100644 --- a/drivers/net/xen-netback/interface.c +++ b/drivers/net/xen-netback/interface.c @@ -576,15 +576,15 @@ void xenvif_disconnect(struct xenvif *vif) void xenvif_free(struct xenvif *vif) { int i, unmap_timeout = 0; - /* Here we want to avoid timeout messages if an skb can be legitimatly - * stucked somewhere else. Realisticly this could be an another vif's + /* Here we want to avoid timeout messages if an skb can be legitimately + * stuck somewhere else. Realistically this could be an another vif's * internal or QDisc queue. That another vif also has this * rx_drain_timeout_msecs timeout, but the timer only ditches the * internal queue. After that, the QDisc queue can put in worst case * XEN_NETIF_RX_RING_SIZE / MAX_SKB_FRAGS skbs into that another vif's * internal queue, so we need several rounds of such timeouts until we * can be sure that no another vif should have skb's from us. We are - * not sending more skb's, so newly stucked packets are not interesting + * not sending more skb's, so newly stuck packets are not interesting * for us here. */ unsigned int worst_case_skb_lifetime = (rx_drain_timeout_msecs/1000) * @@ -599,6 +599,13 @@ void xenvif_free(struct xenvif *vif) netdev_err(vif->dev, "Page still granted! Index: %x\n", i); + /* If there are still unmapped pages, reset the loop to + * start mchecking again. We shouldn't exit here until + * dealloc thread and NAPI instance release all the + * pages. If a kernel bug cause the skbs stall + * somewhere, the interface couldn't be brought down + * properly. + */ i = -1; } } diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c index 1c336f6..0e46184b 100644 --- a/drivers/net/xen-netback/netback.c +++ b/drivers/net/xen-netback/netback.c @@ -56,7 +56,7 @@ bool separate_tx_rx_irq = 1; module_param(separate_tx_rx_irq, bool, 0644); /* When guest ring is filled up, qdisc queues the packets for us, but we have - * to timeout them, otherwise other guests' packets can get stucked there + * to timeout them, otherwise other guests' packets can get stuck there */ unsigned int rx_drain_timeout_msecs = 10000; module_param(rx_drain_timeout_msecs, uint, 0444); @@ -99,6 +99,9 @@ static inline unsigned long idx_to_kaddr(struct xenvif *vif, return (unsigned long)pfn_to_kaddr(idx_to_pfn(vif, idx)); } +#define callback_param(vif, pending_idx) \ + (vif->pending_tx_info[pending_idx].callback_struct) + /* Find the containing VIF's structure from a pointer in pending_tx_info array */ static inline struct xenvif* ubuf_to_vif(struct ubuf_info *ubuf) @@ -1025,12 +1028,12 @@ static void xenvif_fill_frags(struct xenvif *vif, struct sk_buff *skb) /* 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; + &callback_param(vif, pending_idx); 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); + callback_param(vif, prev_pending_idx).ctx = + &callback_param(vif, pending_idx); - vif->pending_tx_info[pending_idx].callback_struct.ctx = NULL; + callback_param(vif, pending_idx).ctx = NULL; prev_pending_idx = pending_idx; txp = &vif->pending_tx_info[pending_idx].req; @@ -1400,13 +1403,13 @@ static int xenvif_tx_submit(struct xenvif *vif) memcpy(skb->data, (void *)(idx_to_kaddr(vif, pending_idx)|txp->offset), data_len); - vif->pending_tx_info[pending_idx].callback_struct.ctx = NULL; + callback_param(vif, pending_idx).ctx = NULL; if (data_len < txp->size) { /* Append the packet payload as a fragment. */ txp->offset += data_len; txp->size -= data_len; skb_shinfo(skb)->destructor_arg = - &vif->pending_tx_info[pending_idx].callback_struct; + &callback_param(vif, pending_idx); } else { /* Schedule a response immediately. */ xenvif_idx_unmap(vif, pending_idx); @@ -1550,7 +1553,6 @@ static inline void xenvif_tx_dealloc_action(struct xenvif *vif) idx_to_kaddr(vif, pending_idx), GNTMAP_host_map, vif->grant_tx_handle[pending_idx]); - /* Btw. already unmapped? */ xenvif_grant_handle_reset(vif, pending_idx); ++gop; } @@ -1683,12 +1685,20 @@ void xenvif_idx_unmap(struct xenvif *vif, u16 pending_idx) idx_to_kaddr(vif, pending_idx), GNTMAP_host_map, vif->grant_tx_handle[pending_idx]); - /* Btw. already unmapped? */ xenvif_grant_handle_reset(vif, pending_idx); ret = gnttab_unmap_refs(&tx_unmap_op, NULL, &vif->mmap_pages[pending_idx], 1); - BUG_ON(ret); + if (ret) { + netdev_err(vif->dev, + "Unmap fail: ret: %d pending_idx: %d host_addr: %llx handle: %x status: %d\n", + ret, + pending_idx, + tx_unmap_op.host_addr, + tx_unmap_op.handle, + tx_unmap_op.status); + BUG(); + } xenvif_idx_release(vif, pending_idx, XEN_NETIF_RSP_OKAY); }
Ian made some late comments about the grant mapping series, I incorporated the outcomes into this patch. Additional comments, refactoring etc. Signed-off-by: Zoltan Kiss <zoltan.kiss@citrix.com> --- -- 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