netdev - Re: [PATCH net-next v2 2/2] xen-netback: Grant copy the header instead of map and memcpy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1396352440.8667.117.camel@kazak.uk.xensource.com>
Date:	Tue, 1 Apr 2014 12:40:40 +0100
From:	Ian Campbell <Ian.Campbell@...rix.com>
To:	Zoltan Kiss <zoltan.kiss@...rix.com>
CC:	<wei.liu2@...rix.com>, <xen-devel@...ts.xenproject.org>,
	<paul.durrant@...rix.com>, <netdev@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <jonathan.davies@...rix.com>
Subject: Re: [PATCH net-next v2 2/2] xen-netback: Grant copy the header
 instead of map and memcpy

On Mon, 2014-03-31 at 16:08 +0100, Zoltan Kiss wrote:
>  
>  check_frags:
> -	for (i = start; i < nr_frags; i++) {
> +	for (i = 0; i < nr_frags; i++, gop_map++) {
>  		int j, newerr;
>  
>  		pending_idx = frag_get_pending_idx(&shinfo->frags[i]);
> -		tx_info = &vif->pending_tx_info[pending_idx];
>  
>  		/* Check error status: if okay then remember grant handle. */
> -		newerr = (++gop_map)->status;
> +		newerr = (gop_map)->status;

You've reworked the handling of gop_map and when and where it is
incremented, which might be a legit cleanup but does it relate to the
bulk of this change somehow that I'm missing?

>  [...] 
>  		__skb_put(skb, data_len);
> +		vif->tx_copy_ops[*copy_ops].source.u.ref = txreq.gref;
> +		vif->tx_copy_ops[*copy_ops].source.domid = vif->domid;
> +		vif->tx_copy_ops[*copy_ops].source.offset = txreq.offset;
> +
> +		vif->tx_copy_ops[*copy_ops].dest.u.gmfn =
> +			virt_to_mfn(skb->data);
> +		vif->tx_copy_ops[*copy_ops].dest.domid = DOMID_SELF;
> +		vif->tx_copy_ops[*copy_ops].dest.offset =
> +			offset_in_page(skb->data);
> +
> +		vif->tx_copy_ops[*copy_ops].len = data_len;
> +		vif->tx_copy_ops[*copy_ops].flags = GNTCOPY_source_gref;

We have gnttab_set_map_op. Should we have gnttap_set_copy_op too?

> -	BUG_ON(ret);
> +	else {
> +		gnttab_batch_copy(vif->tx_copy_ops, nr_cops);
> +		if (nr_mops != 0) {


if (nr_mops) would do.

> +			ret = gnttab_map_refs(vif->tx_map_ops,

So we use gnttab_batch_copy and gnttab_map_refs.

Shouldn't we either use gnttab_batch_copy and gnttab_batch_map or
gnttab_copy gnttab_map_refs. (where gnttab_copy might be a bare
GNTTABOP_copy or might be a helper wrapper).

The point of the batch interface is to handle page unsharing etc, but
doing it only for copies seems like a waste one way or another.

#include <paul's-comments>

Ian.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html