Re: [PATCH -mm 5/9] netconsole: Introduce dev_status member
Hi Stephen,
On Thu, 5 Jul 2007, Stephen Hemminger wrote:
> On Wed, 4 Jul 2007 08:56:42 -0500
> Matt Mackall <[EMAIL PROTECTED]> wrote:
> [...]
> > > + if (nt->dev_status) {
> >
> > Why not simply call net_dev_is_up?
>
> The flags field values are really BSD legacy-ish stuff and should not be
> used internally. IFF_UP etc, do not have the necessary atomic properties.
>
> Use netif_running() instead.
Ok, thanks for letting me know about that. I'll make the change.
Satyam
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH -mm 5/9] netconsole: Introduce dev_status member
On Wed, 4 Jul 2007 08:56:42 -0500
Matt Mackall <[EMAIL PROTECTED]> wrote:
> On Wed, Jul 04, 2007 at 04:38:04PM +0530, Satyam Sharma wrote:
> > From: Satyam Sharma <[EMAIL PROTECTED]>
> >
> > [5/9] netconsole: Introduce dev_status member
> >
> > Introduce a new member in netconsole_target that tracks the status (up or
> > down) of the underlying interface network device that the specific logging
> > target netpoll is attached to.
> >
> > We then join this up with the just-introduced net_device notifier, and
> > introduce NETDEV_UP and NETDEV_DOWN notifications. By disabling the target
> > when the corresponding local interface is down, we save the overhead of
> > unnecessarily disabling interrupts and calling into the netpoll stack in
> > console->write().
>
> Yuck.
>
> > +/*
> > + * Why no net_dev_is_up() in netdevice.h? The kernel could lose a lot of
> > + * weight if only netdevice.h had the good sense to export such a function.
> > + * Oh well ...
> > + */
> > +static inline int net_dev_is_up(struct net_device *net_dev)
> > +{
> > + return ((net_dev->flags & IFF_UP) == IFF_UP);
> > +}
>
> Why editorialize? Why not just add this to netdevice.h?
>
> > + if (nt->dev_status) {
>
> Why not simply call net_dev_is_up?
The flags field values are really BSD legacy-ish stuff and should not be
used internally. IFF_UP etc, do not have the necessary atomic properties.
Use netif_running() instead.
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH -mm 5/9] netconsole: Introduce dev_status member
On Wed, 4 Jul 2007, Joel Becker wrote:
> On Wed, Jul 04, 2007 at 04:38:04PM +0530, Satyam Sharma wrote:
> > - if (!(event == NETDEV_CHANGEADDR || event == NETDEV_CHANGENAME))
> > - goto done;
> > + if (!(event == NETDEV_UP || event == NETDEV_DOWN ||
> > + event == NETDEV_CHANGEADDR || event == NETDEV_CHANGENAME))
> > + goto done;
> >
> > if (nt->np.dev == dev) {
> > switch (event) {
>
> It's a small nit, but isn't the large if() just the degenerate
> default case of the switch()? Why have it at all?
Yes, the large if() is redundant in this particular patch.
But in further patches, the switch() is enclosed inside a
spin_lock_irqsave() and list_for_each_entry(target_list) and so the
large if() upfront saves us from unnecessarily disabling interrupts
and iterating through the entire target_list in the case that it is
a notification for an event that we don't care about.
So I'll remove this if() from this patch and introduce it in the
later one itself, which is where it starts making some sense, anyway.
Satyam
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH -mm 5/9] netconsole: Introduce dev_status member
On Wed, Jul 04, 2007 at 04:38:04PM +0530, Satyam Sharma wrote:
> - if (!(event == NETDEV_CHANGEADDR || event == NETDEV_CHANGENAME))
> - goto done;
> + if (!(event == NETDEV_UP || event == NETDEV_DOWN ||
> + event == NETDEV_CHANGEADDR || event == NETDEV_CHANGENAME))
> + goto done;
>
> if (nt->np.dev == dev) {
> switch (event) {
It's a small nit, but isn't the large if() just the degenerate
default case of the switch()? Why have it at all?
Joel
--
Life's Little Instruction Book #396
"Never give anyone a fruitcake."
Joel Becker
Principal Software Developer
Oracle
E-mail: [EMAIL PROTECTED]
Phone: (650) 506-8127
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH -mm 5/9] netconsole: Introduce dev_status member
On Wed, 4 Jul 2007, Matt Mackall wrote:
> On Wed, Jul 04, 2007 at 04:38:04PM +0530, Satyam Sharma wrote:
> > [...]
> > +/*
> > + * Why no net_dev_is_up() in netdevice.h? The kernel could lose a lot of
> > + * weight if only netdevice.h had the good sense to export such a function.
> > + * Oh well ...
> > + */
> > +static inline int net_dev_is_up(struct net_device *net_dev)
> > +{
> > + return ((net_dev->flags & IFF_UP) == IFF_UP);
> > +}
>
> Why editorialize? Why not just add this to netdevice.h?
Hmm, I wanted to keep all changes local to netconsole, but ...
ok, will do that.
> > + if (nt->dev_status) {
>
> Why not simply call net_dev_is_up?
Yes, I did consider just calling the function here instead of
introducing another member ... again, ok will do that ...
> > We then join this up with the just-introduced net_device notifier, and
> > introduce NETDEV_UP and NETDEV_DOWN notifications. By disabling the target
> > when the corresponding local interface is down, we save the overhead of
> > unnecessarily disabling interrupts and calling into the netpoll stack in
> > console->write().
>
> Yuck.
... so all this will go too.
Satyam
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH -mm 5/9] netconsole: Introduce dev_status member
On Wed, Jul 04, 2007 at 04:38:04PM +0530, Satyam Sharma wrote:
> From: Satyam Sharma <[EMAIL PROTECTED]>
>
> [5/9] netconsole: Introduce dev_status member
>
> Introduce a new member in netconsole_target that tracks the status (up or
> down) of the underlying interface network device that the specific logging
> target netpoll is attached to.
>
> We then join this up with the just-introduced net_device notifier, and
> introduce NETDEV_UP and NETDEV_DOWN notifications. By disabling the target
> when the corresponding local interface is down, we save the overhead of
> unnecessarily disabling interrupts and calling into the netpoll stack in
> console->write().
Yuck.
> +/*
> + * Why no net_dev_is_up() in netdevice.h? The kernel could lose a lot of
> + * weight if only netdevice.h had the good sense to export such a function.
> + * Oh well ...
> + */
> +static inline int net_dev_is_up(struct net_device *net_dev)
> +{
> + return ((net_dev->flags & IFF_UP) == IFF_UP);
> +}
Why editorialize? Why not just add this to netdevice.h?
> + if (nt->dev_status) {
Why not simply call net_dev_is_up?
--
Mathematics is the supreme nostalgia of our time.
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
