lkml.org 
[lkml]   [2013]   [Dec]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH net-next v2 1/9] xen-netback: Introduce TX grant map definitions
On 13/12/13 15:31, Wei Liu wrote:
> On Thu, Dec 12, 2013 at 11:48:09PM +0000, Zoltan Kiss wrote:
>> This patch contains the new definitions necessary for grant mapping.
>>
>> v2:
>> - move unmapping to separate thread. The NAPI instance has to be scheduled
>> even from thread context, which can cause huge delays
>> - that causes unfortunately bigger struct xenvif
>> - store grant handle after checking validity
>>
>
> If the size of xenvif really becomes a problem, you can try to make
> sratch space like struct gnttab_copy per-cpu. The downside is that
> approach requires much coding and carefully guard against race
> conditions. You would need to consider cost v.s. benefit.

I mentioned this because for the first series I had comments that I
should be more vigilant about this. At that time there was a problem
with struct xenvif allocation which was solved by now. My quick
calculation showed this patch will increase the size with ~15kb

>
>> Signed-off-by: Zoltan Kiss <zoltan.kiss@citrix.com>
>>
>> ---
> [...]
>> #define XENVIF_QUEUE_LENGTH 32
>> #define XENVIF_NAPI_WEIGHT 64
>> diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
>> index c1b7a42..3ddc474 100644
>> --- a/drivers/net/xen-netback/netback.c
>> +++ b/drivers/net/xen-netback/netback.c
>> @@ -772,6 +772,20 @@ static struct page *xenvif_alloc_page(struct xenvif *vif,
>> return page;
>> }
>>
>> +static inline void xenvif_tx_create_gop(struct xenvif *vif, u16 pending_idx,
>> + struct xen_netif_tx_request *txp,
>> + struct gnttab_map_grant_ref *gop)
>> +{
>> + vif->pages_to_map[gop-vif->tx_map_ops] = vif->mmap_pages[pending_idx];
>> + gnttab_set_map_op(gop, idx_to_kaddr(vif, pending_idx),
>> + GNTMAP_host_map | GNTMAP_readonly,
>> + txp->gref, vif->domid);
>> +
>> + memcpy(&vif->pending_tx_info[pending_idx].req, txp,
>> + sizeof(*txp));
>> +
>> +}
>> +
>
> This helper function is not used until next patch. Probably you can move
> it to the second patch.
>
> The same applies to other helper functions as well. Move them to the
> patch they are used. It would be easier for people to review.
I just moved them here because the second patch is already huge, and I
couldn't have an idea to splice it up while keeping it bisectable and
logically consistent. As I mentioned, I welcome ideas about that.

>
>> static struct gnttab_copy *xenvif_get_requests(struct xenvif *vif,
>> struct sk_buff *skb,
>> struct xen_netif_tx_request *txp,
>> @@ -1593,6 +1607,106 @@ static int xenvif_tx_submit(struct xenvif *vif)
>> return work_done;
>> }
>>
>> +void xenvif_zerocopy_callback(struct ubuf_info *ubuf, bool zerocopy_success)
>> +{
>
> Do we care about zerocopy_success? I don't see it used in this function.
It will be used in the 5th patch. Anyway, it's in the definition of the
zerocopy callback.

>
>> + unsigned long flags;
>> + pending_ring_idx_t index;
>> + u16 pending_idx = ubuf->desc;
>> + struct pending_tx_info *temp =
>> + container_of(ubuf, struct pending_tx_info, callback_struct);
>> + struct xenvif *vif =
>> + container_of(temp - pending_idx, struct xenvif,
>> + pending_tx_info[0]);
>> +
>
> The third parameter to container_of should be the name of the member
> within the struct.
Here we have the pending_idx, so we get a pointer for the holding struct
pending_tx_info, then for the beginning of pending_tx_info (temp -
pending_idx), and then to the struct xenvif. It's a bit tricky and not
straightforward, I admit :)

>
>> + spin_lock_irqsave(&vif->dealloc_lock, flags);
>> + do {
>> + pending_idx = ubuf->desc;
>> + ubuf = (struct ubuf_info *) ubuf->ctx;
>> + index = pending_index(vif->dealloc_prod);
>> + vif->dealloc_ring[index] = pending_idx;
>> + /* Sync with xenvif_tx_action_dealloc:
>> + * insert idx then incr producer.
>> + */
>> + smp_wmb();
>> + vif->dealloc_prod++;
>> + } while (ubuf);
>> + wake_up(&vif->dealloc_wq);
>> + spin_unlock_irqrestore(&vif->dealloc_lock, flags);
>> +}
>> +
>> +static inline void xenvif_tx_dealloc_action(struct xenvif *vif)
>> +{
>> + struct gnttab_unmap_grant_ref *gop;
>> + pending_ring_idx_t dc, dp;
>> + u16 pending_idx, pending_idx_release[MAX_PENDING_REQS];
>> + unsigned int i = 0;
>> +
>> + dc = vif->dealloc_cons;
>> + gop = vif->tx_unmap_ops;
>> +
>> + /* Free up any grants we have finished using */
>> + do {
>> + dp = vif->dealloc_prod;
>> +
>> + /* Ensure we see all indices enqueued by netif_idx_release(). */
>
> There is no netif_idx_release in netback code. :-)
Oh yes, that's from the classic code, it should be
xenvif_zerocopy_callback. I will fix it.

>
>> + smp_rmb();
>> +
>> + while (dc != dp) {
>> + pending_idx =
>> + vif->dealloc_ring[pending_index(dc++)];
>> +
>> + /* Already unmapped? */
>> + if (vif->grant_tx_handle[pending_idx] ==
>> + NETBACK_INVALID_HANDLE) {
>> + netdev_err(vif->dev,
>> + "Trying to unmap invalid handle! "
>> + "pending_idx: %x\n", pending_idx);
>> + continue;
>> + }
>
> Should this be BUG_ON? AIUI this kthread should be the only one doing
> unmap, right?
The NAPI instance can do it as well if it is a small packet fits into
PKT_PROT_LEN. But still this scenario shouldn't really happen, I was
just not sure we have to crash immediately. Maybe handle it as a fatal
error and destroy the vif?




\
 
 \ /
  Last update: 2013-12-13 20:01    [W:0.048 / U:0.320 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site