| Message ID | 1429900488-7302-1-git-send-email-sixiao@microsoft.com |
|---|---|
| State | Deferred, archived |
| Delegated to: | David Miller |
| Headers | show |
On Fri, 2015-04-24 at 11:34 -0700, sixiao@microsoft.com wrote: > From: Simon Xiao <sixiao@microsoft.com> > > 1. Introduce netif-msg to netvsc to control debug logging output > and keep msg_enable in netvsc_device_context so that it is > kept persistently. > 2. Only call dump_rndis_message() when NETIF_MSG_RX_ERR or above > is specified in netvsc module debug param. > In non-debug mode, in current code, dump_rndis_message() will not > dump anything but it still initialize some local variables and > process the switch logic which is unnecessary, especially in > high network throughput situation. [] > diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c [] > @@ -888,6 +891,11 @@ static int netvsc_probe(struct hv_device *dev, > > net_device_ctx = netdev_priv(net); > net_device_ctx->device_ctx = dev; > + net_device_ctx->msg_enable = netif_msg_init(debug, default_msg); > + if (netif_msg_probe(net_device_ctx)) > + netdev_dbg(net, "netvsc msg_enable: %d", > + net_device_ctx->msg_enable); Please use newlines to terminate formats. It helps prevent log content interleaving when multiple processes are emitting output at the same time. This could be shortened to use netif_<level> like: netif_dbg(net_device_ctx, probe, net, "netvsc_msg_enable: %d\n", net_device_ctx->msg_enable); -- 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
> -----Original Message----- > From: Joe Perches [mailto:joe@perches.com] > Sent: Friday, April 24, 2015 1:29 PM > To: Simon Xiao > Cc: KY Srinivasan; Haiyang Zhang; devel@linuxdriverproject.org; > netdev@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH net-next,v2,1/1] hv_netvsc: introduce netif-msg into > netvsc module > > On Fri, 2015-04-24 at 11:34 -0700, sixiao@microsoft.com wrote: > > From: Simon Xiao <sixiao@microsoft.com> > > > > 1. Introduce netif-msg to netvsc to control debug logging output and > > keep msg_enable in netvsc_device_context so that it is kept > > persistently. > > 2. Only call dump_rndis_message() when NETIF_MSG_RX_ERR or above is > > specified in netvsc module debug param. > > In non-debug mode, in current code, dump_rndis_message() will not > dump > > anything but it still initialize some local variables and process the > > switch logic which is unnecessary, especially in high network > > throughput situation. > > [] > > > diff --git a/drivers/net/hyperv/netvsc_drv.c > > b/drivers/net/hyperv/netvsc_drv.c > [] > > @@ -888,6 +891,11 @@ static int netvsc_probe(struct hv_device *dev, > > > > net_device_ctx = netdev_priv(net); > > net_device_ctx->device_ctx = dev; > > + net_device_ctx->msg_enable = netif_msg_init(debug, default_msg); > > + if (netif_msg_probe(net_device_ctx)) > > + netdev_dbg(net, "netvsc msg_enable: %d", > > + net_device_ctx->msg_enable); > > Please use newlines to terminate formats. > > It helps prevent log content interleaving when multiple processes are > emitting output at the same time. > > This could be shortened to use netif_<level> like: > > netif_dbg(net_device_ctx, probe, net, "netvsc_msg_enable: %d\n", > net_device_ctx->msg_enable); > Thanks Joe. I would like to leave this to my next patch as there are some places else in netvsc (rndis_filter.c) have the same usage. I would like to fix them in one patch to make them consistent. Thanks, Simon -- 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, 2015-04-24 at 22:52 +0000, Simon Xiao wrote: > > From: Joe Perches [mailto:joe@perches.com] > > On Fri, 2015-04-24 at 11:34 -0700, sixiao@microsoft.com wrote: > > > From: Simon Xiao <sixiao@microsoft.com> > > > > > > 1. Introduce netif-msg to netvsc to control debug logging output and > > > keep msg_enable in netvsc_device_context so that it is kept > > > persistently. [] > > > diff --git a/drivers/net/hyperv/netvsc_drv.c [] > > > + if (netif_msg_probe(net_device_ctx)) > > > + netdev_dbg(net, "netvsc msg_enable: %d", > > > + net_device_ctx->msg_enable); > > > > Please use newlines to terminate formats. > > > > It helps prevent log content interleaving when multiple processes are > > emitting output at the same time. > > > > This could be shortened to use netif_<level> like: > > > > netif_dbg(net_device_ctx, probe, net, "netvsc_msg_enable: %d\n", > > net_device_ctx->msg_enable); > > > > Thanks Joe. I would like to leave this to my next patch as there are some places else in netvsc (rndis_filter.c) > have the same usage. I would like to fix them in one patch to make them consistent. Oh sure no worries. It is nicer though to not introduce new formats with missing newline defects. cheers, Joe -- 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: sixiao@microsoft.com Date: Fri, 24 Apr 2015 11:34:48 -0700 > + net_device_ctx->msg_enable = netif_msg_init(debug, default_msg); > + if (netif_msg_probe(net_device_ctx)) > + netdev_dbg(net, "netvsc msg_enable: %d", > + net_device_ctx->msg_enable); > + I expect you to respin this and fix the newline problem. Also, net-next is not open for submissions yet. -- 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/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h index a10b316..e55c8f4 100644 --- a/drivers/net/hyperv/hyperv_net.h +++ b/drivers/net/hyperv/hyperv_net.h @@ -612,6 +612,15 @@ struct multi_send_data { u32 count; /* counter of batched packets */ }; +/* The context of the netvsc device */ +struct net_device_context { + /* point back to our device context */ + struct hv_device *device_ctx; + struct delayed_work dwork; + struct work_struct work; + u32 msg_enable; /* debug level */ +}; + /* Per netvsc device */ struct netvsc_device { struct hv_device *dev; @@ -667,6 +676,9 @@ struct netvsc_device { struct multi_send_data msd[NR_CPUS]; u32 max_pkt; /* max number of pkt in one send, e.g. 8 */ u32 pkt_align; /* alignment bytes, e.g. 8 */ + + /* The net device context */ + struct net_device_context *nd_ctx; }; /* NdisInitialize message */ diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c index 2e8ad06..c651d4d 100644 --- a/drivers/net/hyperv/netvsc.c +++ b/drivers/net/hyperv/netvsc.c @@ -1197,6 +1197,9 @@ int netvsc_device_add(struct hv_device *device, void *additional_info) */ ndev = net_device->ndev; + /* Add netvsc_device context to netvsc_device */ + net_device->nd_ctx = netdev_priv(ndev); + /* Initialize the NetVSC channel extension */ init_completion(&net_device->channel_init_wait); diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c index a3a9d38..a9609d2 100644 --- a/drivers/net/hyperv/netvsc_drv.c +++ b/drivers/net/hyperv/netvsc_drv.c @@ -40,18 +40,21 @@ #include "hyperv_net.h" -struct net_device_context { - /* point back to our device context */ - struct hv_device *device_ctx; - struct delayed_work dwork; - struct work_struct work; -}; #define RING_SIZE_MIN 64 static int ring_size = 128; module_param(ring_size, int, S_IRUGO); MODULE_PARM_DESC(ring_size, "Ring buffer size (# of pages)"); +static const u32 default_msg = NETIF_MSG_DRV | NETIF_MSG_PROBE | + NETIF_MSG_LINK | NETIF_MSG_IFUP | + NETIF_MSG_IFDOWN | NETIF_MSG_RX_ERR | + NETIF_MSG_TX_ERR; + +static int debug = -1; +module_param(debug, int, S_IRUGO); +MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)"); + static void do_set_multicast(struct work_struct *w) { struct net_device_context *ndevctx = @@ -888,6 +891,11 @@ static int netvsc_probe(struct hv_device *dev, net_device_ctx = netdev_priv(net); net_device_ctx->device_ctx = dev; + net_device_ctx->msg_enable = netif_msg_init(debug, default_msg); + if (netif_msg_probe(net_device_ctx)) + netdev_dbg(net, "netvsc msg_enable: %d", + net_device_ctx->msg_enable); + hv_set_drvdata(dev, net); INIT_DELAYED_WORK(&net_device_ctx->dwork, netvsc_link_change); INIT_WORK(&net_device_ctx->work, do_set_multicast); diff --git a/drivers/net/hyperv/rndis_filter.c b/drivers/net/hyperv/rndis_filter.c index 0d92efe..9118cea 100644 --- a/drivers/net/hyperv/rndis_filter.c +++ b/drivers/net/hyperv/rndis_filter.c @@ -429,7 +429,8 @@ int rndis_filter_receive(struct hv_device *dev, rndis_msg = pkt->data; - dump_rndis_message(dev, rndis_msg); + if (netif_msg_rx_err(net_dev->nd_ctx)) + dump_rndis_message(dev, rndis_msg); switch (rndis_msg->ndis_msg_type) { case RNDIS_MSG_PACKET: