| Message ID | 20100126145738.GA2537@psychotron.lab.eng.brq.redhat.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Jiri Pirko <jpirko@redhat.com> Date: Tue, 26 Jan 2010 15:57:39 +0100 > Temporary macro "netdev_for_each_mc_addr" works in the ugly way, I'm aware, but > it will be replaced. It uses iterator stored in "struct net_device". In every > iteration, it copies addr from the list to "struct netdev_hw_addr" instance > (also stored in "struct net_device"). Driver reads address stored in this > structure. All is protected by addr_list_lock held by a caller. This kind of ugly hack is rarely necessary, so I'm not applying this, sorry. In the macros, use the iterator type the driver instances already use, which is the mcaddr entry pointer. Then by using list_for_each_entry() things should "just work". -- 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
Tue, Feb 02, 2010 at 04:25:33PM CET, davem@davemloft.net wrote: >From: Jiri Pirko <jpirko@redhat.com> >Date: Tue, 26 Jan 2010 15:57:39 +0100 > >> Temporary macro "netdev_for_each_mc_addr" works in the ugly way, I'm aware, but >> it will be replaced. It uses iterator stored in "struct net_device". In every >> iteration, it copies addr from the list to "struct netdev_hw_addr" instance >> (also stored in "struct net_device"). Driver reads address stored in this >> structure. All is protected by addr_list_lock held by a caller. > >This kind of ugly hack is rarely necessary, so I'm not applying >this, sorry. Well, I admit it's ugly but it's only temporary. It would go away once all drivers would be converted to use it (a matter of weeks/month tops I hope). > >In the macros, use the iterator type the driver instances already use, >which is the mcaddr entry pointer. You mean "struct dev_mc_list"? But that would solve nothing. If I would still use current structure in drivers, then still the migration to struct_hw_addr would be all-at-once for all drivers :( This patch was exacly made to avoid this. > >Then by using list_for_each_entry() things should "just work". Maybe I do not uderstand you correctly. Would you please explain? Thanks a lot. Jirka -- 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: Jiri Pirko <jpirko@redhat.com> Date: Tue, 2 Feb 2010 17:03:48 +0100 > You mean "struct dev_mc_list"? But that would solve nothing. If I would still > use current structure in drivers, then still the migration to struct_hw_addr > would be all-at-once for all drivers :( This patch was exacly made to avoid > this. I think changing the iterator type will have to be done wholesale in one changeset, there is no reasonable way to avoid it. -- 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
Tue, Feb 02, 2010 at 05:06:01PM CET, davem@davemloft.net wrote: >From: Jiri Pirko <jpirko@redhat.com> >Date: Tue, 2 Feb 2010 17:03:48 +0100 > >> You mean "struct dev_mc_list"? But that would solve nothing. If I would still >> use current structure in drivers, then still the migration to struct_hw_addr >> would be all-at-once for all drivers :( This patch was exacly made to avoid >> this. > >I think changing the iterator type will have to be done >wholesale in one changeset, there is no reasonable way >to avoid it. So what you are proposing is to change drivers' code to use macro to iterate through lists as step n1 and then change the iterator type as step n2 right? If we would use my patch, we would have this done in single step. But I understand that a kind of ugliness patch introduces is simply not acceptable not even for a short time period, right? Thanks Jirka -- 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: Jiri Pirko <jpirko@redhat.com> Date: Tue, 2 Feb 2010 17:31:08 +0100 > Tue, Feb 02, 2010 at 05:06:01PM CET, davem@davemloft.net wrote: >>From: Jiri Pirko <jpirko@redhat.com> >>Date: Tue, 2 Feb 2010 17:03:48 +0100 >> >>> You mean "struct dev_mc_list"? But that would solve nothing. If I would still >>> use current structure in drivers, then still the migration to struct_hw_addr >>> would be all-at-once for all drivers :( This patch was exacly made to avoid >>> this. >> >>I think changing the iterator type will have to be done >>wholesale in one changeset, there is no reasonable way >>to avoid it. > > So what you are proposing is to change drivers' code to use macro to iterate > through lists as step n1 and then change the iterator type as step n2 right? > > If we would use my patch, we would have this done in single step. But I understand > that a kind of ugliness patch introduces is simply not acceptable not even for a > short time period, right? Right. -- 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/include/linux/netdevice.h b/include/linux/netdevice.h index 93a32a5..e470b22 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -268,6 +268,18 @@ struct netdev_hw_addr_list { #define netdev_for_each_uc_addr(ha, dev) \ list_for_each_entry(ha, &dev->uc.list, list) +#define netdev_mc_count(dev) ((dev)->mc_count) +#define netdev_mc_empty(dev) (netdev_mc_count(dev) == 0) + +/* This is ugly, but only temporary. */ +#define netdev_for_each_mc_addr(ha, dev) \ + for ((dev)->tmp_mc_iter = (dev)->mc_list, \ + netdev_tmp_mc_addr_cpy(dev), \ + ha = &dev->tmp_mc_ha; \ + (dev)->tmp_mc_iter; \ + (dev)->tmp_mc_iter = (dev)->tmp_mc_iter->next, \ + netdev_tmp_mc_addr_cpy(dev)) + struct hh_cache { struct hh_cache *hh_next; /* Next entry */ atomic_t hh_refcnt; /* number of users */ @@ -820,6 +832,8 @@ struct net_device { mac addresses */ int uc_promisc; spinlock_t addr_list_lock; + struct netdev_hw_addr tmp_mc_ha; + struct dev_addr_list *tmp_mc_iter; struct dev_addr_list *mc_list; /* Multicast mac addresses */ int mc_count; /* Number of installed mcasts */ unsigned int promiscuity; @@ -953,6 +967,15 @@ struct net_device { #define NETDEV_ALIGN 32 +/* Used and to be used by netdev_for_each_mc_addr. This will disappear. */ +static inline void netdev_tmp_mc_addr_cpy(struct net_device *dev) +{ + if (dev->tmp_mc_iter) + memcpy(dev->tmp_mc_ha.addr, + dev->tmp_mc_iter->da_addr, + MAX_ADDR_LEN); +} + static inline struct netdev_queue *netdev_get_tx_queue(const struct net_device *dev, unsigned int index)
This patch introduces the similar helpers as those already done for uc list. However multicast lists are no list_head lists but "mademanually". The three macros added by this patch will make the transition of mc_list to list_head smooth. From now on, drivers can (and should) use "netdev_for_each_mc_addr" to iterate over the addresses with iterator of type "struct netdev_hw_addr". Also macros "netdev_mc_count" and "netdev_mc_empty" to read list's length. This is the state which should be reached in all drivers. Temporary macro "netdev_for_each_mc_addr" works in the ugly way, I'm aware, but it will be replaced. It uses iterator stored in "struct net_device". In every iteration, it copies addr from the list to "struct netdev_hw_addr" instance (also stored in "struct net_device"). Driver reads address stored in this structure. All is protected by addr_list_lock held by a caller. Signed-off-by: Jiri Pirko <jpirko@redhat.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